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.

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

core: support saving and loading error artifacts #9397

Merged
merged 6 commits into from
Jul 20, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
25 changes: 21 additions & 4 deletions lighthouse-core/lib/asset-saver.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const rimraf = require('rimraf');
const mkdirp = require('mkdirp');
const NetworkAnalysisComputed = require('../computed/network-analysis.js');
const LoadSimulatorComputed = require('../computed/load-simulator.js');
const LHError = require('../lib/lh-error.js');

const artifactsFilename = 'artifacts.json';
const traceSuffix = '.trace.json';
Expand All @@ -42,9 +43,10 @@ function loadArtifacts(basePath) {
throw new Error('No saved artifacts found at ' + basePath);
}

// load artifacts.json
// load artifacts.json using a reviver to deserialize any LHErrors in artifacts.
const artifactsStr = fs.readFileSync(path.join(basePath, artifactsFilename), 'utf8');
/** @type {LH.Artifacts} */
const artifacts = JSON.parse(fs.readFileSync(path.join(basePath, artifactsFilename), 'utf8'));
const artifacts = JSON.parse(artifactsStr, LHError.parseReviver);

const filenames = fs.readdirSync(basePath);

Expand Down Expand Up @@ -73,6 +75,21 @@ function loadArtifacts(basePath) {
return artifacts;
}

/**
* A replacer function for JSON.stingify of the artifacts. Used to serialize objects that
* JSON won't normally handle.
* @param {string} key
* @param {any} value
*/
function stringifyReplacer(key, value) {
// Currently only handle LHError and other Error types.
if (value instanceof Error) {
return LHError.stringifyReplacer(value);
}

return value;
}

/**
* Save artifacts object mostly to single file located at basePath/artifacts.log.
* Also save the traces & devtoolsLogs to their own files
Expand Down Expand Up @@ -100,8 +117,8 @@ async function saveArtifacts(artifacts, basePath) {
fs.writeFileSync(`${basePath}/${passName}${devtoolsLogSuffix}`, log, 'utf8');
}

// save everything else
const restArtifactsString = JSON.stringify(restArtifacts, null, 2);
// save everything else, using a replacer to serialize LHErrors in the artifacts.
const restArtifactsString = JSON.stringify(restArtifacts, stringifyReplacer, 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

a wild non-null usage in stringify appears! 😮

fs.writeFileSync(`${basePath}/${artifactsFilename}`, restArtifactsString, 'utf8');
log.log('Artifacts saved to disk in folder:', basePath);
log.timeEnd(status);
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-core/lib/i18n/locales/en-XL.json
Original file line number Diff line number Diff line change
Expand Up @@ -1253,9 +1253,15 @@
"lighthouse-core/lib/lh-error.js | dnsFailure": {
"message": "D̂ŃŜ śêŕv̂ér̂ś ĉóûĺd̂ ńôt́ r̂éŝól̂v́ê t́ĥé p̂ŕôv́îd́êd́ d̂óm̂áîń."
},
"lighthouse-core/lib/lh-error.js | erroredRequiredArtifact": {
"message": "R̂éq̂úîŕêd́ {artifactName} ĝát̂h́êŕêŕ êńĉóûńt̂ér̂éd̂ án̂ ér̂ŕôŕ: {errorMessage}"
},
"lighthouse-core/lib/lh-error.js | internalChromeError": {
"message": "Âń îńt̂ér̂ńâĺ Ĉh́r̂óm̂é êŕr̂ór̂ óĉćûŕr̂éd̂. Ṕl̂éâśê ŕêśt̂ár̂t́ Ĉh́r̂óm̂é âńd̂ t́r̂ý r̂é-r̂ún̂ńîńĝ Ĺîǵĥt́ĥóûśê."
},
"lighthouse-core/lib/lh-error.js | missingRequiredArtifact": {
"message": "R̂éq̂úîŕêd́ {artifactName} ĝát̂h́êŕêŕ d̂íd̂ ńôt́ r̂ún̂."
},
"lighthouse-core/lib/lh-error.js | pageLoadFailed": {
"message": "L̂íĝh́t̂h́ôúŝé ŵáŝ ún̂áb̂ĺê t́ô ŕêĺîáb̂ĺŷ ĺôád̂ t́ĥé p̂áĝé ŷóû ŕêq́ûéŝt́êd́. M̂ák̂é ŝúr̂é ŷóû ár̂é t̂éŝt́îńĝ t́ĥé ĉór̂ŕêćt̂ ÚR̂Ĺ âńd̂ t́ĥát̂ t́ĥé ŝér̂v́êŕ îś p̂ŕôṕêŕl̂ý r̂éŝṕôńd̂ín̂ǵ t̂ó âĺl̂ ŕêq́ûéŝt́ŝ."
},
Expand Down
80 changes: 77 additions & 3 deletions lighthouse-core/lib/lh-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ const UIStrings = {

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);


/**
* @typedef LighthouseErrorDefinition
* @property {string} code
Expand All @@ -56,10 +55,17 @@ const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
* @property {boolean} [lhrRuntimeError] True if it should appear in the top-level LHR.runtimeError property.
*/

const LHERROR_SENTINEL = '__LighthouseErrorSentinel';
Copy link
Member

Choose a reason for hiding this comment

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

is this an occasion to use Symbol()??

Copy link
Member

Choose a reason for hiding this comment

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

ah damn it. nope. won't be the same across separate executions.

const ERROR_SENTINEL = '__ErrorSentinel';
/**
* @typedef {{sentinel: '__LighthouseErrorSentinel', code: string, stack?: string, [p: string]: string|undefined}} SerializedLighthouseError
* @typedef {{sentinel: '__ErrorSentinel', message: string, code?: string, stack?: string}} SerializedBaseError
*/

class LighthouseError extends Error {
/**
* @param {LighthouseErrorDefinition} errorDefinition
* @param {Record<string, string|boolean|undefined>=} properties
* @param {Record<string, string|undefined>=} properties
Copy link
Member Author

Choose a reason for hiding this comment

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

I added boolean here in #6014 but have no memory of why, and it doesn't appear to do anything in that PR or in the current codebase, so I'll assume it was from an early edit of that PR and it slipped through at the end unnoticed :)

*/
constructor(errorDefinition, properties) {
super(errorDefinition.code);
Expand Down Expand Up @@ -96,6 +102,74 @@ class LighthouseError extends Error {
const error = new Error(`Protocol error ${errMsg}`);
return Object.assign(error, {protocolMethod: method, protocolError: protocolError.message});
}

/**
* A JSON.stringify replacer to serialize LHErrors and (as a fallback) Errors.
* Returns a simplified version of the error object that can be reconstituted
* as a copy of the original error at parse time.
* @param {unknown} err
Copy link
Member

Choose a reason for hiding this comment

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

why not a Error|LighthouseError ? curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

why not a Error|LighthouseError ? curious.

that's probably better. I was originally writing it more generalized but it wasn't helpful at all

* @return {SerializedBaseError | SerializedLighthouseError}
*/
static stringifyReplacer(err) {
if (err instanceof LighthouseError) {
// Remove class props so that remaining values were what was passed in as `properties`.
// eslint-disable-next-line no-unused-vars
const {name, code, message, friendlyMessage, lhrRuntimeError, stack, ...properties} = err;

return {
sentinel: LHERROR_SENTINEL,
code,
stack,
...properties,
};
}

// We still have some errors that haven't moved to be LHErrors, so serialize them as well.
Copy link
Collaborator

Choose a reason for hiding this comment

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

any error that happens unexpectedly will always be a non-LHError, so it's a condition that's likely to exist forever :)

Suggested change
// We still have some errors that haven't moved to be LHErrors, so serialize them as well.
// Unexpected errors won't be `LHError`, but we still want to serialize them as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we could leave the line in if we want to also nudge folks to look into the still expected ones that aren't LHError yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

we could leave the line in if we want to also nudge folks to look into the still expected ones that aren't LHError yet?

no, I think you're right. We should have some nudges but probably not here :)

if (err instanceof Error) {
const {message, stack} = err;
// @ts-ignore - code can be helpful for e.g. node errors, so attempt to preserve it.
const code = err.code;
return {
sentinel: ERROR_SENTINEL,
message,
code,
stack,
};
}

throw new Error('Invalid value for LHError stringification');
Copy link
Member Author

Choose a reason for hiding this comment

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

this could pass through like parseReviver, but then we wouldn't have a foolproof way to have tsc type check the constructed error substitutes, so this seemed ok

}

/**
* A JSON.parse reviver. If any value passed in is a serialized Error or
Copy link
Collaborator

Choose a reason for hiding this comment

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

worth linking to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#Using_the_reviver_parameter
?

TIL that it visits all the leaf nodes first before visiting parent objects :)

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL that it visits all the leaf nodes first before visiting parent objects :)

yeah, I was a little worried about performance, but it doesn't seem like a big hit, and all the "optimizations" I tried to add fast paths seemed to have no net effect (so maybe the biggest hit is just the overhead from having any replacer/reviver at all)

* LHError, the error is recreated as the original object. Otherwise, the
* value is passed through unchanged.
* @param {string} key
* @param {any} possibleError
* @return {any}
*/
static parseReviver(key, possibleError) {
if (typeof possibleError === 'object' && possibleError !== null) {
if (possibleError.sentinel === LHERROR_SENTINEL) {
// eslint-disable-next-line no-unused-vars
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// eslint-disable-next-line no-unused-vars
// eslint-disable-next-line no-unused-vars - unpack sentinel just to avoid including it in `properties`

const {sentinel, code, stack, ...properties} = /** @type {SerializedLighthouseError} */ (possibleError);
const errorDefinition = LighthouseError.errors[/** @type {keyof typeof ERRORS} */ (code)];
const lhError = new LighthouseError(errorDefinition, properties);
lhError.stack = stack;

return lhError;
}

if (possibleError.sentinel === ERROR_SENTINEL) {
const {message, code, stack} = /** @type {SerializedBaseError} */ (possibleError);
const error = new Error(message);
Object.assign(error, {code, stack});
return error;
}
}

return possibleError;
}
}

const ERRORS = {
Expand Down Expand Up @@ -227,7 +301,7 @@ const ERRORS = {
},

/* Protocol timeout failures
* Requires an additional `icuProtocolMethod` field for translation.
* Requires an additional `protocolMethod` field for translation.
Copy link
Member Author

Choose a reason for hiding this comment

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

driveby, I assume changed at some point and missed

*/
PROTOCOL_TIMEOUT: {
code: 'PROTOCOL_TIMEOUT',
Expand Down
64 changes: 64 additions & 0 deletions lighthouse-core/test/lib/asset-saver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ const assetSaver = require('../../lib/asset-saver.js');
const Metrics = require('../../lib/traces/pwmetrics-events.js');
const assert = require('assert');
const fs = require('fs');
const rimraf = require('rimraf');
const LHError = require('../../lib/lh-error.js');

const traceEvents = require('../fixtures/traces/progressive-app.json');
const dbwTrace = require('../results/artifacts/defaultPass.trace.json');
Expand Down Expand Up @@ -167,6 +169,68 @@ describe('asset-saver helper', () => {
});
});

describe('JSON serialization', () => {
const outputPath = __dirname + '/json-serialization-test-data/';

afterEach(() => {
rimraf.sync(outputPath);
});

it('round trips saved artifacts', async () => {
const artifactsPath = __dirname + '/../results/artifacts/';
const originalArtifacts = await assetSaver.loadArtifacts(artifactsPath);

await assetSaver.saveArtifacts(originalArtifacts, outputPath);
const roundTripArtifacts = await assetSaver.loadArtifacts(outputPath);
expect(roundTripArtifacts).toStrictEqual(originalArtifacts);
});

it('round trips artifacts with an Error member', async () => {
const error = new Error('Connection refused by server');
// test code to make sure e.g. Node errors get serialized well.
error.code = 'ECONNREFUSED';

const artifacts = {
traces: {},
devtoolsLogs: {},
ViewportDimensions: error,
};

await assetSaver.saveArtifacts(artifacts, outputPath);
const roundTripArtifacts = await assetSaver.loadArtifacts(outputPath);
expect(roundTripArtifacts).toStrictEqual(artifacts);

expect(roundTripArtifacts.ViewportDimensions).toBeInstanceOf(Error);
expect(roundTripArtifacts.ViewportDimensions.code).toEqual('ECONNREFUSED');
expect(roundTripArtifacts.ViewportDimensions.stack).toMatch(
/^Error: Connection refused by server.*lighthouse-core\/test\/lib\/asset-saver-test.js/s);
});

it('round trips artifacts with an LHError member', async () => {
// Use an LHError that has an ICU replacement.
const protocolMethod = 'Page.getFastness';
const lhError = new LHError(LHError.errors.PROTOCOL_TIMEOUT, {protocolMethod});

const artifacts = {
traces: {},
devtoolsLogs: {},
ScriptElements: lhError,
};

await assetSaver.saveArtifacts(artifacts, outputPath);
const roundTripArtifacts = await assetSaver.loadArtifacts(outputPath);
expect(roundTripArtifacts).toStrictEqual(artifacts);

expect(roundTripArtifacts.ScriptElements).toBeInstanceOf(LHError);
expect(roundTripArtifacts.ScriptElements.code).toEqual('PROTOCOL_TIMEOUT');
expect(roundTripArtifacts.ScriptElements.protocolMethod).toEqual(protocolMethod);
expect(roundTripArtifacts.ScriptElements.stack).toMatch(
/^LHError: PROTOCOL_TIMEOUT.*lighthouse-core\/test\/lib\/asset-saver-test.js/s);
expect(roundTripArtifacts.ScriptElements.friendlyMessage)
.toBeDisplayString(/\(Method: Page\.getFastness\)/);
});
});

describe('saveLanternNetworkData', () => {
const outputFilename = 'test-lantern-network-data.json';

Expand Down
46 changes: 27 additions & 19 deletions lighthouse-core/test/runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -330,32 +330,40 @@ describe('Runner', () => {
});
});

// TODO: need to support save/load of artifact errors.
// See https://github.com/GoogleChrome/lighthouse/issues/4984
it.skip('outputs an error audit result when required artifact was an Error', () => {
const errorMessage = 'blurst of times';
const artifactError = new Error(errorMessage);
it('outputs an error audit result when required artifact was an Error', async () => {
// Start with empty-artifacts.
const baseArtifacts = assetSaver.loadArtifacts(__dirname +
'/fixtures/artifacts/empty-artifacts/');

const url = 'https://example.com';
// Add error and save artifacts using assetSaver to serialize Error object.
const errorMessage = 'blurst of times';
const artifacts = {
...baseArtifacts,
ViewportDimensions: new Error(errorMessage),
TestedAsMobileDevice: true,
};
const artifactsPath = '.tmp/test_artifacts';
const resolvedPath = path.resolve(process.cwd(), artifactsPath);
await assetSaver.saveArtifacts(artifacts, resolvedPath);

// Load artifacts via auditMode.
const config = new Config({
settings: {
auditMode: resolvedPath,
},
audits: [
// requires ViewportDimensions and TestedAsMobileDevice artifacts
'content-width',
],

artifacts: {
// Error objects don't make it through the Config constructor due to
// JSON.stringify/parse step, so populate with test error below.
ViewportDimensions: null,
},
});
config.artifacts.ViewportDimensions = artifactError;

return Runner.run({}, {url, config}).then(results => {
const auditResult = results.lhr.audits['content-width'];
assert.strictEqual(auditResult.score, null);
assert.strictEqual(auditResult.scoreDisplayMode, 'error');
assert.ok(auditResult.errorMessage.includes(errorMessage));
});
const results = await Runner.run({}, {config});
const auditResult = results.lhr.audits['content-width'];
assert.strictEqual(auditResult.score, null);
assert.strictEqual(auditResult.scoreDisplayMode, 'error');
assert.ok(auditResult.errorMessage.includes(errorMessage));

rimraf.sync(resolvedPath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be done in some sort of after?

Copy link
Member Author

Choose a reason for hiding this comment

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

should this be done in some sort of after?

everything is sync before it, so it seemed ok. Are there tricky jest things to worry about or anything?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no I was just thinking if the assertions fail it won't be cleaned up, but this certainly won't be the only rough edge we have in that situation across the codebase

});

it('only passes the required artifacts to the audit', async () => {
Expand Down