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: expose error stack on errored audits #14491

Merged
merged 10 commits into from
May 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
7 changes: 5 additions & 2 deletions core/audits/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -348,12 +348,14 @@ class Audit {
/**
* @param {typeof Audit} audit
* @param {string | LH.IcuMessage} errorMessage
* @param {string=} errorStack
* @return {LH.RawIcu<LH.Audit.Result>}
*/
static generateErrorAuditResult(audit, errorMessage) {
static generateErrorAuditResult(audit, errorMessage, errorStack) {
return Audit.generateAuditResult(audit, {
score: null,
errorMessage,
errorStack,
});
}

Expand All @@ -371,7 +373,7 @@ class Audit {
let scoreDisplayMode = audit.meta.scoreDisplayMode || Audit.SCORING_MODES.BINARY;

// But override if product contents require it.
if (product.errorMessage) {
if (product.errorMessage !== undefined) {
// Error result.
scoreDisplayMode = Audit.SCORING_MODES.ERROR;
} else if (product.notApplicable) {
Expand Down Expand Up @@ -407,6 +409,7 @@ class Audit {
displayValue: product.displayValue,
explanation: product.explanation,
errorMessage: product.errorMessage,
errorStack: product.errorStack,
warnings: product.warnings,

details: product.details,
Expand Down
18 changes: 13 additions & 5 deletions core/gather/driver/execution-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,22 @@ class ExecutionContext {

this._session.setNextProtocolTimeout(timeout);
const response = await this._session.sendCommand('Runtime.evaluate', evaluationParams);
if (response.exceptionDetails) {

const ex = response.exceptionDetails;
if (ex) {
// An error occurred before we could even create a Promise, should be *very* rare.
// Also occurs when the expression is not valid JavaScript.
const errorMessage = response.exceptionDetails.exception ?
response.exceptionDetails.exception.description :
response.exceptionDetails.text;
return Promise.reject(new Error(`Evaluation exception: ${errorMessage}`));
const elidedExpression = expression.replace(/\s+/g, ' ').substring(0, 100);
const messageLines = [
'Runtime.evaluate exception',
`Expression: ${elidedExpression}\n---- (elided)`,
!ex.stackTrace ? `Parse error at: ${ex.lineNumber + 1}:${ex.columnNumber + 1}` : null,
ex.exception?.description || ex.text,
].filter(Boolean);
const evaluationError = new Error(messageLines.join('\n'));
return Promise.reject(evaluationError);
}

// Protocol should always return a 'result' object, but it is sometimes undefined. See #6026.
if (response.result === undefined) {
return Promise.reject(
Expand Down
2 changes: 1 addition & 1 deletion core/legacy/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ class LegacyResolvedConfig {
}

/**
* @deprecated `Config.fromJson` should be used instead.
* @deprecated `LegacyResolvedConfig.fromJson` should be used instead.
* @constructor
* @param {LH.Config} config
* @param {{settings: LH.Config.Settings, passes: ?LH.Config.Pass[], audits: ?LH.Config.AuditDefn[]}} opts
Expand Down
19 changes: 19 additions & 0 deletions core/lib/asset-saver.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import fs from 'fs';
import path from 'path';
import stream from 'stream';
import url from 'url';

import log from 'lighthouse-logger';

Expand All @@ -16,6 +17,7 @@ import {MetricTraceEvents} from './traces/metric-trace-events.js';
import {NetworkAnalysis} from '../computed/network-analysis.js';
import {LoadSimulator} from '../computed/load-simulator.js';
import {LighthouseError} from '../lib/lh-error.js';
import {LH_ROOT} from '../../root.js';

const optionsFilename = 'options.json';
const artifactsFilename = 'artifacts.json';
Expand Down Expand Up @@ -425,6 +427,22 @@ function normalizeTimingEntries(timings) {
}
}

/**
* @param {LH.Result} lhr
*/
function elideAuditErrorStacks(lhr) {
const baseCallFrameUrl = url.pathToFileURL(LH_ROOT);
for (const auditResult of Object.values(lhr.audits)) {
if (auditResult.errorStack) {
auditResult.errorStack = auditResult.errorStack
// Make paths relative to the repo root.
.replaceAll(baseCallFrameUrl.pathname, '')
// Remove line/col info.
.replaceAll(/:\d+:\d+/g, '');
}
}
}

export {
saveArtifacts,
saveFlowArtifacts,
Expand All @@ -438,4 +456,5 @@ export {
saveLanternNetworkData,
stringifyReplacer,
normalizeTimingEntries,
elideAuditErrorStacks,
};
26 changes: 15 additions & 11 deletions core/lib/lh-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,17 +107,18 @@ const str_ = i18n.createIcuMessageFn(import.meta.url, UIStrings);
const LHERROR_SENTINEL = '__LighthouseErrorSentinel';
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
* @typedef {{sentinel: '__LighthouseErrorSentinel', code: string, stack?: string, cause?: unknown, properties?: {[p: string]: string|undefined}}} SerializedLighthouseError
* @typedef {{sentinel: '__ErrorSentinel', message: string, code?: string, stack?: string, cause?: unknown}} SerializedBaseError
*/

class LighthouseError extends Error {
/**
* @param {LighthouseErrorDefinition} errorDefinition
* @param {Record<string, string|undefined>=} properties
* @param {ErrorOptions=} options
*/
constructor(errorDefinition, properties) {
super(errorDefinition.code);
constructor(errorDefinition, properties, options) {
super(errorDefinition.code, options);
this.name = 'LighthouseError';
this.code = errorDefinition.code;
// Add additional properties to be ICU replacements in the error string.
Expand Down Expand Up @@ -163,26 +164,28 @@ class LighthouseError extends Error {
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;
const {name, code, message, friendlyMessage, lhrRuntimeError, stack, cause, ...properties} = err;

return {
sentinel: LHERROR_SENTINEL,
code,
stack,
...properties,
cause,
properties: /** @type {{ [p: string]: string | undefined }} */ (properties),
};
}

// Unexpected errors won't be LighthouseErrors, but we want them serialized as well.
if (err instanceof Error) {
const {message, stack} = err;
const {message, stack, cause} = err;
// @ts-expect-error - 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,
cause,
};
}

Expand All @@ -203,17 +206,18 @@ class LighthouseError extends Error {
if (possibleError.sentinel === LHERROR_SENTINEL) {
// Include sentinel in destructuring so it doesn't end up in `properties`.
// eslint-disable-next-line no-unused-vars
const {sentinel, code, stack, ...properties} = /** @type {SerializedLighthouseError} */ (possibleError);
const {code, stack, cause, properties} = /** @type {SerializedLighthouseError} */ (possibleError);
const errorDefinition = LighthouseError.errors[/** @type {keyof typeof ERRORS} */ (code)];
const lhError = new LighthouseError(errorDefinition, properties);
const lhError = new LighthouseError(errorDefinition, properties, {cause});
lhError.stack = stack;

return lhError;
}

if (possibleError.sentinel === ERROR_SENTINEL) {
const {message, code, stack} = /** @type {SerializedBaseError} */ (possibleError);
const error = new Error(message);
const {message, code, stack, cause} = /** @type {SerializedBaseError} */ (possibleError);
const opts = cause ? {cause} : undefined;
const error = new Error(message, opts);
Object.assign(error, {code, stack});
return error;
}
Expand Down
6 changes: 4 additions & 2 deletions core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ class Runner {

// Create a friendlier display error and mark it as expected to avoid duplicates in Sentry
const error = new LighthouseError(LighthouseError.errors.ERRORED_REQUIRED_ARTIFACT,
{artifactName, errorMessage: artifactError.message});
{artifactName, errorMessage: artifactError.message}, {cause: artifactError});
// @ts-expect-error Non-standard property added to Error
error.expected = true;
throw error;
Expand Down Expand Up @@ -468,7 +468,9 @@ class Runner {
Sentry.captureException(err, {tags: {audit: audit.meta.id}, level: 'error'});
// Errors become error audit result.
const errorMessage = err.friendlyMessage ? err.friendlyMessage : err.message;
auditResult = Audit.generateErrorAuditResult(audit, errorMessage);
// Prefer the stack trace closest to the error.
const stack = err.cause?.stack ?? err.stack;
auditResult = Audit.generateErrorAuditResult(audit, errorMessage, stack);
}

log.timeEnd(status);
Expand Down
5 changes: 5 additions & 0 deletions core/scripts/cleanup-LHR-for-diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

import {readFileSync, writeFileSync} from 'fs';

import {elideAuditErrorStacks} from '../lib/asset-saver.js';

const filename = process.argv[2];
const extraFlag = process.argv[3];
if (!filename) throw new Error('No filename provided.');
Expand Down Expand Up @@ -45,6 +47,9 @@ function cleanAndFormatLHR(lhrString) {
auditResult.description = '**Excluded from diff**';
}
}

elideAuditErrorStacks(lhr);

// Ensure we have a final newline to conform to .editorconfig
return `${JSON.stringify(lhr, null, 2)}\n`;
}
1 change: 1 addition & 0 deletions core/scripts/update-flow-fixtures.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ async function generateFlowResult() {
// Normalize some data so it doesn't change on every update.
for (const {lhr} of flowResult.steps) {
assetSaver.normalizeTimingEntries(lhr.timing.entries);
assetSaver.elideAuditErrorStacks(lhr);
lhr.timing.total = lhr.timing.entries.length;
}

Expand Down
31 changes: 31 additions & 0 deletions core/test/gather/driver/execution-context-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,37 @@ describe('.evaluateAsync', () => {
const value = await executionContext.evaluateAsync('"magic"', {useIsolation: true});
expect(value).toEqual('mocked value');
});

it('handles runtime evaluation exception', async () => {
/** @type {LH.Crdp.Runtime.ExceptionDetails} */
const exceptionDetails = {
exceptionId: 1,
text: 'Uncaught',
lineNumber: 7,
columnNumber: 8,
stackTrace: {description: '', callFrames: []},
exception: {
type: 'object',
subtype: 'error',
className: 'ReferenceError',
description: 'ReferenceError: Prosmise is not defined\n' +
' at wrapInNativePromise (_lighthouse-eval.js:8:9)\n' +
' at _lighthouse-eval.js:83:8',
},
};
sessionMock.sendCommand = createMockSendCommandFn()
.mockResponse('Page.enable')
.mockResponse('Runtime.enable')
.mockResponse('Page.getResourceTree', {frameTree: {frame: {id: '1337'}}})
.mockResponse('Page.getFrameTree', {frameTree: {frame: {id: '1337'}}})
.mockResponse('Page.createIsolatedWorld', {executionContextId: 9001})
.mockResponse('Runtime.evaluate', {exceptionDetails});

const promise = executionContext.evaluateAsync('new Prosmise', {useIsolation: true});
await expect(promise).rejects.toThrow(/Expression: new Prosmise/);
await expect(promise).rejects.toThrow(/elided/);
await expect(promise).rejects.toThrow(/at wrapInNativePromise/);
});
});

describe('.evaluate', () => {
Expand Down
31 changes: 30 additions & 1 deletion core/test/lib/asset-saver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,9 @@ describe('asset-saver helper', () => {
// Use an LighthouseError that has an ICU replacement.
const protocolMethod = 'Page.getFastness';
const lhError = new LighthouseError(
LighthouseError.errors.PROTOCOL_TIMEOUT, {protocolMethod});
LighthouseError.errors.PROTOCOL_TIMEOUT,
{protocolMethod},
{cause: new Error('the cause')});

const artifacts = {
traces: {},
Expand All @@ -385,6 +387,8 @@ describe('asset-saver helper', () => {
expect(roundTripArtifacts.ScriptElements).toBeInstanceOf(LighthouseError);
expect(roundTripArtifacts.ScriptElements.code).toEqual('PROTOCOL_TIMEOUT');
expect(roundTripArtifacts.ScriptElements.protocolMethod).toEqual(protocolMethod);
expect(roundTripArtifacts.ScriptElements.cause).toBeInstanceOf(Error);
expect(roundTripArtifacts.ScriptElements.cause.message).toEqual('the cause');
expect(roundTripArtifacts.ScriptElements.stack).toMatch(
/^LighthouseError: PROTOCOL_TIMEOUT.*test[\\/]lib[\\/]asset-saver-test\.js/s);
expect(roundTripArtifacts.ScriptElements.friendlyMessage)
Expand Down Expand Up @@ -439,4 +443,29 @@ describe('asset-saver helper', () => {
});
});
});

describe('elideAuditErrorStacks', () => {
it('elides correctly', async () => {
const lhr = JSON.parse(JSON.stringify(dbwResults));
lhr.audits['bf-cache'].errorStack = `Error: LighthouseError: ERRORED_REQUIRED_ARTIFACT
at Runner._runAudit (${LH_ROOT}/core/runner.js:431:25)
at Runner._runAudits (${LH_ROOT}/core/runner.js:370:40)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async Runner.audit (${LH_ROOT}/core/runner.js:62:32)
at async runLighthouse (${LH_ROOT}/cli/run.js:250:8)
at async ${LH_ROOT}/cli/index.js:10:1
at <anonymous>:1:1`;
assetSaver.elideAuditErrorStacks(lhr);

// eslint-disable-next-line max-len
expect(lhr.audits['bf-cache'].errorStack).toEqual(`Error: LighthouseError: ERRORED_REQUIRED_ARTIFACT
at Runner._runAudit (/core/runner.js)
at Runner._runAudits (/core/runner.js)
at process.processTicksAndRejections (node:internal/process/task_queues)
at async Runner.audit (/core/runner.js)
at async runLighthouse (/cli/run.js)
at async /cli/index.js
at <anonymous>`);
});
});
});
31 changes: 31 additions & 0 deletions core/test/runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,37 @@ describe('Runner', () => {
assert.strictEqual(auditResult.score, null);
assert.strictEqual(auditResult.scoreDisplayMode, 'error');
assert.ok(auditResult.errorMessage.includes(errorMessage));
assert.ok(auditResult.errorStack.match(/at [a-zA-Z]*.audit/));
});
});

it('produces an error audit result that prefers cause stack', async () => {
const errorMessage = 'Audit yourself';
const resolvedConfig = await LegacyResolvedConfig.fromJson({
settings: {
auditMode: moduleDir + '/fixtures/artifacts/empty-artifacts/',
},
audits: [
class ThrowyAudit extends Audit {
static get meta() {
return testAuditMeta;
}
static audit() {
this.aFn();
}
static aFn() {
throw new Error(errorMessage);
}
},
],
});

return runGatherAndAudit({}, {resolvedConfig}).then(results => {
const auditResult = results.lhr.audits['throwy-audit'];
assert.strictEqual(auditResult.score, null);
assert.strictEqual(auditResult.scoreDisplayMode, 'error');
assert.ok(auditResult.errorMessage.includes(errorMessage));
assert.ok(auditResult.errorStack.match(/at [a-zA-Z]*.aFn/));
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@
"rollup-plugin-terser": "^7.0.2",
"tabulator-tables": "^4.9.3",
"terser": "^5.3.8",
"testdouble": "^3.16.8",
"testdouble": "^3.17.2",
"typed-query-selector": "^2.6.1",
"typescript": "^5.0.4",
"wait-for-expect": "^3.0.2",
Expand Down
2 changes: 1 addition & 1 deletion tsconfig-base.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"outDir": ".tmp/tsbuildinfo/",
"rootDir": ".",

"target": "es2020",
"target": "es2022",
"module": "es2022",
"moduleResolution": "node",
"esModuleInterop": true,
Expand Down
2 changes: 2 additions & 0 deletions types/audit.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ declare module Audit {
explanation?: string | IcuMessage;
/** Error message from any exception thrown while running this audit. */
errorMessage?: string | IcuMessage;
/** Error stack from any exception thrown while running this audit. */
errorStack?: string;
warnings?: Array<string | IcuMessage>;
/** Overrides scoreDisplayMode with notApplicable if set to true */
notApplicable?: boolean;
Expand Down
Loading