Skip to content

Commit

Permalink
core(runner): abstract gather phase out of runner
Browse files Browse the repository at this point in the history
  • Loading branch information
@patrickhulce
patrickhulce committed Nov 2, 2020
1 parent 636b55d commit 6c6214d
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 28 deletions.
14 changes: 9 additions & 5 deletions lighthouse-core/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,24 @@ const Config = require('./config/config.js');
* they will override any settings in the config.
* @param {LH.Config.Json=} configJSON Configuration for the Lighthouse run. If
* not present, the default config is used.
* @param {Connection=} connection
* @param {Connection=} userConnection
* @return {Promise<LH.RunnerResult|undefined>}
*/
async function lighthouse(url, flags = {}, configJSON, connection) {
async function lighthouse(url, flags = {}, configJSON, userConnection) {
// set logging preferences, assume quiet
flags.logLevel = flags.logLevel || 'error';
log.setLevel(flags.logLevel);

const config = generateConfig(configJSON, flags);

connection = connection || new ChromeProtocol(flags.port, flags.hostname);
const options = {url, config};
const connection = userConnection || new ChromeProtocol(flags.port, flags.hostname);

// kick off a lighthouse run
return Runner.run(connection, {url, config});
/** @param {{requestedUrl: string}} runnerData */
const gatherFn = ({requestedUrl}) => {
return Runner._gatherArtifactsFromBrowser(requestedUrl, options, connection);
};
return Runner.run(gatherFn, options);
}

/**
Expand Down
11 changes: 8 additions & 3 deletions lighthouse-core/runner.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ const LHError = require('./lib/lh-error.js');

class Runner {
/**
* @param {Connection} connection
* @param {(runnerData: {requestedUrl: string, config: Config, driverMock?: Driver}) => Promise<LH.Artifacts>} gatherFn
* @param {{config: Config, url?: string, driverMock?: Driver}} runOpts
* @return {Promise<LH.RunnerResult|undefined>}
*/
static async run(connection, runOpts) {
static async run(gatherFn, runOpts) {
const settings = runOpts.config.settings;
try {
const runnerStatus = {msg: 'Runner setup', id: 'lh:runner:run'};
Expand Down Expand Up @@ -78,7 +78,12 @@ class Runner {
throw new LHError(LHError.errors.INVALID_URL);
}

artifacts = await Runner._gatherArtifactsFromBrowser(requestedUrl, runOpts, connection);
artifacts = await gatherFn({
requestedUrl,
config: runOpts.config,
driverMock: runOpts.driverMock,
});

// -G means save these to ./latest-run, etc.
if (settings.gatherMode) {
const path = Runner._getDataSavePath(settings);
Expand Down
46 changes: 26 additions & 20 deletions lighthouse-core/test/runner-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ const i18n = require('../lib/i18n/i18n.js');
jest.mock('../lib/stack-collector.js', () => () => Promise.resolve([]));

describe('Runner', () => {
const defaultGatherFn = opts => Runner._gatherArtifactsFromBrowser(
opts.requestedUrl,
opts,
null
);

/** @type {jest.Mock} */
let saveArtifactsSpy;
/** @type {jest.Mock} */
Expand Down Expand Up @@ -76,7 +82,7 @@ describe('Runner', () => {

it('-G gathers, quits, and doesn\'t run audits', () => {
const opts = {url, config: generateConfig({gatherMode: artifactsPath}), driverMock};
return Runner.run(null, opts).then(_ => {
return Runner.run(defaultGatherFn, opts).then(_ => {
expect(loadArtifactsSpy).not.toHaveBeenCalled();
expect(saveArtifactsSpy).toHaveBeenCalled();

Expand All @@ -96,7 +102,7 @@ describe('Runner', () => {
// uses the files on disk from the -G test. ;)
it('-A audits from saved artifacts and doesn\'t gather', () => {
const opts = {config: generateConfig({auditMode: artifactsPath}), driverMock};
return Runner.run(null, opts).then(_ => {
return Runner.run(defaultGatherFn, opts).then(_ => {
expect(loadArtifactsSpy).toHaveBeenCalled();
expect(gatherRunnerRunSpy).not.toHaveBeenCalled();
expect(saveArtifactsSpy).not.toHaveBeenCalled();
Expand All @@ -109,7 +115,7 @@ describe('Runner', () => {
const settings = {auditMode: artifactsPath, emulatedFormFactor: 'desktop'};
const opts = {config: generateConfig(settings), driverMock};
try {
await Runner.run(null, opts);
await Runner.run(defaultGatherFn, opts);
assert.fail('should have thrown');
} catch (err) {
assert.ok(/Cannot change settings/.test(err.message), 'should have prevented run');
Expand All @@ -120,7 +126,7 @@ describe('Runner', () => {
const settings = {auditMode: artifactsPath, emulatedFormFactor: 'desktop'};
const opts = {url: 'https://differenturl.com', config: generateConfig(settings), driverMock};
try {
await Runner.run(null, opts);
await Runner.run(defaultGatherFn, opts);
assert.fail('should have thrown');
} catch (err) {
assert.ok(/different URL/.test(err.message), 'should have prevented run');
Expand All @@ -145,7 +151,7 @@ describe('Runner', () => {
it('-GA is a normal run but it saves artifacts and LHR to disk', () => {
const settings = {auditMode: artifactsPath, gatherMode: artifactsPath};
const opts = {url, config: generateConfig(settings), driverMock};
return Runner.run(null, opts).then(_ => {
return Runner.run(defaultGatherFn, opts).then(_ => {
expect(loadArtifactsSpy).not.toHaveBeenCalled();
expect(gatherRunnerRunSpy).toHaveBeenCalled();
expect(saveArtifactsSpy).toHaveBeenCalled();
Expand All @@ -156,7 +162,7 @@ describe('Runner', () => {

it('non -G/-A run doesn\'t save artifacts to disk', () => {
const opts = {url, config: generateConfig(), driverMock};
return Runner.run(null, opts).then(_ => {
return Runner.run(defaultGatherFn, opts).then(_ => {
expect(loadArtifactsSpy).not.toHaveBeenCalled();
expect(gatherRunnerRunSpy).toHaveBeenCalled();
expect(saveArtifactsSpy).not.toHaveBeenCalled();
Expand All @@ -182,7 +188,7 @@ describe('Runner', () => {
settings: {gatherMode: artifactsPath},
passes: [{gatherers: [WarningAndErrorGatherer]}],
});
await Runner.run(null, {url, config: gatherConfig, driverMock});
await Runner.run(defaultGatherFn, {url, config: gatherConfig, driverMock});

// Artifacts are still localizable.
const artifacts = assetSaver.loadArtifacts(resolvedPath);
Expand Down Expand Up @@ -211,7 +217,7 @@ describe('Runner', () => {
settings: {auditMode: artifactsPath},
audits: [{implementation: DummyAudit}],
});
const {lhr} = await Runner.run(null, {url, config: auditConfig});
const {lhr} = await Runner.run(defaultGatherFn, {url, config: auditConfig});

// Messages are now localized and formatted.
expect(lhr.runWarnings[0]).toBe('Potential savings of 2 KiB');
Expand All @@ -234,7 +240,7 @@ describe('Runner', () => {
],
});

return Runner.run(null, {url, config, driverMock}).then(_ => {
return Runner.run(defaultGatherFn, {url, config, driverMock}).then(_ => {
expect(gatherRunnerRunSpy).toHaveBeenCalled();
assert.ok(typeof config.passes[0].gatherers[0] === 'object');
});
Expand All @@ -249,7 +255,7 @@ describe('Runner', () => {
],
});

return Runner.run(null, {url, config, driverMock})
return Runner.run(defaultGatherFn, {url, config, driverMock})
.then(_ => {
assert.ok(false);
}, err => {
Expand Down Expand Up @@ -560,7 +566,7 @@ describe('Runner', () => {
}],
});

return Runner.run(null, {url, config, driverMock})
return Runner.run(defaultGatherFn, {url, config, driverMock})
.then(_ => {
assert.ok(false);
}, err => {
Expand All @@ -579,7 +585,7 @@ describe('Runner', () => {
],
});

return Runner.run(null, {url, config, driverMock}).then(results => {
return Runner.run(defaultGatherFn, {url, config, driverMock}).then(results => {
assert.ok(results.lhr.lighthouseVersion);
assert.ok(results.lhr.fetchTime);
assert.equal(results.lhr.requestedUrl, url);
Expand Down Expand Up @@ -609,7 +615,7 @@ describe('Runner', () => {
},
});

return Runner.run(null, {url, config, driverMock}).then(results => {
return Runner.run(defaultGatherFn, {url, config, driverMock}).then(results => {
expect(gatherRunnerRunSpy).toHaveBeenCalled();
assert.ok(results.lhr.lighthouseVersion);
assert.ok(results.lhr.fetchTime);
Expand Down Expand Up @@ -681,7 +687,7 @@ describe('Runner', () => {
],
});

return Runner.run(null, {url, config, driverMock}).then(results => {
return Runner.run(defaultGatherFn, {url, config, driverMock}).then(results => {
// User-specified artifact.
assert.ok(results.artifacts.ViewportDimensions);

Expand All @@ -700,7 +706,7 @@ describe('Runner', () => {
audits: [],
});

return Runner.run(null, {config, driverMock}).then(results => {
return Runner.run(defaultGatherFn, {config, driverMock}).then(results => {
assert.deepStrictEqual(results.lhr.runWarnings, [
'I\'m a warning!',
'Also a warning',
Expand Down Expand Up @@ -731,7 +737,7 @@ describe('Runner', () => {
],
});

return Runner.run(null, {config, driverMock}).then(results => {
return Runner.run(defaultGatherFn, {config, driverMock}).then(results => {
assert.deepStrictEqual(results.lhr.runWarnings, [warningString]);
});
});
Expand Down Expand Up @@ -772,7 +778,7 @@ describe('Runner', () => {

it('includes a top-level runtimeError when a gatherer throws one', async () => {
const config = new Config(configJson);
const {lhr} = await Runner.run(null, {url: 'https://example.com/', config, driverMock});
const {lhr} = await Runner.run(defaultGatherFn, {url: 'https://example.com/', config, driverMock});

// Audit error included the runtimeError
expect(lhr.audits['test-audit'].scoreDisplayMode).toEqual('error');
Expand Down Expand Up @@ -801,7 +807,7 @@ describe('Runner', () => {
});

const config = new Config(configJson);
const {lhr} = await Runner.run(null, {url, config, driverMock: errorDriverMock});
const {lhr} = await Runner.run(defaultGatherFn, {url, config, driverMock: errorDriverMock});

// Audit error still includes the gatherer runtimeError.
expect(lhr.audits['test-audit'].scoreDisplayMode).toEqual('error');
Expand All @@ -825,7 +831,7 @@ describe('Runner', () => {
};

try {
await Runner.run(null, {url: 'https://example.com/', driverMock: erroringDriver, config: new Config()});
await Runner.run(defaultGatherFn, {url: 'https://example.com/', driverMock: erroringDriver, config: new Config()});
assert.fail('should have thrown');
} catch (err) {
assert.equal(err.code, LHError.errors.PROTOCOL_TIMEOUT.code);
Expand All @@ -844,7 +850,7 @@ describe('Runner', () => {
},
});

const results = await Runner.run(null, {url, config, driverMock});
const results = await Runner.run(defaultGatherFn, {url, config, driverMock});
assert.ok(Array.isArray(results.report) && results.report.length === 2,
'did not return multiple reports');
assert.ok(JSON.parse(results.report[0]), 'did not return json output');
Expand Down

0 comments on commit 6c6214d

Please sign in to comment.