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(lifecycle): allow gathering & auditing to run separately #3743

Merged
merged 20 commits into from
Jan 5, 2018
Merged
Show file tree
Hide file tree
Changes from 16 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
13 changes: 9 additions & 4 deletions lighthouse-cli/cli-flags.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ function getFlags(manualArgv) {

.group(
[
'save-assets', 'save-artifacts', 'list-all-audits', 'list-trace-categories',
'save-assets', 'list-all-audits', 'list-trace-categories',
'additional-trace-categories', 'config-path', 'chrome-flags', 'perf', 'port',
'hostname', 'max-wait-for-load', 'enable-error-reporting',
'hostname', 'max-wait-for-load', 'enable-error-reporting', 'gather-mode', 'audit-mode',
],
'Configuration:')
.describe({
Expand All @@ -66,8 +66,10 @@ function getFlags(manualArgv) {
'disable-device-emulation': 'Disable Nexus 5X emulation',
'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',
'save-assets': 'Save the trace contents & screenshots to disk',
'save-artifacts': 'Save all gathered artifacts 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',
'additional-trace-categories':
Expand All @@ -85,6 +87,8 @@ function getFlags(manualArgv) {
'max-wait-for-load':
'The timeout (in milliseconds) to wait before the page is considered done loading and the run should continue. WARNING: Very high values can lead to large traces and instability',
})
// set aliases
.alias({'gather-mode': 'G', 'audit-mode': 'A'})

.group(['output', 'output-path', 'view'], 'Output:')
.describe({
Expand All @@ -100,8 +104,9 @@ function getFlags(manualArgv) {
// boolean values
.boolean([
'disable-storage-reset', 'disable-device-emulation', 'disable-cpu-throttling',
'disable-network-throttling', 'save-assets', 'save-artifacts', 'list-all-audits',
'disable-network-throttling', 'save-assets', 'list-all-audits',
'list-trace-categories', 'perf', 'view', 'verbose', 'quiet', 'help',
'gather-mode', 'audit-mode',
])
.choices('output', printer.getValidOutputOptions())
// force as an array
Expand Down
65 changes: 38 additions & 27 deletions lighthouse-cli/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,19 +108,19 @@ function handleError(err) {
* @return {!Promise<void>}
*/
function saveResults(results, artifacts, flags) {
let promise = Promise.resolve(results);
const cwd = process.cwd();
let promise = Promise.resolve();

const shouldSaveResults = flags.auditMode || (flags.gatherMode === flags.auditMode);
if (!shouldSaveResults) return promise;

// Use the output path as the prefix for all generated files.
// If no output path is set, generate a file prefix using the URL and date.
const configuredPath = !flags.outputPath || flags.outputPath === 'stdout' ?
getFilenamePrefix(results) :
flags.outputPath.replace(/\.\w{2,4}$/, '');
const resolvedPath = path.resolve(cwd, configuredPath);

if (flags.saveArtifacts) {
assetSaver.saveArtifacts(artifacts, resolvedPath);
}

if (flags.saveAssets) {
promise = promise.then(_ => assetSaver.saveAssets(artifacts, results.audits, resolvedPath));
}
Expand Down Expand Up @@ -162,32 +162,43 @@ function saveResults(results, artifacts, flags) {
function runLighthouse(url, flags, config) {
/** @type {!LH.LaunchedChrome} */
let launchedChrome;
const shouldGather = flags.gatherMode || flags.gatherMode === flags.auditMode;
let chromeP = Promise.resolve();

if (shouldGather) {
chromeP = chromeP.then(_ =>
getDebuggableChrome(flags).then(launchedChromeInstance => {
launchedChrome = launchedChromeInstance;
flags.port = launchedChrome.port;
})
);
}

return getDebuggableChrome(flags)
.then(launchedChromeInstance => {
launchedChrome = launchedChromeInstance;
flags.port = launchedChrome.port;
return lighthouse(url, flags, config);
})
.then(results => {
const resultsP = chromeP.then(_ => {
return lighthouse(url, flags, config).then(results => {
return potentiallyKillChrome().then(_ => results);
}).then(results => {
const artifacts = results.artifacts;
delete results.artifacts;

return saveResults(results, artifacts, flags)
.then(_ => launchedChrome.kill())
.then(_ => results);
})
.catch(err => {
return Promise.resolve()
.then(_ => {
if (launchedChrome !== undefined) {
return launchedChrome.kill()
// TODO: keeps tsc happy (erases return type) but is useless.
.then(_ => {});
}
})
.then(_ => handleError(err));
return saveResults(results, artifacts, flags).then(_ => results);
});
});

return resultsP.catch(err => {
return Promise.resolve()
.then(_ => potentiallyKillChrome())
.then(_ => handleError(err));
});

/**
* @return {!Promise<{}>}
*/
function potentiallyKillChrome() {
if (launchedChrome !== undefined) {
return launchedChrome.kill();
}
return Promise.resolve({});
}
}

module.exports = {
Expand Down
17 changes: 3 additions & 14 deletions lighthouse-core/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,12 @@ function validatePasses(passes, audits, rootPath) {
});
}

function validateCategories(categories, audits, auditResults, groups) {
function validateCategories(categories, audits, groups) {
if (!categories) {
return;
}

const auditIds = audits ?
audits.map(audit => audit.meta.name) :
auditResults.map(audit => audit.name);
const auditIds = audits.map(audit => audit.meta.name);
Object.keys(categories).forEach(categoryId => {
categories[categoryId].audits.forEach((audit, index) => {
if (!audit.id) {
Expand Down Expand Up @@ -319,10 +317,6 @@ class Config {
this._configDir = configPath ? path.dirname(configPath) : undefined;

this._passes = configJSON.passes || null;
this._auditResults = configJSON.auditResults || null;
if (this._auditResults && !Array.isArray(this._auditResults)) {
throw new Error('config.auditResults must be an array');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is anyone using this should we wait for a major version? I'm fine nuking, but seems like it could still be supported

Copy link
Member

Choose a reason for hiding this comment

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

Ideally it would stick around, at least until 3.0. auditResults can skip everything and go right to the scoring (basically -R)

Copy link
Member

Choose a reason for hiding this comment

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

(it's also helpful for tests where you just want to test the -R part)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to just kill auditResults. Turns out we dont really have tests using it (except one).

auditResults is this weird inbetween value that's only different from the LHR because of these few lines: https://github.com/GoogleChrome/lighthouse/blob/gar/lighthouse-core/runner.js#L128-L155

if we REALLY want to support -R then we'd definitely not put auditResults on the config instance because it's bizarre. (obv that would break backcompat (for those mystery users)). if we want to do this i'd prefer to do it in a followup

}

this._audits = Config.requireAudits(configJSON.audits, this._configDir);
this._artifacts = expandArtifacts(configJSON.artifacts);
Expand All @@ -331,7 +325,7 @@ class Config {

// validatePasses must follow after audits are required
validatePasses(configJSON.passes, this._audits, this._configDir);
validateCategories(configJSON.categories, this._audits, this._auditResults, this._groups);
validateCategories(configJSON.categories, this._audits, this._groups);
}

/**
Expand Down Expand Up @@ -599,11 +593,6 @@ class Config {
return this._audits;
}

/** @type {Array<!AuditResult>} */
get auditResults() {
return this._auditResults;
}

/** @type {Array<!Artifacts>} */
get artifacts() {
return this._artifacts;
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,6 @@ class GatherRunner {
const tracingData = {
traces: {},
devtoolsLogs: {},
networkRecords: {},
};

if (typeof options.url !== 'string' || options.url.length === 0) {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ function lighthouse(url, flags = {}, configJSON) {

// kick off a lighthouse run
return Runner.run(connection, {url, flags, config})
.then(lighthouseResults => {
.then((lighthouseResults = {}) => {
Copy link
Member

Choose a reason for hiding this comment

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

what execution path leads to this being undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

-G does. no lighthouse results because we saved artifacts and are resolving early

i could pass an object back there, but it seems slightly more awkward. open to ideas.

// Annotate with time to run lighthouse.
const endTime = Date.now();
lighthouseResults.timing = lighthouseResults.timing || {};
Expand Down
101 changes: 90 additions & 11 deletions lighthouse-core/lib/asset-saver.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@
'use strict';

const fs = require('fs');
const path = require('path');
const log = require('lighthouse-logger');
const stream = require('stream');
const stringifySafe = require('json-stringify-safe');
const Metrics = require('./traces/pwmetrics-events');
const TraceParser = require('./traces/trace-parser');
const rimraf = require('rimraf');
const mkdirp = require('mkdirp');

/**
* Generate basic HTML page of screenshot filmstrip
Expand Down Expand Up @@ -55,19 +58,94 @@ img {
`;
}

const artifactsFilename = 'artifacts.json';
const traceSuffix = '.trace.json';
const devtoolsLogSuffix = '.devtoolslog.json';

/**
* Load artifacts object from files located within basePath
* Also save the traces to their own files
* @param {string} basePath
* @return {!Promise<!Artifacts>}
*/
// Set to ignore because testing it would imply testing fs, which isn't strictly necessary.
/* istanbul ignore next */
function loadArtifacts(basePath) {
log.log('Reading artifacts from disk:', basePath);

if (!fs.existsSync(basePath)) return Promise.reject(new Error('No saved artifacts found'));

// load artifacts.json
const filenames = fs.readdirSync(basePath);
const artifacts = JSON.parse(fs.readFileSync(path.join(basePath, artifactsFilename), 'utf8'));

// load devtoolsLogs
artifacts.devtoolsLogs = {};
filenames.filter(f => f.endsWith(devtoolsLogSuffix)).map(filename => {
const passName = filename.replace(devtoolsLogSuffix, '');
const devtoolsLog = JSON.parse(fs.readFileSync(path.join(basePath, filename), 'utf8'));
artifacts.devtoolsLogs[passName] = devtoolsLog;
});

// load traces
artifacts.traces = {};
const promises = filenames.filter(f => f.endsWith(traceSuffix)).map(filename => {
return new Promise(resolve => {
const passName = filename.replace(traceSuffix, '');
const readStream = fs.createReadStream(path.join(basePath, filename), {
encoding: 'utf-8',
highWaterMark: 4 * 1024 * 1024, // TODO benchmark to find the best buffer size here
});
const parser = new TraceParser();
readStream.on('data', chunk => parser.parseChunk(chunk));
readStream.on('end', _ => {
artifacts.traces[passName] = parser.getTrace();
resolve();
});
});
});
return Promise.all(promises).then(_ => artifacts);
}

/**
* Save entire artifacts object to a single stringified file located at
* pathWithBasename + .artifacts.log
* Save artifacts object mostly to single file located at basePath/artifacts.log.
* Also save the traces & devtoolsLogs to their own files
* @param {!Artifacts} artifacts
* @param {string} pathWithBasename
* @param {string} basePath
*/
// Set to ignore because testing it would imply testing fs, which isn't strictly necessary.
/* istanbul ignore next */
function saveArtifacts(artifacts, pathWithBasename) {
const fullPath = `${pathWithBasename}.artifacts.log`;
// The networkRecords artifacts have circular references
fs.writeFileSync(fullPath, stringifySafe(artifacts));
log.log('artifacts file saved to disk', fullPath);
function saveArtifacts(artifacts, basePath) {
mkdirp.sync(basePath);
rimraf.sync(`${basePath}/*${traceSuffix}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe chrome launcher had a lot of issues on windows when using sync rimraf. Should we move to async?

Copy link
Member Author

Choose a reason for hiding this comment

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

rimraf's bumps since then were around this issue so i am hoping we're good now. https://github.com/isaacs/rimraf/commits/master

rimraf.sync(`${basePath}/${artifactsFilename}`);

// We don't want to mutate the artifacts as provided
artifacts = Object.assign({}, artifacts);

// save traces
const traces = artifacts.traces;
let promise = Promise.all(Object.keys(traces).map(passName => {
return saveTrace(traces[passName], `${basePath}/${passName}${traceSuffix}`);
}));

// save devtools log
const devtoolsLogs = artifacts.devtoolsLogs;
promise = promise.then(_ => {
Object.keys(devtoolsLogs).map(passName => {
const log = JSON.stringify(devtoolsLogs[passName]);
fs.writeFileSync(`${basePath}/${passName}${devtoolsLogSuffix}`, log, 'utf8');
});
delete artifacts.traces;
Copy link
Member

Choose a reason for hiding this comment

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

can't delete traces on artifacts, maybe make a copy of the object?

delete artifacts.devtoolsLogs;
});

// save everything else
promise = promise.then(_ => {
fs.writeFileSync(`${basePath}/${artifactsFilename}`, JSON.stringify(artifacts), 'utf8');
Copy link
Collaborator

Choose a reason for hiding this comment

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

care about prettifying?

a consequence of this I just ran into is that some artifacts are objects like Map/Set that aren't reconstituted. we'll have to move away from that. it might just be me with unminified-javascript right now :)

log.log('Artifacts saved to disk in folder:', basePath);
});
return promise;
}

/**
Expand Down Expand Up @@ -178,7 +256,7 @@ function saveTrace(traceData, traceFilename) {
function saveAssets(artifacts, audits, pathWithBasename) {
return prepareAssets(artifacts, audits).then(assets => {
return Promise.all(assets.map((data, index) => {
const devtoolsLogFilename = `${pathWithBasename}-${index}.devtoolslog.json`;
const devtoolsLogFilename = `${pathWithBasename}-${index}${devtoolsLogSuffix}`;
fs.writeFileSync(devtoolsLogFilename, JSON.stringify(data.devtoolsLog, null, 2));
log.log('saveAssets', 'devtools log saved to disk: ' + devtoolsLogFilename);

Expand All @@ -190,7 +268,7 @@ function saveAssets(artifacts, audits, pathWithBasename) {
fs.writeFileSync(screenshotsJSONFilename, JSON.stringify(data.screenshots, null, 2));
log.log('saveAssets', 'screenshots saved to disk: ' + screenshotsJSONFilename);

const streamTraceFilename = `${pathWithBasename}-${index}.trace.json`;
const streamTraceFilename = `${pathWithBasename}-${index}${traceSuffix}`;
log.log('saveAssets', 'streaming trace file to disk: ' + streamTraceFilename);
return saveTrace(data.traceData, streamTraceFilename).then(_ => {
log.log('saveAssets', 'trace file streamed to disk: ' + streamTraceFilename);
Expand Down Expand Up @@ -221,6 +299,7 @@ function logAssets(artifacts, audits) {

module.exports = {
saveArtifacts,
loadArtifacts,
saveAssets,
prepareAssets,
saveTrace,
Expand Down
Loading