report: add method to get the final screenshot #5673

paulirish · 2018-07-17T04:17:18Z

Util.getFinalScreenshot(lhr);  // data uri string

patrickhulce · 2018-07-17T16:58:27Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

+   * @override
+   */
+  static getLastScreenshot(category) {
+    const auditRef = category.auditRefs.find(audit => audit.id === 'screenshot-thumbnails');


should we give them a new proper full-res final screenshot audit?

brendankenny · 2018-07-17T21:48:29Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

+  /**
+   * @param {LH.ReportResult.Category} category
+   * @return {null|string}
+   * @override


don't think this overrides anything?

brendankenny · 2018-07-17T21:58:22Z

lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js


  beforeAll(() => {
    global.URL = URL;
    global.Util = Util;
    global.CriticalRequestChainRenderer = CriticalRequestChainRenderer;
-    global.CategoryRenderer = CategoryRenderer;


PerformanceCategoryRenderer has to be in a higher scope (for the new suite)
and PerformanceCategoryRenderer extends this one, so the require will fail if this isn't done beforehand

brendankenny · 2018-07-17T22:00:22Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

+   * @return {null|string}
+   * @override
+   */
+  static getLastScreenshot(category) {


maybe we don't want this kind of api on a category renderer? Could it live in utils.js?

it could live in Util, sure. But it seems more specific to perf.

it could live in Util, sure. But it seems more specific to perf.

it is specific to perf, but it's also a utility method and doesn't really match the rest of the API (which is entirely construction and render() right now). It feels weird to mix DOM generation and helpful utility methods. And since it's static and not called from within performance-category-renderer.js, you have to know to call it with the perf category anyways. We also have other single-use methods in utils.js, like getEnvironmentDisplayValues or getEmulationDescriptions.

Though now that this is an audit, maybe we shouldn't have a method at all and the PSI renderer can just reach into the audit itself?

patrickhulce

lookin' good!

patrickhulce · 2018-07-18T18:05:18Z

lighthouse-core/audits/final-screenshot.js

+      throw new LHError(LHError.errors.NO_SCREENSHOTS);
+    }
+
+    // The timing isn't too important, so don't fail the audit if there's no navigationStart found


won't trace-of-tab just throw if there's no navstart though?

patrickhulce · 2018-07-18T18:22:21Z

lighthouse-core/test/report/html/renderer/category-renderer-test.js

-    const perfCategory = sampleResults.reportCategories.find(cat => cat.id === 'performance');
-    const categoryDOM2 = renderer.render(perfCategory, sampleResults.categoryGroups);
-    assert.ok(!categoryDOM2.querySelector('.lh-audit-group--manual'));
+    // Performance has its own category renderer


I'm still a bit confused why these test changes had to be done for the final screenshot audit to happen :)

patrickhulce · 2018-07-18T18:24:31Z

lighthouse-core/test/audits/final-screenshot-test.js

+    const results = await FinalScreenshotAudit.audit(artifacts);
+
+    assert.ok(results.rawValue);
+    assert.equal(results.details.items[0].timing, 818);


assert ok on the screenshot? since that's what we actually care about :)

patrickhulce · 2018-07-18T18:24:38Z

lighthouse-core/test/audits/final-screenshot-test.js

@@ -0,0 +1,34 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


brendankenny · 2018-07-18T19:07:54Z

lighthouse-core/audits/final-screenshot.js

+   */
+  static async audit(artifacts) {
+    const trace = artifacts.traces[Audit.DEFAULT_PASS];
+    const traceOfTab = await artifacts.requestTraceOfTab(trace);


can also use the screenshots computed artifact and take the last one (it's already prefaced by data:image/jpg;base64,, but that doesn't seem too bad)

ho shit. that's much better. totally forgot. thank you

brendankenny · 2018-07-18T19:11:23Z

lighthouse-core/audits/final-screenshot.js

+    return {
+      rawValue: true,
+      details: {
+        type: 'screenshot',


it looks like this is type filmstrip. If we're going to make a new type that's screenshot (singular), probably no reason to use items, could flatten and do

details: { type: 'screenshot', timestamp: number, timing: number, data: string }

brendankenny · 2018-07-18T19:12:32Z

lighthouse-core/audits/final-screenshot.js

@@ -0,0 +1,63 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


brendankenny · 2018-07-18T19:13:38Z

lighthouse-core/test/audits/final-screenshot-test.js

+    assert.ok(results.rawValue);
+    assert.equal(results.details.items[0].timing, 818);
+    assert.equal(results.details.items[0].timestamp, 225414990064);
+  }, 10000);


need the adjusted timeout?

brendankenny · 2018-07-18T19:19:18Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

+   * @return {null|string}
+   * @override
+   */
+  static getLastScreenshot(category) {


it could live in Util, sure. But it seems more specific to perf.

it is specific to perf, but it's also a utility method and doesn't really match the rest of the API (which is entirely construction and render() right now). It feels weird to mix DOM generation and helpful utility methods. And since it's static and not called from within performance-category-renderer.js, you have to know to call it with the perf category anyways. We also have other single-use methods in utils.js, like getEnvironmentDisplayValues or getEmulationDescriptions.

Though now that this is an audit, maybe we shouldn't have a method at all and the PSI renderer can just reach into the audit itself?

old!

brendankenny

test additions, but otherwise LGTM!

brendankenny · 2018-07-31T00:37:09Z

lighthouse-core/test/report/html/renderer/util-test.js

@@ -142,4 +146,23 @@ describe('util helpers', () => {
    Util.formatDisplayValue(displayValue);
    assert.deepStrictEqual(displayValue, cloned, 'displayValue was mutated');
  });
+
+  describe('getFinalScreenshot', () => {
+    sampleResults.reportCategories = Object.values(sampleResults.categories);


should stringify/parse this to not mutate the module

brendankenny · 2018-07-31T00:41:17Z

lighthouse-core/test/report/html/renderer/util-test.js

+    });
+
+    it('returns null if there are no screenshots', () => {
+      const fakeCategory = Object.assign({}, category, {auditRefs: []});


can you pass in the pwa category or cancel out just the final screenshot audit (or both?)? It's hard to tell which error case this is hitting since the category shape is so smooshed up by this point :)

brendankenny · 2018-07-31T00:41:51Z

lighthouse-core/lib/asset-saver.js

@@ -172,7 +172,6 @@ async function prepareAssets(artifacts, audits) {
    const Runner = require('../runner.js');
    const computedArtifacts = Runner.instantiateComputedArtifacts();
    /** @type {Array<Screenshot>} */
-    // @ts-ignore TODO(bckenny): need typed computed artifacts


brendankenny · 2018-07-31T00:44:30Z

lighthouse-core/test/audits/final-screenshot-test.js

+    const results = await FinalScreenshotAudit.audit(artifacts);
+
+    assert.ok(results.rawValue);
+    assert.equal(results.details.timestamp, 225414990.064);


assert.ok(results.details.data.startsWith('data:image/jpeg;base64,whateverthestartis'));?

brendankenny · 2018-07-31T00:46:34Z

CategoryRenderer › renders manual audits if the category contains them is definitely a weird test to be failing on, at least based on the description :)

Does it need to be set as notApplicable or something to not go through that loop?

patrickhulce reviewed Jul 17, 2018

View reviewed changes

paulirish added the 3.0 label Jul 17, 2018

brendankenny reviewed Jul 17, 2018

View reviewed changes

patrickhulce previously requested changes Jul 18, 2018

View reviewed changes

brendankenny reviewed Jul 18, 2018

View reviewed changes

paulirish added 5 commits July 30, 2018 16:41

report: add helper method for getting final screenshot datauri

8c8c82c

introduce a new 'final-screenshot' audit

52a14fe

add tests.

d9a614e

feedback.

5d80a88

feedback.

d778f8b

paulirish force-pushed the getlastss branch from 1b83e47 to d778f8b Compare July 31, 2018 00:04

paulirish changed the title ~~report: add helper method for getting final screenshot datauri~~ report: add method to get the final screenshot Jul 31, 2018

move method to util

5d175b7

paulirish force-pushed the getlastss branch from bbb4c8b to 5d175b7 Compare July 31, 2018 00:18

screenshot details type

6c5a5a6

brendankenny approved these changes Jul 31, 2018

View reviewed changes

fixup tests.

05910ce

paulirish merged commit f16b67e into master Aug 1, 2018

paulirish deleted the getlastss branch August 1, 2018 00:20

paulirish removed 3.0 labels Dec 18, 2018

patrickhulce mentioned this pull request Jan 25, 2019

(minor) Inconsistent format of screenshot-thumbnails and final-screenshot #7092

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: add method to get the final screenshot #5673

report: add method to get the final screenshot #5673

paulirish commented Jul 17, 2018 •

edited

Loading

patrickhulce Jul 17, 2018

paulirish Jul 17, 2018

brendankenny Jul 17, 2018

brendankenny Jul 17, 2018

paulirish Jul 17, 2018

brendankenny Jul 17, 2018

paulirish Jul 17, 2018

brendankenny Jul 18, 2018

patrickhulce left a comment

patrickhulce Jul 18, 2018

patrickhulce Jul 18, 2018

patrickhulce Jul 18, 2018

patrickhulce Jul 18, 2018

brendankenny Jul 18, 2018

paulirish Jul 19, 2018

brendankenny Jul 18, 2018

brendankenny Jul 18, 2018

brendankenny Jul 18, 2018

brendankenny Jul 18, 2018

brendankenny left a comment

brendankenny Jul 31, 2018

brendankenny Jul 31, 2018

brendankenny Jul 31, 2018

brendankenny Jul 31, 2018

brendankenny commented Jul 31, 2018

		@@ -0,0 +1,34 @@
		/**
		* @license Copyright 2017 Google Inc. All Rights Reserved.

		@@ -0,0 +1,63 @@
		/**
		* @license Copyright 2017 Google Inc. All Rights Reserved.

report: add method to get the final screenshot #5673

report: add method to get the final screenshot #5673

Conversation

paulirish commented Jul 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

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

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

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

brendankenny commented Jul 31, 2018

paulirish commented Jul 17, 2018 •

edited

Loading