Return artifacts from runner and move assets/artifacts saving to cli #1163

weiwei-lin · 2016-12-14T23:55:48Z

Moving assets/artifacts saving to cli will allow us to host trace files in the future #616 (with #1130). Because the server needs to know where those trace files are but it's unnatural to include those file paths in the results returned by runner.
This will also make it possible to compare timeline data in Project Performance Experiment #1143.

Taking over from #1115

brendankenny

definitely liking this in the CLI instead of runner

brendankenny · 2016-12-15T00:00:38Z

lighthouse-cli/bin.ts

@@ -67,6 +67,7 @@ const cliFlags = yargs
    'mobile',
    'save-assets',
    'save-artifacts',
+    'exclude-artifacts',


we should try to pick a flag name with a default false value (so that undefined or anything falsy will be coerced to it). Maybe just --include-artifacts?

I guess the question is, what does this flag observably do? At this point it won't affect the html report, so users need to choose json to see the artifacts in the output (and they could use --save-artifacts for that).

Maybe it makes sense not to have a CLI flag, and for now always clear results.artifacts in bin.ts. Then future PRs can add flags for their own behavior (like #1130 adding --view), which can control if the artifacts property is cleared or not.

That should help with later orthogonality of flags, so we don't have to figure out things like, what if the user sets --view and --include-artifacts=false

brendankenny · 2016-12-15T00:22:51Z

lighthouse-cli/bin.ts

  // Process URLs once at a time
  const address = addresses.shift();
  if (!address) {
    return;
  }

  return lighthouse(address, flags, config)
+    .then((results: Results) => {
+      if (flags.saveArtifacts) {
+        return assetSaver.saveArtifacts(results.artifacts).then((_: Object) => results);


assetSaver.saveArtifacts is sync, so won't return a promise (feel free to alter here or the function itself :)

brendankenny · 2016-12-15T00:25:15Z

lighthouse-cli/bin.ts

+    })
+    .then((results: Results) => {
+      if (flags.saveAssets) {
+        return assetSaver.saveAssets({url: results.url}, results.artifacts).then((_:Object) => results);


assetSaver.saveAssets returns Promise<undefined>, so easier to do .then(() => results)

brendankenny · 2016-12-15T00:30:09Z

lighthouse-core/runner.js

      // Run each audit sequentially, the auditResults array has all our fine work
      const auditResults = [];
      run = run.then(artifacts => config.audits.reduce((chain, audit) => {
        return chain.then(_ => {
          return Runner._runAudit(audit, artifacts);
        }).then(ret => auditResults.push(ret));
-      }, Promise.resolve()).then(_ => auditResults));
+      }, Promise.resolve()).then(_ => {


can you re-indent this block (moving config.audits.reduce((chain, audit) => { from above to its own line) to make it clearer that artifacts is still in scope?

I updated this to use "for .. of loop" and make those audit runs become a "standard run" (i.e. resolves to artifacts). Hopefully, this will increase readability.

brendankenny · 2016-12-15T00:33:56Z

lighthouse-cli/bin.ts

+    })
+    .then((results: Results) => {
+      if (flags.excludeArtifacts) {
+        delete results.artifacts;


doing results.artifacts = undefined; here instead should be ok. Using delete tempts slow objects and we're about to do a bunch of processing on the results, and e.g. JSON.stringify drops undefined properties anyway

brendankenny · 2016-12-15T00:36:12Z

lighthouse-core/runner.js

    } else if (config.auditResults) {
      // If there are existing audit results, surface those here.
-      run = run.then(_ => config.auditResults);
+      run = run.then(_ => {
+        return {artifacts: config.artifacts, auditResults: config.auditResults};


do you still want the default {} if there's no config.artifacts or is undefined ok in that case?

brendankenny · 2016-12-15T00:36:55Z

lighthouse-core/runner.js

@@ -119,7 +108,9 @@ class Runner {

    // Format and aggregate results before returning.
    run = run
-      .then(auditResults => {
+      .then(auditData => {


doesn't really matter, but maybe runResults?

brendankenny · 2016-12-15T00:37:40Z

lighthouse-core/runner.js

@@ -119,7 +108,9 @@ class Runner {

    // Format and aggregate results before returning.
    run = run
-      .then(auditResults => {
+      .then(auditData => {
+        const artifacts = auditData.artifacts;


nit: just inline these below rather than declaring separate variables

brendankenny · 2016-12-15T01:45:32Z

should also add some simple tests to runner-test.js that artifacts produced by gatherers or passed in on config.artifacts get copied through runner. There are a few tests in there already demonstrating use of the driver mock, passing in artifacts in a config, etc.

weiwei-lin · 2016-12-15T04:00:38Z

Fixed some bugs and coding style. Still working on test cases.

weiwei-lin · 2016-12-15T23:24:03Z

Added test cases to ensure artifacts is returned when 1. auditResults is generated in Runner.run, 2. existing auditResults is given to Runner.run

brendankenny

more review :)

(sorry about the rebase)

brendankenny · 2016-12-16T00:21:07Z

lighthouse-cli/bin.ts

  // Process URLs once at a time
  const address = addresses.shift();
  if (!address) {
    return;
  }

  return lighthouse(address, flags, config)
+    .then((results: Results) => {
+      if (flags.saveArtifacts) {


maybe combine these two saveArtifacts and saveAssets handlers into one then block? (and maybe even pull into a separate function? not sure about that one, though)

brendankenny · 2016-12-16T00:22:17Z

lighthouse-cli/bin.ts

+      return results;
+    })
+    .then((results: Results) => {
+      results.artifacts = undefined;


should add a comment explaining why we're doing this. Maybe something simple like // don't include artifacts in printed output by default. or something?

brendankenny · 2016-12-16T01:55:39Z

lighthouse-cli/bin.ts

  // Process URLs once at a time
  const address = addresses.shift();
  if (!address) {
    return;
  }

  return lighthouse(address, flags, config)
+    .then((results: Results) => {


sorry, will have to rebase after #1141

brendankenny · 2016-12-16T01:56:55Z

lighthouse-core/test/runner-test.js

+    });
+
+    return Runner.run({}, {url, config}).then(results => {
+      assert.ok(results.artifacts !== undefined);


maybe check that results.artifacts.HTTPS is still on there? Do we want to be stricter with this check in any other way?

brendankenny · 2016-12-16T01:57:45Z

lighthouse-core/test/runner-test.js

@@ -295,4 +295,56 @@ describe('Runner', () => {
      assert.strictEqual(AuditClass.meta.name, auditExpectedName);
    });
  });
+
+  it('returns artifacts when audits are given', () => {


maybe something like 'results include artifacts when audits are given'?

brendankenny · 2016-12-16T01:58:24Z

lighthouse-core/test/runner-test.js

+    });
+  });
+
+  it('returns artifacts when auditResult is given', () => {


same thing, something like 'results include artifacts when auditResult is given'?

brendankenny · 2016-12-16T02:13:45Z

lighthouse-core/test/runner-test.js

+    return Runner.run(null, {url, config, driverMock}).then(results => {
+      assert.ok(results.artifacts !== undefined);
+    });
+  });


should probably have one more with passes and audits in the config file. driverMock will provide a trace, so could do a pass that records a trace and then an audit that purely requires a trace (maybe user-timings?). Something like

const config = new Config({ passes: [{ recordTrace: true, gatherers: [] }], audits: [ 'user-timings' ] });

and then the result should have a results.artifacts.traces array with length > 0

That's kind of venturing into integration testing (relying on gather-runner internally, for one), but good to catch this case because most lighthouse runs are with configs of the passes/audits format. What do you think?

brendankenny · 2016-12-16T02:15:49Z

lighthouse-core/runner.js

    } else if (config.auditResults) {
      // If there are existing audit results, surface those here.
-      run = run.then(_ => config.auditResults);
+      // Instantiate and return artifacts for consistency.
+      // Although we can move this upwards to reduce code repetition, this process might be quite


the // Although... comment lines probably aren't necessary here (though good for the code reviewer :).

I do think you're right we want to have the same object shape for artifacts regardless of path through here, so adding the computed artifacts is the right move

weiwei-lin · 2016-12-16T06:04:20Z

lighthouse-cli/bin.ts

-          return Printer.write(results, 'html', filePath).then(() => {
-            opn(`http://localhost:${port}/reports/${filename}`);
-          });
+        const filename = `${assetSaver.getFilenamePrefix({url})}.report.html`;


Although this line can be combined with line 284, we need a variable outside of the 'then-chain' to do that. Also, since we no longer use symlinks, the filenames do not need to be the same. And in the future, we may save JSON instead of html.

weiwei-lin · 2016-12-16T06:06:54Z

lighthouse-core/test/runner-test.js

+          continue;
+        }
+        assert.ok(results.artifacts.hasOwnProperty(method));
+      }


assetSaver needs computedArtifacts to save screenshots.

weiwei-lin · 2016-12-16T06:40:33Z

lighthouse-cli/bin.ts

      if (flags.output === Printer.OutputMode[Printer.OutputMode.pretty]) {
-        Printer.write(results, 'html', filename);
+        const filename = `${assetSaver.getFilenamePrefix({url})}.report.html`;
+        return Printer.write(results, 'html', filename);
      }


Fixed a concurrency error here. Previously, promise will resolve to next 'then' before Printer finishes writing. No sure if that matters.

weiwei-lin · 2016-12-21T04:14:03Z

Thanks for the advice. Ready for review again! @brendankenny PTAL.

paulirish

Yup this works well for me.
Only one nit, otherwise I think this is good to go.

Will merge after the nit is addressed. :)

paulirish · 2017-01-04T03:57:33Z

lighthouse-cli/bin.ts

+      return results;
+    })
+    .then((results: Results) => {
+      // remove artifacts so users won't get unnecessary artifacts in the report.


let's just do this last bit in the previous .then(), since its all related

Edit: I will come back to this after I rebased this branch
Edit: Fixed. Thanks for the advice

Runner now returns artifacts.\nAdded --exclude-aritifacts, default to be true, will delete artifacts after runner returns it\n

… assigning an empty object to it

Now artifacts are always deleted/undeined before passing to Printer.\nNo longer use .then() after assetSaver.saveArtifacts since it's sync.\nChanged audit run in Runner.run to use for .. of loop instead of reduce to increase readability.

also made runner always return instantiated artifacts for consistency

…rtServer

weiwei-lin · 2017-01-04T06:42:19Z

Yup this works well for me.
Only one nit, otherwise I think this is good to go.

Will merge after the nit is addressed. :)

Ready for review again. PTAL @paulirish.

paulirish · 2017-01-04T06:44:11Z

lighthouse-cli/bin.ts

                       config: Object): Promise<undefined> {

  let chromeLauncher: ChromeLauncher;
  return initPort(flags)
    .then(() => getDebuggableChrome(flags))
    .then(chrome => chromeLauncher = chrome)
    .then(() => lighthouse(url, flags, config))
+    .then((results: Results) => {
+      // delte artifacts from result so reports won't include artifacts.


delte => delete

paulirish · 2017-01-04T06:44:38Z

lighthouse-cli/bin.ts

+      return results;
+    })
+    .then((results: Results) => {
+      return results;


i believe you can nuke this entire .then() now..

Oops. Sorry, wasn't careful enough when checking after rebase. Didn't notice that. Already fixed.

brendankenny · 2017-01-04T06:58:17Z

lighthouse-cli/bin.ts

                       config: Object): Promise<undefined> {

  let chromeLauncher: ChromeLauncher;
  return initPort(flags)
    .then(() => getDebuggableChrome(flags))
    .then(chrome => chromeLauncher = chrome)
    .then(() => lighthouse(url, flags, config))
+    .then((results: Results) => {
+      // delte artifacts from result so reports won't include artifacts.
+      const artifacts = results.artifacts;


brendankenny

some small last things. I like this refactor

brendankenny · 2017-01-04T07:10:36Z

lighthouse-core/test/runner-test.js

+    });
+
+    return Runner.run({}, {url, config}).then(results => {
+      if (!results.artifacts instanceof Object) {


not sure if these instanceof checks are really necessary. If not an object, it will throw on the property accesses below anyway

Thanks for the advice. Fixed.

brendankenny · 2017-01-04T07:11:55Z

lighthouse-core/test/runner-test.js

+      assert.strictEqual(results.artifacts.HTTPS.value, true);
+
+      for (const method in computedArtifacts) {
+        if (!computedArtifacts.hasOwnProperty(method)) {


could you just do a loop over Object.keys(computedArtifacts) here to avoid hasOwnProperty, etc?

Thanks for the advice. Fixed.

brendankenny · 2017-01-04T07:12:22Z

lighthouse-core/test/runner-test.js

+    });
+
+    return Runner.run(null, {url, config, driverMock}).then(results => {
+      if (!results.artifacts instanceof Object) {


same as above

brendankenny · 2017-01-04T07:12:28Z

lighthouse-core/test/runner-test.js

+        assert.ok(false);
+        return;
+      }
+


no results.artifacts.HTTPS here?

No sure what you mean here. Do you mean I need to check whether results.artifacts.HTTPS exists?
If that's the case, do we need to check results.artifacts as well? And also results.audits and so on in other test cases?

I mean in the other two tests you check assert.strictEqual(results.artifacts.HTTPS.value, true);. I'm not sure what the value of results.artifacts.HTTPS.value will be here, but I believe runner will attempt to run the HTTPS gatherer with the mock driver supplied, so it seems like it's worth keeping the check in there since this is checking the artifacts coming through

Got it. And added assert.ok(results.artifacts.HTTPS) to check whether non-computed artifacts attributes are returned.

brendankenny · 2017-01-04T07:12:34Z

lighthouse-core/test/runner-test.js

+        return;
+      }
+
+      for (const method in computedArtifacts) {


same as above

brendankenny · 2017-01-04T07:12:51Z

lighthouse-core/test/runner-test.js

+    });
+
+    return Runner.run(null, {url, config, driverMock}).then(results => {
+      if (!results.artifacts instanceof Object) {


and same here and the for..in loop below too :)

brendankenny · 2017-01-04T07:13:36Z

lighthouse-cli/bin.ts

+    .then((results: Results) => {
+      // delete artifacts from result so reports won't include artifacts.
+      const artifacts = results.artifacts;
+      results.artifacts = undefined;


style nit: maybe add a line break after this line to separate the two actions?

Thanks. Fixed.

brendankenny · 2017-01-04T07:58:42Z

lighthouse-core/test/runner-test.js

+    return Runner.run({}, {url, config}).then(results => {
+      assert.strictEqual(results.artifacts.HTTPS.value, true);
+
+      for (const method in Object.keys(computedArtifacts)) {


I think you want for...of here

What a dumb mistake : (
Fixed.

brendankenny

Really like how this change turned out!

brendankenny · 2017-01-04T19:03:56Z

thanks for all your patience :)

…oogleChrome#1163)

brendankenny requested changes Dec 15, 2016

View reviewed changes

weiwei-lin force-pushed the flag-provide-artifacts branch from 88fe221 to 8b47bc4 Compare December 15, 2016 03:58

weiwei-lin closed this Dec 15, 2016

weiwei-lin deleted the flag-provide-artifacts branch December 15, 2016 10:45

weiwei-lin restored the flag-provide-artifacts branch December 15, 2016 10:45

weiwei-lin reopened this Dec 15, 2016

brendankenny requested changes Dec 16, 2016

View reviewed changes

weiwei-lin force-pushed the flag-provide-artifacts branch from 4890cac to 4489d1b Compare December 16, 2016 05:53

weiwei-lin commented Dec 16, 2016

View reviewed changes

weiwei-lin force-pushed the flag-provide-artifacts branch 2 times, most recently from db32b77 to 23a5483 Compare December 21, 2016 04:08

paulirish reviewed Jan 4, 2017

View reviewed changes

Weiwei Lin and others added 13 commits January 4, 2017 17:11

Add --exclude-artifacts

95abf67

Runner now returns artifacts.\nAdded --exclude-aritifacts, default to be true, will delete artifacts after runner returns it\n

Moved saveAtrifacts and saveAssets to cli

374270e

delete results.artifacts when --exclude-artifacts presents instead of…

632cb46

… assigning an empty object to it

no longer require asset-saver in runner.js

c3fe1e2

fixed coding style

41851d1

added test cases for testing artifacts are returned by runner

1442bb4

also made runner always return instantiated artifacts for consistency

fixed passing non-boolean value to assert.ok

42ef7ac

fixed typo

d0a7451

removed unwanted console.log

111bc67

fixed coding style and a concurrency error related to pretty print

e77befe

fixed a bug where port number is not passed to performanceXserver.sta…

fd1503e

…rtServer

no longer start performanceX server when --view does not present

55764fe

weiwei-lin force-pushed the flag-provide-artifacts branch from 23a5483 to 55764fe Compare January 4, 2017 06:35

paulirish reviewed Jan 4, 2017

View reviewed changes

removed an useless .then block

2359fda

brendankenny reviewed Jan 4, 2017

View reviewed changes

fixed a typo

9c7d23c

brendankenny requested changes Jan 4, 2017

View reviewed changes

simplified unit test

02970a1

brendankenny reviewed Jan 4, 2017

View reviewed changes

fixed a bug where for .. of should be used instead of for .. in

e66ca26

brendankenny approved these changes Jan 4, 2017

View reviewed changes

brendankenny merged commit 30f1344 into GoogleChrome:master Jan 4, 2017

paulirish mentioned this pull request Jan 4, 2017

Exclude artifacts from result handed to extension/devtools. #1400

Merged

weiwei-lin deleted the flag-provide-artifacts branch January 4, 2017 22:38

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017

Return artifacts from runner and move assets/artifacts saving to cli (G…

4796fdd

…oogleChrome#1163)

Return artifacts from runner and move assets/artifacts saving to cli #1163

Return artifacts from runner and move assets/artifacts saving to cli #1163

Conversation

weiwei-lin commented Dec 14, 2016 • edited by brendankenny Loading

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

brendankenny Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Dec 15, 2016

weiwei-lin commented Dec 15, 2016

weiwei-lin commented Dec 15, 2016

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiwei-lin commented Dec 21, 2016

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiwei-lin Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

weiwei-lin commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Jan 4, 2017

weiwei-lin commented Dec 14, 2016 •

edited by brendankenny

Loading

brendankenny Dec 15, 2016 •

edited

Loading

brendankenny Dec 15, 2016 •

edited

Loading

brendankenny Dec 15, 2016 •

edited

Loading

brendankenny Dec 15, 2016 •

edited

Loading

brendankenny Dec 16, 2016 •

edited

Loading

weiwei-lin Jan 4, 2017 •

edited

Loading