Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(runner): add custom folder support to -G/-A #4792

Merged
merged 5 commits into from
Mar 20, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lighthouse-cli/cli-flags.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ function getFlags(manualArgv) {
'disable-cpu-throttling': 'Disable CPU throttling',
'disable-network-throttling': 'Disable network throttling',
'gather-mode':
'Collect artifacts from a connected browser and save to disk. If audit-mode is not also enabled, the run will quit early.',
'audit-mode': 'Process saved artifacts from disk',
'Collect artifacts from a connected browser and save to disk. (Artifacts folder path may optionally be provided). If audit-mode is not also enabled, the run will quit early.',
'audit-mode': 'Process saved artifacts from disk. (Artifacts folder path may be provided, otherwise defaults to ./latest-run/)',
'save-assets': 'Save the trace contents & screenshots to disk',
'list-all-audits': 'Prints a list of all available audits and exits',
'list-trace-categories': 'Prints a list of all required trace categories and exits',
Expand Down Expand Up @@ -111,7 +111,7 @@ function getFlags(manualArgv) {
'disable-storage-reset', 'disable-device-emulation', 'disable-cpu-throttling',
'disable-network-throttling', 'save-assets', 'list-all-audits',
'list-trace-categories', 'perf', 'view', 'verbose', 'quiet', 'help',
'gather-mode', 'audit-mode', 'mixed-content',
'mixed-content',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mention path option in description?

])
.choices('output', printer.getValidOutputOptions())
// force as an array
Expand Down
43 changes: 29 additions & 14 deletions lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ const path = require('path');
const URL = require('./lib/url-shim');
const Sentry = require('./lib/sentry');

const basePath = path.join(process.cwd(), 'latest-run');

class Runner {
static run(connection, opts) {
// Clean opts input.
Expand Down Expand Up @@ -68,7 +66,8 @@ class Runner {
// Gather phase
// Either load saved artifacts off disk, from config, or get from the browser
if (opts.flags.auditMode && !opts.flags.gatherMode) {
run = run.then(_ => Runner._loadArtifactsFromDisk());
const path = Runner._getArtifactsPath(opts.flags);
run = run.then(_ => Runner._loadArtifactsFromDisk(path));
} else if (opts.config.artifacts) {
run = run.then(_ => opts.config.artifacts);
} else {
Expand Down Expand Up @@ -123,10 +122,11 @@ class Runner {

/**
* No browser required, just load the artifacts from disk
* @param {string} path
* @return {!Promise<!Artifacts>}
*/
static _loadArtifactsFromDisk() {
return assetSaver.loadArtifacts(basePath);
static _loadArtifactsFromDisk(path) {
return assetSaver.loadArtifacts(path);
}

/**
Expand All @@ -135,27 +135,30 @@ class Runner {
* @param {*} connection
* @return {!Promise<!Artifacts>}
*/
static _gatherArtifactsFromBrowser(opts, connection) {
static async _gatherArtifactsFromBrowser(opts, connection) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fun!

if (!opts.config.passes) {
return Promise.reject(new Error('No browser artifacts are either provided or requested.'));
}

opts.driver = opts.driverMock || new Driver(connection);
return GatherRunner.run(opts.config.passes, opts).then(artifacts => {
const flags = opts.flags;
const shouldSave = flags.gatherMode;
const p = shouldSave ? Runner._saveArtifacts(artifacts): Promise.resolve();
return p.then(_ => artifacts);
});
const artifacts = await GatherRunner.run(opts.config.passes, opts);

const flags = opts.flags;
if (flags.gatherMode) {
const path = Runner._getArtifactsPath(flags);
await Runner._saveArtifacts(artifacts, path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inherited, but this would probably make more sense back up in run()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true. but how about we do that when runner goes async/await?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well you're already refactoring it to async/await so you brought it on yourself :P

It doesn't look like there are any tests of this method (since it's private, yay), so should pop into the surrounding else branch with no side effects?

}
return artifacts;
}

/**
* Save collected artifacts to disk
* @param {!Artifacts} artifacts
* @param {string} path
* @return {!Promise>}
*/
static _saveArtifacts(artifacts) {
return assetSaver.saveArtifacts(artifacts, basePath);
static _saveArtifacts(artifacts, path) {
return assetSaver.saveArtifacts(artifacts, path);
}

/**
Expand Down Expand Up @@ -410,6 +413,18 @@ class Runner {
extraHeaders: flags.extraHeaders || {},
};
}

/**
* Get path to use for -G and -A modes. Defaults to $CWD/latest-run
* @param {Flags} flags
* @return {string}
*/
static _getArtifactsPath(flags) {
// This enables usage like: -GA=./custom-folder
if (typeof flags.auditMode === 'string') return path.join(process.cwd(), flags.auditMode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want path.resolve? To handle absolute paths

if (typeof flags.gatherMode === 'string') return path.join(process.cwd(), flags.gatherMode);
return path.join(process.cwd(), 'latest-run');
}
}

module.exports = Runner;
19 changes: 15 additions & 4 deletions lighthouse-core/test/runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ const driverMock = require('./gather/fake-driver');
const Config = require('../config/config');
const Audit = require('../audits/audit');
const assetSaver = require('../lib/asset-saver');
const fs = require('fs');
const assert = require('assert');
const path = require('path');
const sinon = require('sinon');
const rimraf = require('rimraf');

const computedArtifacts = Runner.instantiateComputedArtifacts();

Expand All @@ -36,17 +38,23 @@ describe('Runner', () => {
resetSpies();
});

describe('Gather Mode & Audit Mode', () => {
describe.only('Gather Mode & Audit Mode', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

const url = 'https://example.com';
const generateConfig = _ => new Config({
passes: [{
gatherers: ['viewport-dimensions'],
}],
audits: ['content-width'],
});
const artifactsPath = '.tmp/test_artifacts';
const resolvedPath = Runner._getArtifactsPath({auditMode: artifactsPath});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the expected behavior is that we support a path relative to cwd, I think we want an impl independent of the thing itself to test it (e.g. const resolvedPath = path.resolve(process.cwd(), artifactsPath) or whatever works from this file)


after(() => {
rimraf.sync(resolvedPath);
});

it('-G gathers, quits, and doesn\'t run audits', () => {
const opts = {url, config: generateConfig(), driverMock, flags: {gatherMode: true}};
const opts = {url, config: generateConfig(), driverMock, flags: {gatherMode: artifactsPath}};
return Runner.run(null, opts).then(_ => {
assert.equal(loadArtifactsSpy.called, false, 'loadArtifacts was called');

Expand All @@ -57,12 +65,15 @@ describe('Runner', () => {

assert.equal(gatherRunnerRunSpy.called, true, 'GatherRunner.run was not called');
assert.equal(runAuditSpy.called, false, '_runAudit was called');

assert.ok(fs.existsSync(resolvedPath));
assert.ok(fs.existsSync(`${resolvedPath}/artifacts.json`));
});
});

// uses the files on disk from the -G test. ;)
it('-A audits from saved artifacts and doesn\'t gather', () => {
const opts = {url, config: generateConfig(), driverMock, flags: {auditMode: true}};
const opts = {url, config: generateConfig(), driverMock, flags: {auditMode: artifactsPath}};
return Runner.run(null, opts).then(_ => {
assert.equal(loadArtifactsSpy.called, true, 'loadArtifacts was not called');
assert.equal(gatherRunnerRunSpy.called, false, 'GatherRunner.run was called');
Expand All @@ -73,7 +84,7 @@ describe('Runner', () => {

it('-GA is a normal run but it saves artifacts to disk', () => {
const opts = {url, config: generateConfig(), driverMock,
flags: {auditMode: true, gatherMode: true}};
flags: {auditMode: artifactsPath, gatherMode: artifactsPath}};
return Runner.run(null, opts).then(_ => {
assert.equal(loadArtifactsSpy.called, false, 'loadArtifacts was called');
assert.equal(gatherRunnerRunSpy.called, true, 'GatherRunner.run was not called');
Expand Down
4 changes: 4 additions & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,10 @@ lighthouse -A http://example.com

lighthouse -GA http://example.com
# Normal gather + audit run, but also saves collected artifacts to disk for subsequent -A runs.


# You can optionally provide a custom folder destination to -G/-A/-GA. Without a value, the default will be `$PWD/latest-run`.
lighthouse -GA=./gmailartifacts https://gmail.com
```


Expand Down
4 changes: 2 additions & 2 deletions typings/externs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ declare namespace LH {
enableErrorReporting: boolean;
listAllAudits: boolean;
listTraceCategories: boolean;
auditMode: boolean;
gatherMode: boolean;
auditMode: boolean|string;
gatherMode: boolean|string;
configPath?: string;
perf: boolean;
mixedContent: boolean;
Expand Down