-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Changes from all commits
be5c3cb
e1968bd
977f8a1
3ce20d2
12e460f
73c79c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -56,10 +56,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'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this an occasion to use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added |
||||||
*/ | ||||||
constructor(errorDefinition, properties) { | ||||||
super(errorDefinition.code); | ||||||
|
@@ -96,6 +103,77 @@ 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. | ||||||
* @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#The_replacer_parameter | ||||||
* @param {Error|LighthouseError} err | ||||||
* @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, | ||||||
}; | ||||||
} | ||||||
|
||||||
// Unexpected errors won't be LHErrors, but we want them serialized as well. | ||||||
if (err instanceof Error) { | ||||||
const {message, stack} = err; | ||||||
// @ts-ignore - code can be helpful for e.g. node errors, so preserve it if it's present. | ||||||
const code = err.code; | ||||||
return { | ||||||
sentinel: ERROR_SENTINEL, | ||||||
message, | ||||||
code, | ||||||
stack, | ||||||
}; | ||||||
} | ||||||
|
||||||
throw new Error('Invalid value for LHError stringification'); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this could pass through like |
||||||
} | ||||||
|
||||||
/** | ||||||
* A JSON.parse reviver. If any value passed in is a serialized Error or | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. | ||||||
* @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#Using_the_reviver_parameter | ||||||
* @param {string} key | ||||||
* @param {any} possibleError | ||||||
* @return {any} | ||||||
*/ | ||||||
static parseReviver(key, possibleError) { | ||||||
if (typeof possibleError === 'object' && possibleError !== null) { | ||||||
if (possibleError.sentinel === LHERROR_SENTINEL) { | ||||||
// Include sentinel in destructuring so it doesn't end up in `properties`. | ||||||
// eslint-disable-next-line no-unused-vars | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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 = { | ||||||
|
@@ -227,7 +305,7 @@ const ERRORS = { | |||||
}, | ||||||
|
||||||
/* Protocol timeout failures | ||||||
* Requires an additional `icuProtocolMethod` field for translation. | ||||||
* Requires an additional `protocolMethod` field for translation. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be done in some sort of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
everything is sync before it, so it seemed ok. Are there tricky jest things to worry about or anything? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 () => { | ||
|
There was a problem hiding this comment.
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! 😮