Repository: GoogleChrome/lighthouse
Pull request #5684 (2018-07-18 18:58:19)

core: expose LighthouseRunWarnings on audit context
I was thinking about #5666 this morning and realized we shouldn't be altering artifacts in audits. Instead, this exposes a way to push top-level warnings from audits via the `auditContext`, similar to [how gatherers can do so from the `passContext`](https://github.com/GoogleChrome/lighthouse/blob/098d28189ba33f292d4f06824d62116a372aa976/lighthouse-core/gather/gather-runner.js#L411-L412).

happy to bikeshed on anything

brendankenny: > Is there a user-facing downside of mutating the artifact?

In `-A` mode we still pass the `artifacts` back out, so you'd get the artifacts with an additional warning from what you passed in. In the CLI we don't save the artifacts if it's just `-A` mode, but if used as a node module, repeated processing would add more and more extension bootup time warnings to the run warnings. It is admittedly a minor problem :)

Mostly it's the conceptual cleanliness, which is why I mentioned being open to bikeshedding on the approach.

> AFAICT, the gatherers are directly pushing onto the same array that gets exposed as an artifact, so it already seemed pretty much the same to me.

Right, and here it parallels that with audits pushing onto the warnings array that gets exposed on the LHR (which also includes the warnings copied from the artifact). Keeps artifact generation solely in gatherRunner and gatherers, but yes, in the end result functionally equivalent.
brendankenny: > In the CLI 

actually, maybe it's observable in the CLI if run with `-GA` and then again with `-A` you'd get two copies of the warning in the resulting report, one saved from the original `-A` and one added by the second -A (you'd just max out at two warnings as the artifacts wouldn't be re-saved from the second run)
patrickhulce: Ah gotcha `-A` + node module, yeah makes sense! In CLI `-GA`, we should be saving the artifacts before audit mode so the saved artifacts should never have results of auditing, right?
https://github.com/GoogleChrome/lighthouse/blob/098d28189ba33f292d4f06824d62116a372aa976/lighthouse-core/runner.js#L84-L87

either way, this seems good 👍 
brendankenny: > In CLI -GA, we should be saving the artifacts before audit mode

oh riiiight, yeah, good call

Patch:
diff --git a/lighthouse-core/audits/bootup-time.js b/lighthouse-core/audits/bootup-time.js
index 8768e56a2..51d80344b 100644
--- a/lighthouse-core/audits/bootup-time.js
+++ b/lighthouse-core/audits/bootup-time.js
@@ -23,7 +23,7 @@ class BootupTime extends Audit {
       description: 'Consider reducing the time spent parsing, compiling, and executing JS. ' +
         'You may find delivering smaller JS payloads helps with this. [Learn ' +
         'more](https://developers.google.com/web/tools/lighthouse/audits/bootup).',
-      requiredArtifacts: ['traces', 'LighthouseRunWarnings'],
+      requiredArtifacts: ['traces'],
     };
   }
 
@@ -85,7 +85,6 @@ class BootupTime extends Audit {
    * @return {Promise<LH.Audit.Product>}
    */
   static async audit(artifacts, context) {
-    const runWarnings = artifacts.LighthouseRunWarnings;
     const settings = context.settings || {};
     const trace = artifacts.traces[BootupTime.DEFAULT_PASS];
     const devtoolsLog = artifacts.devtoolsLogs[BootupTime.DEFAULT_PASS];
@@ -133,8 +132,8 @@ class BootupTime extends Audit {
 
     // TODO: consider moving this to core gathering so you don't need to run the audit for warning
     if (hadExcessiveChromeExtension) {
-      runWarnings.push('Chrome extensions negatively affected this page\'s load performance. ' +
-        'Try auditing the page in incognito mode or from a clean Chrome profile.');
+      context.LighthouseRunWarnings.push('Chrome extensions negatively affected this page\'s load' +
+        ' performance. Try auditing the page in incognito mode or from a clean Chrome profile.');
     }
 
     const summary = {wastedMs: totalBootupTime};
diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js
index 77717cab6..e84410700 100644
--- a/lighthouse-core/runner.js
+++ b/lighthouse-core/runner.js
@@ -94,7 +94,8 @@ class Runner {
       if (!opts.config.audits) {
         throw new Error('No audits to evaluate.');
       }
-      const auditResults = await Runner._runAudits(settings, opts.config.audits, artifacts);
+      const auditResults = await Runner._runAudits(settings, opts.config.audits, artifacts,
+          lighthouseRunWarnings);
 
       // LHR construction phase
       log.log('status', 'Generating results...');
@@ -166,13 +167,14 @@ class Runner {
   }
 
   /**
-   * Save collected artifacts to disk
+   * Run all audits with specified settings and artifacts.
    * @param {LH.Config.Settings} settings
    * @param {Array<LH.Config.AuditDefn>} audits
    * @param {LH.Artifacts} artifacts
+   * @param {Array<string>} runWarnings
    * @return {Promise<Array<LH.Audit.Result>>}
    */
-  static async _runAudits(settings, audits, artifacts) {
+  static async _runAudits(settings, audits, artifacts, runWarnings) {
     log.log('status', 'Analyzing and running audits...');
     artifacts = Object.assign({}, Runner.instantiateComputedArtifacts(), artifacts);
 
@@ -190,7 +192,7 @@ class Runner {
     // Run each audit sequentially
     const auditResults = [];
     for (const auditDefn of audits) {
-      const auditResult = await Runner._runAudit(auditDefn, artifacts, settings);
+      const auditResult = await Runner._runAudit(auditDefn, artifacts, settings, runWarnings);
       auditResults.push(auditResult);
     }
 
@@ -203,10 +205,11 @@ class Runner {
    * @param {LH.Config.AuditDefn} auditDefn
    * @param {LH.Artifacts} artifacts
    * @param {LH.Config.Settings} settings
+   * @param {Array<string>} runWarnings
    * @return {Promise<LH.Audit.Result>}
    * @private
    */
-  static async _runAudit(auditDefn, artifacts, settings) {
+  static async _runAudit(auditDefn, artifacts, settings, runWarnings) {
     const audit = auditDefn.implementation;
     const status = `Evaluating: ${audit.meta.title}`;
 
@@ -254,7 +257,13 @@ class Runner {
 
       // all required artifacts are in good shape, so we proceed
       const auditOptions = Object.assign({}, audit.defaultOptions, auditDefn.options);
-      const product = await audit.audit(artifacts, {options: auditOptions, settings: settings});
+      const auditContext = {
+        options: auditOptions,
+        settings,
+        LighthouseRunWarnings: runWarnings,
+      };
+
+      const product = await audit.audit(artifacts, auditContext);
       auditResult = Audit.generateAuditResult(audit, product);
     } catch (err) {
       log.warn(audit.meta.id, `Caught exception: ${err.message}`);
diff --git a/typings/audit.d.ts b/typings/audit.d.ts
index 66e84e482..15527db54 100644
--- a/typings/audit.d.ts
+++ b/typings/audit.d.ts
@@ -7,8 +7,11 @@
 declare global {
   module LH.Audit {
     export interface Context {
-      options: Record<string, any>; // audit options
+      /** audit options */
+      options: Record<string, any>;
       settings: Config.Settings;
+      /** Push to this array to add top-level warnings to the LHR. */
+      LighthouseRunWarnings: Array<string>;
     }
 
     export interface ScoreOptions {
diff --git a/typings/gatherer.d.ts b/typings/gatherer.d.ts
index 7d679db96..db4755b36 100644
--- a/typings/gatherer.d.ts
+++ b/typings/gatherer.d.ts
@@ -18,7 +18,7 @@ declare global {
       passConfig: Config.Pass
       settings: Config.Settings;
       options?: object;
-      /** Push to this array to add top-level warnings to the LHR.  */
+      /** Push to this array to add top-level warnings to the LHR. */
       LighthouseRunWarnings: Array<string>;
     }
 


Test patch:
diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js
index 4f765d65c..f266ed7f3 100644
--- a/lighthouse-core/test/runner-test.js
+++ b/lighthouse-core/test/runner-test.js
@@ -36,6 +36,13 @@ describe('Runner', () => {
     resetSpies();
   });
 
+  const basicAuditMeta = {
+    id: 'test-audit',
+    title: 'A test audit',
+    description: 'An audit for testing',
+    requiredArtifacts: [],
+  };
+
   describe('Gather Mode & Audit Mode', () => {
     const url = 'https://example.com';
     const generateConfig = settings => new Config({
@@ -546,6 +553,58 @@ describe('Runner', () => {
     });
   });
 
+  it('includes any LighthouseRunWarnings from audits in LHR', () => {
+    const warningString = 'Really important audit warning!';
+
+    const config = new Config({
+      settings: {
+        auditMode: __dirname + '/fixtures/artifacts/empty-artifacts/',
+      },
+      audits: [
+        class WarningAudit extends Audit {
+          static get meta() {
+            return basicAuditMeta;
+          }
+          static audit(artifacts, context) {
+            context.LighthouseRunWarnings.push(warningString);
+            return {
+              rawValue: 5,
+            };
+          }
+        },
+      ],
+    });
+
+    return Runner.run(null, {config, driverMock}).then(results => {
+      assert.deepStrictEqual(results.lhr.runWarnings, [warningString]);
+    });
+  });
+
+  it('includes any LighthouseRunWarnings from errored audits in LHR', () => {
+    const warningString = 'Audit warning just before a terrible error!';
+
+    const config = new Config({
+      settings: {
+        auditMode: __dirname + '/fixtures/artifacts/empty-artifacts/',
+      },
+      audits: [
+        class WarningAudit extends Audit {
+          static get meta() {
+            return basicAuditMeta;
+          }
+          static audit(artifacts, context) {
+            context.LighthouseRunWarnings.push(warningString);
+            throw new Error('Terrible.');
+          }
+        },
+      ],
+    });
+
+    return Runner.run(null, {config, driverMock}).then(results => {
+      assert.deepStrictEqual(results.lhr.runWarnings, [warningString]);
+    });
+  });
+
   it('can handle array of outputs', async () => {
     const url = 'https://example.com';
     const config = new Config({