From b7add1fcebbb3d61e3fb03fb159928e4b8d9d205 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 18 Oct 2018 18:48:30 -0700 Subject: [PATCH 1/4] core: remove recoverOrThrow / err.fatal (#6298) --- lighthouse-core/gather/gather-runner.js | 25 +++----------- lighthouse-core/gather/gatherers/gatherer.js | 5 ++- lighthouse-core/runner.js | 8 ++--- .../test/gather/gather-runner-test.js | 34 +------------------ lighthouse-core/test/runner-test.js | 29 ++-------------- 5 files changed, 12 insertions(+), 89 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 2d045fcf966e..a259e6a8a2fd 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -130,20 +130,6 @@ class GatherRunner { }); } - /** - * Test any error output from the promise, absorbing non-fatal errors and - * throwing on fatal ones so that run is stopped. - * @param {Promise<*>} promise - * @return {Promise} - */ - static recoverOrThrow(promise) { - return promise.catch(err => { - if (err.fatal) { - throw err; - } - }); - } - /** * Returns an error if the original network request failed or wasn't found. * @param {string} url The URL of the original requested page. @@ -197,7 +183,7 @@ class GatherRunner { passContext.options = gathererDefn.options || {}; const artifactPromise = Promise.resolve().then(_ => gatherer.beforePass(passContext)); gathererResults[gatherer.name] = [artifactPromise]; - await GatherRunner.recoverOrThrow(artifactPromise); + await artifactPromise; } } @@ -241,7 +227,7 @@ class GatherRunner { const gathererResult = gathererResults[gatherer.name] || []; gathererResult.push(artifactPromise); gathererResults[gatherer.name] = gathererResult; - await GatherRunner.recoverOrThrow(artifactPromise); + await artifactPromise; } } @@ -308,7 +294,7 @@ class GatherRunner { const gathererResult = gathererResults[gatherer.name] || []; gathererResult.push(artifactPromise); gathererResults[gatherer.name] = gathererResult; - await GatherRunner.recoverOrThrow(artifactPromise); + await artifactPromise; log.verbose('statusEnd', status); } @@ -319,7 +305,7 @@ class GatherRunner { /** * Takes the results of each gatherer phase for each gatherer and uses the * last produced value (that's not undefined) as the artifact for that - * gatherer. If a non-fatal error was rejected from a gatherer phase, + * gatherer. If an error was rejected from a gatherer phase, * uses that error object as the artifact instead. * @param {Partial} gathererResults * @param {LH.BaseArtifacts} baseArtifacts @@ -341,8 +327,7 @@ class GatherRunner { // Typecast pretends artifact always provided here, but checked below for top-level `throw`. gathererArtifacts[gathererName] = /** @type {NonVoid} */ (artifact); } catch (err) { - // An error result must be non-fatal to not have caused an exit by now, - // so return it to runner to handle turning it into an error audit. + // return error runner to handle turning it into an error audit. gathererArtifacts[gathererName] = err; } diff --git a/lighthouse-core/gather/gatherers/gatherer.js b/lighthouse-core/gather/gatherers/gatherer.js index b2d71af50822..a76a25932ec5 100644 --- a/lighthouse-core/gather/gatherers/gatherer.js +++ b/lighthouse-core/gather/gatherers/gatherer.js @@ -13,9 +13,8 @@ * method. All methods can return the artifact value directly or return a * Promise that resolves to that value. * - * If an Error is thrown (or a Promise that rejects on an Error), the - * GatherRunner will check for a `fatal` property on the Error. If not set to - * `true`, the runner will treat it as an error internal to the gatherer and + * If an Error is thrown (or a Promise that rejects on an Error), + * the runner will treat it as an error internal to the gatherer and * continue execution of any remaining gatherers. */ class Gatherer { diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js index 3a752d1f85d8..08e16b07d53e 100644 --- a/lighthouse-core/runner.js +++ b/lighthouse-core/runner.js @@ -251,8 +251,7 @@ class Runner { throw new Error(`Required ${artifactName} gatherer did not run.`); } - // If artifact was an error, it must be non-fatal (or gatherRunner would - // have thrown). Output error result on behalf of audit. + // If artifact was an error, output error result on behalf of audit. if (artifacts[artifactName] instanceof Error) { /** @type {Error} */ // @ts-ignore An artifact *could* be an Error, but caught here, so ignore elsewhere. @@ -286,12 +285,9 @@ class Runner { auditResult = Audit.generateAuditResult(audit, product); } catch (err) { log.warn(audit.meta.id, `Caught exception: ${err.message}`); - if (err.fatal) { - throw err; - } Sentry.captureException(err, {tags: {audit: audit.meta.id}, level: 'error'}); - // Non-fatal error become error audit result. + // Errors become error audit result. const errorMessage = err.friendlyMessage ? `${err.friendlyMessage} (${err.message})` : `Audit error: ${err.message}`; diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 8ab6da53e655..fc7b91a48937 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -915,7 +915,7 @@ describe('GatherRunner', function() { }); }); - it('supports sync and async throwing of non-fatal errors from gatherers', () => { + it('supports sync and async throwing of errors from gatherers', () => { const gatherers = [ // sync new class BeforeSync extends Gatherer { @@ -974,38 +974,6 @@ describe('GatherRunner', function() { }); }); - it('rejects if a gatherer returns a fatal error', () => { - const errorMessage = 'Gather Failed in pass()'; - const err = new Error(errorMessage); - err.fatal = true; - const gatherers = [ - // sync - new class GathererSuccess extends Gatherer { - afterPass() { - return 1; - } - }(), - new class GathererFailure extends Gatherer { - pass() { - return Promise.reject(err); - } - }, - ].map(instance => ({instance})); - const passes = [{ - blankDuration: 0, - gatherers, - }]; - - return GatherRunner.run(passes, { - driver: fakeDriver, - requestedUrl: 'https://example.com', - settings: {}, - config: new Config({}), - }).then( - _ => assert.ok(false), - err => assert.strictEqual(err.message, errorMessage)); - }); - it('rejects if a gatherer does not provide an artifact', () => { const passes = [{ blankDuration: 0, diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js index ce8ad951ab78..97f753e3b832 100644 --- a/lighthouse-core/test/runner-test.js +++ b/lighthouse-core/test/runner-test.js @@ -290,7 +290,7 @@ 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 a non-fatal Error', () => { + it.skip('outputs an error audit result when required artifact was a Error', () => { const errorMessage = 'blurst of times'; const artifactError = new Error(errorMessage); @@ -326,7 +326,7 @@ describe('Runner', () => { requiredArtifacts: [], }; - it('produces an error audit result when an audit throws a non-fatal Error', () => { + it('produces an error audit result when an audit throws an Error', () => { const errorMessage = 'Audit yourself'; const config = new Config({ settings: { @@ -351,31 +351,6 @@ describe('Runner', () => { assert.ok(auditResult.errorMessage.includes(errorMessage)); }); }); - - it('rejects if an audit throws a fatal error', () => { - const errorMessage = 'Uh oh'; - const config = new Config({ - settings: { - auditMode: __dirname + '/fixtures/artifacts/empty-artifacts/', - }, - audits: [ - class FatalThrowyAudit extends Audit { - static get meta() { - return testAuditMeta; - } - static audit() { - const fatalError = new Error(errorMessage); - fatalError.fatal = true; - throw fatalError; - } - }, - ], - }); - - return Runner.run({}, {config}).then( - _ => assert.ok(false), - err => assert.strictEqual(err.message, errorMessage)); - }); }); it('accepts devtoolsLog in artifacts', () => { From cff18e97f55d22f5a6a2079bab64de7363eeda2c Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Fri, 19 Oct 2018 10:12:03 -0700 Subject: [PATCH 2/4] add catch --- lighthouse-core/gather/gather-runner.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index a259e6a8a2fd..a9e8e6718ac3 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -183,7 +183,7 @@ class GatherRunner { passContext.options = gathererDefn.options || {}; const artifactPromise = Promise.resolve().then(_ => gatherer.beforePass(passContext)); gathererResults[gatherer.name] = [artifactPromise]; - await artifactPromise; + await artifactPromise.catch(() => {}); } } @@ -227,7 +227,7 @@ class GatherRunner { const gathererResult = gathererResults[gatherer.name] || []; gathererResult.push(artifactPromise); gathererResults[gatherer.name] = gathererResult; - await artifactPromise; + await artifactPromise.catch(() => {}); } } @@ -294,7 +294,7 @@ class GatherRunner { const gathererResult = gathererResults[gatherer.name] || []; gathererResult.push(artifactPromise); gathererResults[gatherer.name] = gathererResult; - await artifactPromise; + await artifactPromise.catch(() => {}); log.verbose('statusEnd', status); } From 7c5ba9e8001dd90a14ce486b3ed50c7a555f5d08 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Fri, 19 Oct 2018 10:12:51 -0700 Subject: [PATCH 3/4] Update lighthouse-core/gather/gather-runner.js Co-Authored-By: Hoten --- lighthouse-core/gather/gather-runner.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index a9e8e6718ac3..2fad6ac72a39 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -327,7 +327,7 @@ class GatherRunner { // Typecast pretends artifact always provided here, but checked below for top-level `throw`. gathererArtifacts[gathererName] = /** @type {NonVoid} */ (artifact); } catch (err) { - // return error runner to handle turning it into an error audit. + // Return error to runner to handle turning it into an error audit. gathererArtifacts[gathererName] = err; } From acbb919f545c64a5587aa11ca5882aae65d19096 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 19 Oct 2018 11:14:07 -0700 Subject: [PATCH 4/4] Update lighthouse-core/test/runner-test.js Co-Authored-By: Hoten --- lighthouse-core/test/runner-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js index 97f753e3b832..45a83721e6b4 100644 --- a/lighthouse-core/test/runner-test.js +++ b/lighthouse-core/test/runner-test.js @@ -290,7 +290,7 @@ 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 a Error', () => { + it.skip('outputs an error audit result when required artifact was an Error', () => { const errorMessage = 'blurst of times'; const artifactError = new Error(errorMessage);