core(runner): add custom folder support to -G/-A #4792

paulirish · 2018-03-17T00:45:33Z

Enable this:

lighthouse -G=./cnn_artifacts http://cnn.com
...
lighthouse -A=./cnn_artifacts http://cnn.com

Path is resolved via pwd, but absolute paths are allowable, too.

patrickhulce · 2018-03-17T01:06:46Z

lighthouse-core/runner.js

@@ -144,18 +143,20 @@ class Runner {
    return GatherRunner.run(opts.config.passes, opts).then(artifacts => {
      const flags = opts.flags;
      const shouldSave = flags.gatherMode;
-      const p = shouldSave ? Runner._saveArtifacts(artifacts): Promise.resolve();
+      const path = Runner._getArtifactsPath(flags);
+      const p = shouldSave ? Runner._saveArtifacts(artifacts, path): Promise.resolve();


since we just bumped our ecmaVersion how about some async/await to kill this dumb so-node-6 p variable :D

patrickhulce

❤️it!

brendankenny · 2018-03-19T20:24:21Z

lighthouse-core/runner.js

@@ -68,7 +66,7 @@ class Runner {
    // Gather phase
    // Either load saved artifacts off disk, from config, or get from the browser
    if (opts.flags.auditMode && !opts.flags.gatherMode) {
-      run = run.then(_ => Runner._loadArtifactsFromDisk());
+      run = run.then(_ => Runner._loadArtifactsFromDisk(Runner._getArtifactsPath(opts.flags)));


nit: pull into a var

brendankenny · 2018-03-19T20:24:40Z

lighthouse-core/runner.js

@@ -135,27 +134,30 @@ class Runner {
   * @param {*} connection
   * @return {!Promise<!Artifacts>}
   */
-  static _gatherArtifactsFromBrowser(opts, connection) {
+  static async _gatherArtifactsFromBrowser(opts, connection) {


brendankenny · 2018-03-19T20:26:42Z

lighthouse-core/runner.js

+    const flags = opts.flags;
+    if (flags.gatherMode) {
+      const path = Runner._getArtifactsPath(flags);
+      await Runner._saveArtifacts(artifacts, path);


inherited, but this would probably make more sense back up in run()

true. but how about we do that when runner goes async/await?

Well you're already refactoring it to async/await so you brought it on yourself :P

It doesn't look like there are any tests of this method (since it's private, yay), so should pop into the surrounding else branch with no side effects?

brendankenny · 2018-03-19T20:27:04Z

lighthouse-core/runner.js

 }

 module.exports = Runner;
+
+


extra line?

brendankenny · 2018-03-19T20:28:31Z

lighthouse-cli/cli-flags.js

@@ -111,7 +111,7 @@ function getFlags(manualArgv) {
        'disable-storage-reset', 'disable-device-emulation', 'disable-cpu-throttling',
        'disable-network-throttling', 'save-assets', 'list-all-audits',
        'list-trace-categories', 'perf', 'view', 'verbose', 'quiet', 'help',
-        'gather-mode', 'audit-mode', 'mixed-content',
+        'mixed-content',


mention path option in description?

brendankenny · 2018-03-19T20:29:10Z

lighthouse-core/runner.js

+
+  /**
+   * Get path to use for -G and -A modes. Defaults to $CWD/latest-run
+   * @param {*} flags


more specific type pls

also need to update externs.d.ts

brendankenny · 2018-03-19T20:35:15Z

lighthouse-core/test/runner-test.js

+    const artifactsPath = '.tmp/test_artifacts';
+
+    after(() => {
+      const path = Runner._getArtifactsPath({auditMode: artifactsPath});


test to verify that artifacts do exist at that location? And preferably since Runner._getArtifactsPath is private no need to call it for rimraf if verified that runner is saving at that location

brendankenny · 2018-03-19T23:06:30Z

lighthouse-core/runner.js

+   */
+  static _getArtifactsPath(flags) {
+    // This enables usage like: -GA=./custom-folder
+    if (typeof flags.auditMode === 'string') return path.join(process.cwd(), flags.auditMode);


I think you want path.resolve? To handle absolute paths

brendankenny · 2018-03-19T23:06:50Z

lighthouse-core/test/runner-test.js

@@ -36,17 +38,23 @@ describe('Runner', () => {
    resetSpies();
  });

-  describe('Gather Mode & Audit Mode', () => {
+  describe.only('Gather Mode & Audit Mode', () => {


brendankenny · 2018-03-19T23:08:43Z

lighthouse-core/test/runner-test.js

    const url = 'https://example.com';
    const generateConfig = _ => new Config({
      passes: [{
        gatherers: ['viewport-dimensions'],
      }],
      audits: ['content-width'],
    });
+    const artifactsPath = '.tmp/test_artifacts';
+    const resolvedPath = Runner._getArtifactsPath({auditMode: artifactsPath});


since the expected behavior is that we support a path relative to cwd, I think we want an impl independent of the thing itself to test it (e.g. const resolvedPath = path.resolve(process.cwd(), artifactsPath) or whatever works from this file)

brendankenny

LGTM on the 🍏!

paulirish requested a review from patrickhulce as a code owner March 17, 2018 00:45

devtools-bot added the waiting4reviewer label Mar 17, 2018

patrickhulce reviewed Mar 17, 2018

View reviewed changes

paulirish force-pushed the customfoldersupport branch from 1543d58 to 1ba7d5a Compare March 19, 2018 20:02

patrickhulce approved these changes Mar 19, 2018

View reviewed changes

brendankenny requested changes Mar 19, 2018

View reviewed changes

paulirish mentioned this pull request Mar 19, 2018

deps(browserify): update deep transitive dependency to use recent acorn #4813

Merged

paulirish added 3 commits March 19, 2018 15:30

core(runner): add custom folder support to -G/-A

6b45ef7

docs.

fb7f04c

await, fam

99f66c0

paulirish force-pushed the customfoldersupport branch from 9ac25f4 to 56c2a2b Compare March 19, 2018 22:45

feedback.

0589b8f

paulirish force-pushed the customfoldersupport branch from 56c2a2b to 0589b8f Compare March 19, 2018 22:48

brendankenny requested changes Mar 19, 2018

View reviewed changes

feedback.

d33a6f2

brendankenny approved these changes Mar 19, 2018

View reviewed changes

paulirish merged commit 8b02a90 into master Mar 20, 2018

paulirish deleted the customfoldersupport branch April 27, 2018 21:51

paulirish removed the waiting4reviewer label Apr 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(runner): add custom folder support to -G/-A #4792

core(runner): add custom folder support to -G/-A #4792

paulirish commented Mar 17, 2018

patrickhulce Mar 17, 2018 •

edited

Loading

paulirish Mar 17, 2018

paulirish Mar 19, 2018

patrickhulce left a comment

brendankenny Mar 19, 2018

brendankenny Mar 19, 2018

brendankenny Mar 19, 2018

paulirish Mar 19, 2018

brendankenny Mar 19, 2018

brendankenny Mar 19, 2018

brendankenny Mar 19, 2018

brendankenny Mar 19, 2018

brendankenny Mar 19, 2018

brendankenny Mar 19, 2018

brendankenny Mar 19, 2018

brendankenny Mar 19, 2018

brendankenny Mar 19, 2018

brendankenny left a comment

core(runner): add custom folder support to -G/-A #4792

core(runner): add custom folder support to -G/-A #4792

Conversation

paulirish commented Mar 17, 2018

patrickhulce Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce Mar 17, 2018 •

edited

Loading