core(lifecycle): allow gathering & auditing to run separately #3743

paulirish · 2017-11-03T19:06:13Z

The mystical -GAR feature. Actually its just -GA for now.

Here's how this works:

lighthouse -G http://example.com
# launches browser, collects data, saves to disk (in `./latest-run/`) and quits

lighthouse -A http://example.com
# skips browser interaction, loads artifacts from disk (in `./latest-run/`), runs audits on them, generates report

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

--gather-mode and --audit-mode are the long versions of -G and -A.
config.auditResults is removed. it was weird.
assetSaver got a new impl of saveArtifacts, and added loadArtifacts
--save-artifacts CLI flag was removed, as it was never really used and makes less sense in -G world.

Todo:

Decide if we want to rename these, as "gather" hasn't been really user-visible before.

Future work:

Add -G=./artifacts-save-path/
Add -A=./artifacts-load-path/

patrickhulce

nice this is awesome!! been soooooo excited for this 🎉 🕺 🕺 🕺 🎉

patrickhulce · 2017-11-03T20:13:25Z

lighthouse-core/config/config.js

@@ -318,10 +316,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');


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

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

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

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

patrickhulce · 2017-11-03T20:14:58Z

lighthouse-core/lib/asset-saver.js

+  artifacts.traces = {};
+
+  const filenames = fs.readdirSync(basePath);
+  const promises = filenames.filter(filename => filename.endsWith('-trace.json')).map(filename => {


should we do both traces and devtoolsLogs this way? It'd be nice to be consistent with how the assets are saved to disk in non-GAR mode but also less useful since there's nothing you can do with just the devtoolslog

nit: we also liked .trace.json/.devtoolslog.json if we want to stick with it here too :)

patrickhulce · 2017-11-03T20:16:40Z

lighthouse-core/lib/asset-saver.js

-  log.log('artifacts file saved to disk', fullPath);
+function saveArtifacts(artifacts, basePath) {
+  assert.notEqual(basePath, '/');
+  rimraf.sync(basePath);


since we actually know about all the files we create, can we just delete those instead of the whole directory?

patrickhulce · 2017-11-03T20:30:36Z

lighthouse-core/runner.js

+    const shouldGatherAndQuit = opts.flags.gatherMode && !opts.flags.auditMode;
+    const shouldOnlyAudit = opts.flags.auditMode && !opts.flags.gatherMode;
+    const shouldDefaultRunButSaveArtifacts = opts.flags.auditMode && opts.flags.gatherMode;
+    const shouldDoTypicalRun = !opts.flags.gatherMode && !opts.flags.auditMode;


can we pick either Typical or Default :)

patrickhulce · 2017-11-03T20:31:54Z

lighthouse-core/runner.js


-        return artifacts;
+    if (shouldLoadArtifactsFromDisk) {
+      config.removePasses();


seems like this should be unnecessary given we've exploded the various should* branches into variables above?

patrickhulce · 2017-11-03T20:39:14Z

lighthouse-core/runner.js

 const fs = require('fs');
 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) {


this method is a bit big for its 👖 these days, it seems like it can roughly be broken down into

loadOrGatherArtifacts

potentiallySaveArtifacts

potentiallyComputeAudits

combine results

do you think it'd be easier to handle the config.gatherMode type flags individually in each of these 4 hypothetical methods rather than exploding the matrix of choices? the plain english variable names of "this is what is happening" is kinda nice right now, but its quite a long function

Agreed on no real need to make a variable for each possible state. Rather than just four, can't we just have

loadArtifacts

gatherArtifacts

saveArtifacts

runAudits

score

createLHR
and just have run() run the ones that are needed for each flag?

sg. say hello to

_loadArtifactsFromDisk
_gatherArtifactsFromBrowser
_saveArtifacts
_runAudits
_scoreAndCategorize

patrickhulce · 2017-11-03T20:40:33Z

lighthouse-core/test/index-test.js

@@ -87,7 +87,7 @@ describe('Module Tests', function() {
      });
  });

-  it('should return formatted audit results when given no categories', function() {


oh wow you don't even get any category results? yeah let's nuke this

the original test was added to ensure that you'd still get json output even if there were no categories in the config (e.g. you don't care about categories). Is that still possible?

added a new test to assert things finish even if no categories in the config

patrickhulce · 2017-11-03T20:43:45Z

lighthouse-core/runner.js

-      });
+    // Entering: Gather phase
+    if (!config.passes && !config.artifacts && !shouldLoadArtifactsFromDisk) {
+      const err = new Error('You must require either gather passes or provide saved artifacts.');


"You must require" seems a bit odd, "You must provide either ..." or "The config requires ... " perhaps?

how about

No browser artifacts are either provided or requested.

sg, even further?

No browser artifacts provided or requested.

patrickhulce · 2017-11-03T20:44:02Z

lighthouse-core/test/gather/gather-runner-test.js

-        assert.equal(artifacts.networkRecords['firstPass'], undefined);
-        assert.equal(artifacts.networkRecords['secondPass'], undefined);
+        assert.equal(artifacts.networkRecords, undefined);
+        assert.equal(artifacts.networkRecords, undefined);


nit: remove one of these

patrickhulce · 2017-11-03T20:45:34Z

lighthouse-core/test/runner-test.js

 const computedArtifacts = Runner.instantiateComputedArtifacts();

 /* eslint-env mocha */

 describe('Runner', () => {
+  const saveArtifactsSpy = sinon.spy(assetSaver, 'saveArtifacts');
+  const loadArtifactsSpy = sinon.spy(assetSaver, 'loadArtifacts');


oh these are straight spies, I thought you were stubbing

I'd have a slight preference for stubbing loadArtifacts and asserting the LHR looks like we'd expect it to than 100% relying on 'was called' assertions

i think we'll have to discuss this in person, as all runner tests do actually run gatherrunner.run, runAudits, generateReport, etc. So only going with stubs for these seems inconsistent.
Also, we'll have to mimic what these methods do on the test side if we want to see that everything works (for example: how do we test that lighthouse -G completes successfully unless it can read the files off disk?)

I'm all for having at least some integration testing of this 👍

Maybe we'll chat in person but seems like there's room for both

brendankenny

GAAAAAAAAA(r)!

brendankenny · 2017-11-03T20:56:50Z

lighthouse-core/test/index-test.js

@@ -87,7 +87,7 @@ describe('Module Tests', function() {
      });
  });

-  it('should return formatted audit results when given no categories', function() {


the original test was added to ensure that you'd still get json output even if there were no categories in the config (e.g. you don't care about categories). Is that still possible?

brendankenny · 2017-11-03T20:59:13Z

lighthouse-cli/cli-flags.ts

@@ -66,6 +69,9 @@ export function getFlags(manualArgv?: string) {
        '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, save, & quit. However, if audit-mode is also enabled, then the run will complete after saving artifacts to disk.',


not sure how to parse the second part of this

brendankenny · 2017-11-03T21:00:10Z

lighthouse-cli/run.ts

-
-    await saveResults(results, artifacts!, flags);
-    await launchedChrome.kill();
+    if (shouldSaveResults){


can this be combined with the logic of whether or not to save already in saveResults?

brendankenny · 2017-11-03T21:01:05Z

lighthouse-cli/run.ts


    return results;
  } catch (err) {
+    return handleError(err);


does this work? handleError calls process.exit()

fair. changed it.

brendankenny · 2017-11-03T21:04:46Z

lighthouse-core/config/config.js

@@ -318,10 +316,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');


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

brendankenny · 2017-11-03T21:09:58Z

lighthouse-core/lib/asset-saver.js

+    // do everything else
+    delete artifacts.traces;
+    // The networkRecords artifacts have circular references
+    fs.writeFileSync(`${basePath}/artifacts.json`, stringifySafe(artifacts), 'utf8');


stringifySafe will be broken on re-import if we actually have circular references, so should just use JSON.stringify (and hopefully we've done things correctly :)

brendankenny · 2017-11-03T21:11:00Z

lighthouse-core/lib/asset-saver.js

+
+  const p = Promise.all(savePromies).then(_ => {
+    // do everything else
+    delete artifacts.traces;


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

brendankenny · 2017-11-03T21:13:21Z

lighthouse-core/runner.js

 const fs = require('fs');
 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) {


Agreed on no real need to make a variable for each possible state. Rather than just four, can't we just have

loadArtifacts

gatherArtifacts

saveArtifacts

runAudits

score

createLHR
and just have run() run the ones that are needed for each flag?

brendankenny · 2017-11-03T21:17:53Z

lighthouse-core/test/runner-test.js

      }],
+      audits: [


how does this test audit output without running a gatherer or audit?

it does run a gatherer and an audit. :o

brendankenny · 2017-11-03T21:18:53Z

lighthouse-core/config/config.js

@@ -318,10 +316,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');


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

wardpeet · 2017-11-04T15:20:32Z

will there be docs about this? This feature is really great if you run it in a cloud environment!

wardpeet · 2017-11-04T15:26:09Z

lighthouse-core/lib/asset-saver.js

-  log.log('artifacts file saved to disk', fullPath);
+function saveArtifacts(artifacts, basePath) {
+  if (!fs.existsSync(basePath)) {
+    fs.mkdirSync(basePath);


we should use mkdirp.sync here to make sure we can create dirs recursively (we should already have it as a dependency)

wardpeet · 2017-11-04T15:28:28Z

lighthouse-core/lib/asset-saver.js

+  if (!fs.existsSync(basePath)) {
+    fs.mkdirSync(basePath);
+  }
+  rimraf.sync(`${basePath}/*${traceSuffix}`);


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

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

paulirish · 2017-11-18T05:04:16Z

Okay folks.. A fresh update here.

The big changes to runner are in this commit: a3da663
There's no way to make that diff super straightforward, but I hope it's a bit easier to read now.

Otherwise, the remaining feedback has been addressed as well.

patrickhulce · 2017-11-20T19:11:27Z

lighthouse-cli/cli-flags.ts

@@ -66,6 +69,9 @@ export function getFlags(manualArgv?: string) {
        '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 quit early.',


the run will quit early?

patrickhulce · 2017-11-20T19:13:41Z

lighthouse-cli/run.ts

@@ -92,8 +92,12 @@ function handleError(err: LighthouseError) {
 }

 export function saveResults(results: Results, artifacts: Object, flags: Flags) {
+  const shouldSaveResults = flags.auditMode || (flags.gatherMode == flags.auditMode);
+  if (shouldSaveResults) return;


wait this seems backwards, shouldn't it be if (!shouldSaveResults)?

if not, maybe just method of variable name needs some tweaking :)

patrickhulce · 2017-11-20T19:14:47Z

lighthouse-cli/run.ts

-    }
-
-    return handleError(err);
+    await potentiallyKillChrome(launchedChrome) return handleError(err);


missing a ;?

patrickhulce · 2018-01-02T19:14:57Z

hey @devtools-bot why'd you change this :P

paulirish · 2018-01-05T02:46:19Z

Ready for another look.

patrickhulce

🚢 it!

patrickhulce · 2018-01-05T17:50:13Z

lighthouse-core/lib/asset-saver.js

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


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 :)

patrickhulce · 2018-01-05T17:55:42Z

lighthouse-core/runner.js

 const fs = require('fs');
 const path = require('path');
 const URL = require('./lib/url-shim');
 const Sentry = require('./lib/sentry');

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


let's add this to .gitignore :)

paulirish requested review from brendankenny and patrickhulce November 3, 2017 19:06

paulirish force-pushed the gar branch 2 times, most recently from 49567a9 to e8a056c Compare November 3, 2017 19:17

patrickhulce suggested changes Nov 3, 2017

View reviewed changes

brendankenny requested changes Nov 3, 2017

View reviewed changes

paulirish added the waiting4committer label Nov 4, 2017

wardpeet reviewed Nov 4, 2017

View reviewed changes

darcyclarke mentioned this pull request Nov 10, 2017

Remove <url> requirement #3803

Closed

paulirish added waiting4reviewer and removed waiting4committer labels Nov 18, 2017

patrickhulce suggested changes Nov 21, 2017

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Dec 14, 2017

devtools-bot added waiting4reviewer and removed waiting4committer labels Dec 18, 2017

patrickhulce added waiting4committer and removed waiting4reviewer labels Jan 2, 2018

paulirish force-pushed the gar branch from cec4f31 to 7aa5bc5 Compare January 5, 2018 02:16

paulirish added 8 commits January 4, 2018 18:18

-GA flags. tests.

2645c46

tests sorted except for auditResults.

5dd3d50

remove auditResults

e0c0013

cleanup.

f72379b

cli fixes

309401e

feedback.

7bdd889

cleanup CLI

d429332

mkdirp in assetsaver

a43b016

paulirish added 6 commits January 4, 2018 18:18

break apart runner into separate phase methods

80b6b96

update tests.

f072088

exclude devtoolsLogs from artifacts.json

245d9e3

fixup, post rebase

a7a4902

typecheck is happy

9332fd6

pr feedback

93aeddd

paulirish force-pushed the gar branch from 7aa5bc5 to 93aeddd Compare January 5, 2018 02:18

devtools-bot added waiting4reviewer and removed waiting4committer labels Jan 5, 2018

paulirish added 2 commits January 4, 2018 18:37

remove legacy --save-artifacts CLI flag.

ee704bd

cleanup promises a bit.

f14f2b0

patrickhulce approved these changes Jan 5, 2018

View reviewed changes

paulirish added 4 commits January 5, 2018 10:20

convert Script artifact to dict

bda2045

feedback.

1ac586c

logic error: don't save artifacts on a non -G/-A run

04b7a09

exclude mkdirp/rimraf from going to browserified output

9a4b36d

paulirish force-pushed the gar branch from 6a25244 to 9a4b36d Compare January 5, 2018 19:51

paulirish mentioned this pull request Jan 5, 2018

FontSize artifact isn't serializable #4184

Closed

paulirish merged commit a2bcf82 into master Jan 5, 2018

paulirish deleted the gar branch January 5, 2018 20:17

paulirish removed the waiting4reviewer label Mar 6, 2018

core(lifecycle): allow gathering & auditing to run separately #3743

core(lifecycle): allow gathering & auditing to run separately #3743

Conversation

paulirish commented Nov 3, 2017 • edited Loading

Todo:

Future work:

patrickhulce 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

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

paulirish Nov 18, 2017 • edited Loading

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

wardpeet commented Nov 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

paulirish commented Nov 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jan 2, 2018

paulirish commented Jan 5, 2018

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Nov 3, 2017 •

edited

Loading

paulirish Nov 18, 2017 •

edited

Loading

paulirish commented Nov 18, 2017 •

edited

Loading