Skip to content

cli(lifecycle): introduce report-only runs#4210

Closed
paulirish wants to merge 4 commits into
masterfrom
reportmode
Closed

cli(lifecycle): introduce report-only runs#4210
paulirish wants to merge 4 commits into
masterfrom
reportmode

Conversation

@paulirish

@paulirish paulirish commented Jan 9, 2018

Copy link
Copy Markdown
Member
  1. If you run lighthouse with -GA or -A either one of the following, it will also save the LHR results json to disk within the ./latest-run/ folder, alongside the artifacts.
  2. When running lighthouse with -R, it'll read that latest-run/lhr.json file and then generate a report and save that to disk. (It'll abide your output options or our defaults).

Also

  • it turned out Printer.write() was more complicated than necessary.
  • runner.js doesn't need to know about the 'latest-run' path. Moved that to be private to asset-saver.
  • Unlike -G and -A, the -R flag is CLI only, and lighthouse-core is unaware of it completely. there's something nice about that.
  • todo: readme update, tests

GAR!

@patrickhulce patrickhulce left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

sweet, hurray no more report hacking script 🎉 :D

Comment thread lighthouse-cli/printer.js
return new Promise((resolve, reject) => {
const outputPath = checkOutputPath(path);
const output = createOutput(results, mode);
const outputPath = checkOutputPath(path);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this looks just like cleanup no functional change right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yup. though i remember some bizarre rejection problems were, i think, why we complicated this method. we'll see.

Comment thread lighthouse-cli/run.js
promise = promise.then(_ => assetSaver.saveAssets(artifacts, results.audits, resolvedPath));
} else if (flags.gatherMode || flags.auditMode) {
const latestPath = path.join(cwd, 'latest-run', 'lhr.json');
promise = promise.then(_ => Printer.write(results, Printer.OutputMode.json, latestPath));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

will we be able to do this for just gatherMode? seems like we can only do this with auditMode or gatherMode === auditMode

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

actually yah this should instead be

else if (flags.auditMode) {

Comment thread lighthouse-cli/run.js
* @return {!Promise<!LH.Results|void>}
*/
function runLighthouse(url, flags, config) {
if (flags.reportMode) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this means you can't really do -GAR for the full thing and existence of -R forces report-only mode no matter what, is that what we want? I guess auditMode continues on to do what reportMode would do anyhow

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

hmm that's true. let's talk about what's ideal here.

@brendankenny

Copy link
Copy Markdown
Contributor

Definitely need to finish the GAR, but this patch is now super out of sync. Closing for now and with the hope that the next approach can be even simpler with our Runner revisions since Jan 2018.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants