Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core(computed): fix new computed artifact interface #6151

Merged
merged 1 commit into from
Oct 2, 2018
Merged

core(computed): fix new computed artifact interface #6151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lighthouse-core/audits/final-screenshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
/**
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/manifest-short-name-length.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/splash-screen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/themed-omnibox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/webapp-install-banner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
15 changes: 8 additions & 7 deletions lighthouse-core/gather/computed/new-computed-artifact.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
12 changes: 4 additions & 8 deletions lighthouse-core/gather/computed/screenshots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand All @@ -30,4 +26,4 @@ class ScreenshotFilmstrip extends ComputedArtifact {
}
}

module.exports = ScreenshotFilmstrip;
module.exports = makeComputedArtifact(Screenshots);
1 change: 1 addition & 0 deletions lighthouse-core/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ class Runner {
// Computed artifacts switching to the new system.
'new-computed-artifact.js',
'manifest-values.js',
'screenshots.js',
];

const fileList = [
Expand Down
11 changes: 2 additions & 9 deletions lighthouse-core/test/audits/final-screenshot-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
26 changes: 13 additions & 13 deletions lighthouse-core/test/gather/computed/manifest-values-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,30 +35,30 @@ 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);
});

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);
});

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

Expand All @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the naming change from manifestArtifact to Manifest halfway through? Are they different later than in the beginning?

Copy link
Member Author

@brendankenny brendankenny Oct 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are they different later than in the beginning?

no, just different original authors/written at different times in the code base, I assume

const colorResults = results.allChecks.filter(i => i.id.includes('Color'));
assert.equal(colorResults.every(i => i.passing === false), true);
});
Expand All @@ -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);
});
Expand All @@ -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);
});
Expand Down Expand Up @@ -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);
});
Expand All @@ -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);
});
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
16 changes: 4 additions & 12 deletions lighthouse-core/test/gather/computed/screenshots-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
1 change: 0 additions & 1 deletion typings/artifacts.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down