Start using errors instead of -1 for error artifacts #1560
Conversation
|
|
| @@ -29,8 +29,7 @@ class CSSUsage extends Gatherer { | |||
| .catch(err => { | |||
| // TODO(phulce): Remove this once CSS usage hits stable | |||
| if (/startRuleUsageTracking/.test(err.message)) { | |||
| this.failure = 'CSS Usage tracking requires Chrome \u2265 56'; | |||
| return; | |||
| throw new Error('CSS Usage tracking requires Chrome \u2265 56'); | |||
There was a problem hiding this comment.
@patrickhulce now that errors are non-fatal by default, afterPass still throws but this error wins
|
Looks nice so far! Do we have a location where we'd quit on a fatal error? Would love to see that in action. |
What do you mean? Like a test run really quits? We don't have any gatherer that throws a fatal error yet :) (and I'm not entirely sure what would warrant doing that at this point) The |
|
yah? nah? |
|
|
||
| // If artifact was an error, it must be non-fatal (or gatherRunner would | ||
| // have thrown). Output error result on behalf of audit. | ||
| if (artifacts[artifactName] instanceof Error) { |
There was a problem hiding this comment.
Any of the same machinery for audits? Or are we still treating all errors there as fatal?
There was a problem hiding this comment.
Any of the same machinery for audits? Or are we still treating all errors there as fatal?
we can. There's no reason we can't also get rid of generateAuditResult({rawValue: -1, debugString: 'whatever'}) and just let audits throw errors and teach the reporter(s) to handle output for those cases
| * @param {!Promise<*>} promise | ||
| * @return {!Promise<*>} | ||
| */ | ||
| static recoverOrThrow(promise) { | ||
| return promise.catch(err => { | ||
| if (!err.recoverable) { | ||
| if (err.fatal) { |
There was a problem hiding this comment.
Any of the same machinery for audits? Or are we still treating all errors there as fatal?
|
Nice I like this direction a lot!! Can't wait to get that tracking threaded through... |
switch from "recoverable" errors to fatal/non-fatal gatherer errors
brendankenny
force-pushed
the
gathererrors
branch
from
cb9bda4
to
73c94ad
Compare
indeed :) |
|
LGTY? I was planning on merging and doing these in chunks so I don't have to keep rebasing :) |
|
👍 |
retrying #1498 (as next step of #941), now with errors from gatherers being considered recoverable by default. If you want to stop a run, throw an error with a
fatalproperty set totrue.Biggest benefit is that if something throws inside a gatherer that isn't something the gatherer cares about or is specifically watching for, it can just let the exception go. That means
catchblock in every gatherer-1artifacts...now they're justErrorobjects-1artifacts in every audit. If one of an audit'srequiredArtifactsis an error, the audit never runs and LH emits an error result on behalf of that audit.First commits changes old
recoverableinfra to supportfatal.Other commits try switching over some of our gatherers to using this format. The first two (
meta viewportandtheme-color) are simple, the last commit is giving that treatment to the slightly more complicatedStylesandCSSUsagegatherers and the two audits that need them.Thoughts?