Repository: GoogleChrome/lighthouse
Pull request #6151 (2018-10-01 19:34:54)

core(computed): fix new computed artifact interface
#5907 didn't quite work because computed artifacts need to call each other, so need access to the `context` to pass it on to the next computed artifact :)

Since that's now explicit, flipped the order of the parameters so it's `compute_(inputArtifacts, context)`, which matches the interface for `audit.audit()` (not sure why I did it the other way before). About 3/4 of this PR is flipping the order for `manifest-values` from #5907.

Also updated `screenshots.js` to this new format.


Patch:
diff --git a/lighthouse-core/audits/final-screenshot.js b/lighthouse-core/audits/final-screenshot.js
index 11f852ad8..e5a6264d1 100644
--- a/lighthouse-core/audits/final-screenshot.js
+++ b/lighthouse-core/audits/final-screenshot.js
@@ -7,6 +7,7 @@
 
 const Audit = require('./audit');
 const LHError = require('../lib/lh-error');
+const Screenshots = require('../gather/computed/screenshots.js');
 
 class FinalScreenshot extends Audit {
   /**
@@ -24,11 +25,12 @@ class FinalScreenshot extends Audit {
 
   /**
    * @param {LH.Artifacts} artifacts
+   * @param {LH.Audit.Context} context
    * @return {Promise<LH.Audit.Product>}
    */
-  static async audit(artifacts) {
+  static async audit(artifacts, context) {
     const trace = artifacts.traces[Audit.DEFAULT_PASS];
-    const screenshots = await artifacts.requestScreenshots(trace);
+    const screenshots = await Screenshots.request(trace, context);
     const finalScreenshot = screenshots[screenshots.length - 1];
 
     if (!finalScreenshot) {
diff --git a/lighthouse-core/audits/manifest-short-name-length.js b/lighthouse-core/audits/manifest-short-name-length.js
index 71979f640..7a0041d20 100644
--- a/lighthouse-core/audits/manifest-short-name-length.js
+++ b/lighthouse-core/audits/manifest-short-name-length.js
@@ -30,7 +30,7 @@ class ManifestShortNameLength extends Audit {
    * @return {Promise<LH.Audit.Product>}
    */
   static async audit(artifacts, context) {
-    const manifestValues = await ManifestValues.request(context, artifacts.Manifest);
+    const manifestValues = await ManifestValues.request(artifacts.Manifest, context);
     // If there's no valid manifest, this audit is not applicable
     if (manifestValues.isParseFailure) {
       return {
diff --git a/lighthouse-core/audits/splash-screen.js b/lighthouse-core/audits/splash-screen.js
index 568f938b8..a848cdaac 100644
--- a/lighthouse-core/audits/splash-screen.js
+++ b/lighthouse-core/audits/splash-screen.js
@@ -72,7 +72,7 @@ class SplashScreen extends MultiCheckAudit {
     /** @type {Array<string>} */
     const failures = [];
 
-    const manifestValues = await ManifestValues.request(context, artifacts.Manifest);
+    const manifestValues = await ManifestValues.request(artifacts.Manifest, context);
     SplashScreen.assessManifest(manifestValues, failures);
 
     return {
diff --git a/lighthouse-core/audits/themed-omnibox.js b/lighthouse-core/audits/themed-omnibox.js
index b3760b7ca..6a95190e2 100644
--- a/lighthouse-core/audits/themed-omnibox.js
+++ b/lighthouse-core/audits/themed-omnibox.js
@@ -79,7 +79,7 @@ class ThemedOmnibox extends MultiCheckAudit {
     /** @type {Array<string>} */
     const failures = [];
 
-    const manifestValues = await ManifestValues.request(context, artifacts.Manifest);
+    const manifestValues = await ManifestValues.request(artifacts.Manifest, context);
     ThemedOmnibox.assessManifest(manifestValues, failures);
     ThemedOmnibox.assessMetaThemecolor(artifacts.ThemeColor, failures);
 
diff --git a/lighthouse-core/audits/webapp-install-banner.js b/lighthouse-core/audits/webapp-install-banner.js
index d08fb5bca..804238fb8 100644
--- a/lighthouse-core/audits/webapp-install-banner.js
+++ b/lighthouse-core/audits/webapp-install-banner.js
@@ -128,7 +128,7 @@ class WebappInstallBanner extends MultiCheckAudit {
     /** @type {Array<string>} */
     let offlineWarnings = [];
 
-    const manifestValues = await ManifestValues.request(context, artifacts.Manifest);
+    const manifestValues = await ManifestValues.request(artifacts.Manifest, context);
     const manifestFailures = WebappInstallBanner.assessManifest(manifestValues);
     const swFailures = WebappInstallBanner.assessServiceWorker(artifacts);
     if (!swFailures.length) {
diff --git a/lighthouse-core/gather/computed/new-computed-artifact.js b/lighthouse-core/gather/computed/new-computed-artifact.js
index f77136519..b3d1e22c9 100644
--- a/lighthouse-core/gather/computed/new-computed-artifact.js
+++ b/lighthouse-core/gather/computed/new-computed-artifact.js
@@ -11,25 +11,26 @@ const ArbitraryEqualityMap = require('../../lib/arbitrary-equality-map.js');
  * @template {string} N
  * @template A
  * @template R
- * @param {{name: N, compute_: (artifact: A) => Promise<R>}} computableArtifact
- * @return {{name: N, request: (context: LH.Audit.Context, artifact: A) => Promise<R>}}
+ * @param {{name: N, compute_: (artifacts: A, context: LH.Audit.Context) => Promise<R>}} computableArtifact
+ * @return {{name: N, request: (artifacts: A, context: LH.Audit.Context) => Promise<R>}}
  */
 function makeComputedArtifact(computableArtifact) {
   /**
+   * @param {A} artifacts
    * @param {LH.Audit.Context} context
-   * @param {A} artifact
    */
-  const request = ({computedCache}, artifact) => {
+  const request = (artifacts, context) => {
+    const computedCache = context.computedCache;
     const cache = computedCache.get(computableArtifact.name) || new ArbitraryEqualityMap();
     computedCache.set(computableArtifact.name, cache);
 
-    const computed = /** @type {Promise<R>|undefined} */ (cache.get(artifact));
+    const computed = /** @type {Promise<R>|undefined} */ (cache.get(artifacts));
     if (computed) {
       return computed;
     }
 
-    const artifactPromise = computableArtifact.compute_(artifact);
-    cache.set(artifact, artifactPromise);
+    const artifactPromise = computableArtifact.compute_(artifacts, context);
+    cache.set(artifacts, artifactPromise);
 
     return artifactPromise;
   };
diff --git a/lighthouse-core/gather/computed/screenshots.js b/lighthouse-core/gather/computed/screenshots.js
index 625616a51..3e0cc4dde 100644
--- a/lighthouse-core/gather/computed/screenshots.js
+++ b/lighthouse-core/gather/computed/screenshots.js
@@ -5,20 +5,16 @@
  */
 'use strict';
 
-const ComputedArtifact = require('./computed-artifact');
+const makeComputedArtifact = require('./new-computed-artifact');
 
 const SCREENSHOT_TRACE_NAME = 'Screenshot';
 
-class ScreenshotFilmstrip extends ComputedArtifact {
-  get name() {
-    return 'Screenshots';
-  }
-
+class Screenshots {
   /**
    * @param {LH.Trace} trace
    * @return {Promise<Array<{timestamp: number, datauri: string}>>}
   */
-  async compute_(trace) {
+  static async compute_(trace) {
     return trace.traceEvents
       .filter(evt => evt.name === SCREENSHOT_TRACE_NAME)
       .map(evt => {
@@ -30,4 +26,4 @@ class ScreenshotFilmstrip extends ComputedArtifact {
   }
 }
 
-module.exports = ScreenshotFilmstrip;
+module.exports = makeComputedArtifact(Screenshots);
diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js
index 096ed4d39..5a9f5c708 100644
--- a/lighthouse-core/runner.js
+++ b/lighthouse-core/runner.js
@@ -387,6 +387,7 @@ class Runner {
       // Computed artifacts switching to the new system.
       'new-computed-artifact.js',
       'manifest-values.js',
+      'screenshots.js',
     ];
 
     const fileList = [
diff --git a/typings/artifacts.d.ts b/typings/artifacts.d.ts
index a4110bc9f..63aca08d5 100644
--- a/typings/artifacts.d.ts
+++ b/typings/artifacts.d.ts
@@ -131,7 +131,6 @@ declare global {
       requestPushedRequests(devtoolsLogs: DevtoolsLog): Promise<Artifacts.NetworkRequest[]>;
       requestMainThreadTasks(trace: Trace): Promise<Artifacts.TaskNode[]>;
       requestTraceOfTab(trace: Trace): Promise<Artifacts.TraceOfTab>;
-      requestScreenshots(trace: Trace): Promise<{timestamp: number, datauri: string}[]>;
       requestSpeedline(trace: Trace): Promise<LH.Artifacts.Speedline>;
 
       // Metrics.


Test patch:
diff --git a/lighthouse-core/test/audits/final-screenshot-test.js b/lighthouse-core/test/audits/final-screenshot-test.js
index 86ed9d564..b3d32531d 100644
--- a/lighthouse-core/test/audits/final-screenshot-test.js
+++ b/lighthouse-core/test/audits/final-screenshot-test.js
@@ -7,24 +7,17 @@
 
 const assert = require('assert');
 
-const Runner = require('../../runner.js');
 const FinalScreenshotAudit = require('../../audits/final-screenshot');
 const pwaTrace = require('../fixtures/traces/progressive-app-m60.json');
 
 /* eslint-env jest */
 
 describe('Final screenshot', () => {
-  let computedArtifacts;
-
-  beforeAll(() => {
-    computedArtifacts = Runner.instantiateComputedArtifacts();
-  });
-
   it('should extract a final screenshot from a trace', async () => {
     const artifacts = Object.assign({
       traces: {defaultPass: pwaTrace},
-    }, computedArtifacts);
-    const results = await FinalScreenshotAudit.audit(artifacts);
+    });
+    const results = await FinalScreenshotAudit.audit(artifacts, {computedCache: new Map()});
 
     assert.ok(results.rawValue);
     assert.equal(results.details.timestamp, 225414990.064);
diff --git a/lighthouse-core/test/gather/computed/manifest-values-test.js b/lighthouse-core/test/gather/computed/manifest-values-test.js
index 34a32ca4f..b040d7b8f 100644
--- a/lighthouse-core/test/gather/computed/manifest-values-test.js
+++ b/lighthouse-core/test/gather/computed/manifest-values-test.js
@@ -35,7 +35,7 @@ function noUrlManifestParser(manifestSrc) {
 describe('ManifestValues computed artifact', () => {
   it('reports a parse failure if page had no manifest', async () => {
     const manifestArtifact = null;
-    const results = await ManifestValues.request(getMockContext(), manifestArtifact);
+    const results = await ManifestValues.request(manifestArtifact, getMockContext());
     assert.equal(results.isParseFailure, true);
     assert.ok(results.parseFailureReason, 'No manifest was fetched');
     assert.equal(results.allChecks.length, 0);
@@ -43,7 +43,7 @@ describe('ManifestValues computed artifact', () => {
 
   it('reports a parse failure if page had an unparseable manifest', async () => {
     const manifestArtifact = noUrlManifestParser('{:,}');
-    const results = await ManifestValues.request(getMockContext(), manifestArtifact);
+    const results = await ManifestValues.request(manifestArtifact, getMockContext());
     assert.equal(results.isParseFailure, true);
     assert.ok(results.parseFailureReason.includes('failed to parse as valid JSON'));
     assert.equal(results.allChecks.length, 0);
@@ -51,14 +51,14 @@ describe('ManifestValues computed artifact', () => {
 
   it('passes the parsing checks on an empty manifest', async () => {
     const manifestArtifact = noUrlManifestParser('{}');
-    const results = await ManifestValues.request(getMockContext(), manifestArtifact);
+    const results = await ManifestValues.request(manifestArtifact, getMockContext());
     assert.equal(results.isParseFailure, false);
     assert.equal(results.parseFailureReason, undefined);
   });
 
   it('passes the all checks with fixture manifest', async () => {
     const manifestArtifact = noUrlManifestParser(manifestSrc);
-    const results = await ManifestValues.request(getMockContext(), manifestArtifact);
+    const results = await ManifestValues.request(manifestArtifact, getMockContext());
     assert.equal(results.isParseFailure, false);
     assert.equal(results.parseFailureReason, undefined);
 
@@ -71,7 +71,7 @@ describe('ManifestValues computed artifact', () => {
       const Manifest = noUrlManifestParser(JSON.stringify({
         start_url: '/',
       }));
-      const results = await ManifestValues.request(getMockContext(), Manifest);
+      const results = await ManifestValues.request(Manifest, getMockContext());
       const colorResults = results.allChecks.filter(i => i.id.includes('Color'));
       assert.equal(colorResults.every(i => i.passing === false), true);
     });
@@ -82,7 +82,7 @@ describe('ManifestValues computed artifact', () => {
         theme_color: 'no',
       }));
 
-      const results = await ManifestValues.request(getMockContext(), Manifest);
+      const results = await ManifestValues.request(Manifest, getMockContext());
       const colorResults = results.allChecks.filter(i => i.id.includes('Color'));
       assert.equal(colorResults.every(i => i.passing === false), true);
     });
@@ -93,7 +93,7 @@ describe('ManifestValues computed artifact', () => {
         theme_color: '#FAFAFA',
       }));
 
-      const results = await ManifestValues.request(getMockContext(), Manifest);
+      const results = await ManifestValues.request(Manifest, getMockContext());
       const colorResults = results.allChecks.filter(i => i.id.includes('Color'));
       assert.equal(colorResults.every(i => i.passing === true), true);
     });
@@ -127,7 +127,7 @@ describe('ManifestValues computed artifact', () => {
           name: 'NoIconsHere',
         });
         const Manifest = noUrlManifestParser(manifestSrc);
-        const results = await ManifestValues.request(getMockContext(), Manifest);
+        const results = await ManifestValues.request(Manifest, getMockContext());
         const iconResults = results.allChecks.filter(i => i.id.includes('Icons'));
         assert.equal(iconResults.every(i => i.passing === false), true);
       });
@@ -137,7 +137,7 @@ describe('ManifestValues computed artifact', () => {
           icons: [],
         });
         const Manifest = noUrlManifestParser(manifestSrc);
-        const results = await ManifestValues.request(getMockContext(), Manifest);
+        const results = await ManifestValues.request(Manifest, getMockContext());
         const iconResults = results.allChecks.filter(i => i.id.includes('Icons'));
         assert.equal(iconResults.every(i => i.passing === false), true);
       });
@@ -151,7 +151,7 @@ describe('ManifestValues computed artifact', () => {
           }],
         });
         const Manifest = noUrlManifestParser(manifestSrc);
-        const results = await ManifestValues.request(getMockContext(), Manifest);
+        const results = await ManifestValues.request(Manifest, getMockContext());
         const iconResults = results.allChecks.filter(i => i.id.includes('Icons'));
 
         assert.equal(iconResults.every(i => i.passing === false), true);
@@ -165,7 +165,7 @@ describe('ManifestValues computed artifact', () => {
           }],
         });
         const Manifest = noUrlManifestParser(manifestSrc);
-        const results = await ManifestValues.request(getMockContext(), Manifest);
+        const results = await ManifestValues.request(Manifest, getMockContext());
         const iconResults = results.allChecks.filter(i => i.id.includes('Icons'));
 
         assert.equal(iconResults.every(i => i.passing === true), true);
@@ -181,7 +181,7 @@ describe('ManifestValues computed artifact', () => {
           }],
         });
         const Manifest = noUrlManifestParser(manifestSrc);
-        const results = await ManifestValues.request(getMockContext(), Manifest);
+        const results = await ManifestValues.request(Manifest, getMockContext());
         const iconResults = results.allChecks.filter(i => i.id.includes('Icons'));
 
         assert.equal(iconResults.every(i => i.passing === true), true);
@@ -196,7 +196,7 @@ describe('ManifestValues computed artifact', () => {
           }],
         });
         const Manifest = noUrlManifestParser(manifestSrc);
-        const results = await ManifestValues.request(getMockContext(), Manifest);
+        const results = await ManifestValues.request(Manifest, getMockContext());
         const iconResults = results.allChecks.filter(i => i.id.includes('Icons'));
 
         assert.equal(iconResults.every(i => i.passing === false), true);
diff --git a/lighthouse-core/test/gather/computed/screenshots-test.js b/lighthouse-core/test/gather/computed/screenshots-test.js
index 4c7ddf123..2f32e51a0 100644
--- a/lighthouse-core/test/gather/computed/screenshots-test.js
+++ b/lighthouse-core/test/gather/computed/screenshots-test.js
@@ -6,22 +6,14 @@
 'use strict';
 
 /* eslint-env jest */
-const ScreenshotsGather = require('../../../gather/computed/screenshots');
-const Runner = require('../../../runner.js');
+const Screenshots = require('../../../gather/computed/screenshots');
 const assert = require('assert');
 const pwaTrace = require('../../fixtures/traces/progressive-app.json');
 
-const screenshotsGather = new ScreenshotsGather({});
-
-describe('Screenshot gatherer', () => {
+describe('Screenshot computed artifact', () => {
   it('returns an artifact for a real trace', () => {
-    const artifacts = Object.assign({
-      traces: {
-        [screenshotsGather.DEFAULT_PASS]: pwaTrace,
-      },
-    }, Runner.instantiateComputedArtifacts());
-
-    return artifacts.requestScreenshots({traceEvents: pwaTrace}).then(screenshots => {
+    const context = {computedCache: new Map()};
+    return Screenshots.request({traceEvents: pwaTrace}, context).then(screenshots => {
       assert.ok(Array.isArray(screenshots));
       assert.equal(screenshots.length, 7);