core(config): assert all audit requiredArtifacts will be gathered #9284
Conversation
lighthouse-core/config/config.js
Outdated
| 'URL', | ||
| 'Timing', | ||
| 'PageLoadError', | ||
| ]; |
There was a problem hiding this comment.
I couldn't think of a better way to do this. Suggestions? We don't have a place where we export it anywhere. We could separate construction from populating in gather-runner, I guess...
There was a problem hiding this comment.
Object.keys(/** @type {Record<keyof LH.BaseArtifacts, string>} */ ({
...
}))to enable type checking at least so it's automated to update?
There was a problem hiding this comment.
to enable type checking at least so it's automated to update?
haha, it's terrible but I love it :)
| @@ -296,7 +296,7 @@ class Runner { | |||
| if (noArtifact || noTrace) { | |||
| log.warn('Runner', | |||
There was a problem hiding this comment.
originally made this log.verbose, under the theory that you get a big red pageLoadError logged above this, but that's scrolled away and you just get a bunch of blue status Auditing: Properly size images +1ms, etc messages that make everything look ok. So now we just don't log here and below
| @@ -58,7 +58,7 @@ describe('Config', () => { | |||
| assert.equal(MyAudit, newConfig.audits[0].implementation); | |||
| }); | |||
|
|
|||
| it('doesn\'t change directly injected plugin instances', () => { | |||
There was a problem hiding this comment.
these are just confusing old uses of "plugin" for "live Gatherer instances"
lighthouse-core/config/config.js
Outdated
| 'URL', | ||
| 'Timing', | ||
| 'PageLoadError', | ||
| ]; |
There was a problem hiding this comment.
Object.keys(/** @type {Record<keyof LH.BaseArtifacts, string>} */ ({
...
}))to enable type checking at least so it's automated to update?
| const auditMeta = auditDefn.implementation.meta; | ||
| for (const requiredArtifact of auditMeta.requiredArtifacts) { | ||
| if (!foundGatherers.has(requiredArtifact)) { | ||
| throw new Error(`${requiredArtifact} gatherer, required by audit ${auditMeta.id}, ` + |
There was a problem hiding this comment.
should we make this an i18n'd LHError too while we're adding it? I'd personally vote no on all config errors since we never expect these to happen in blessed environments :)
There was a problem hiding this comment.
should we make this an i18n'd LHError too while we're adding it? I'd personally vote no on all config errors since we never expect these to happen in blessed environments :)
yeah, I'll vote no too. Luckily like a million tests will fail if the default config starts throwing this error :)
as mentioned in #8865 (review), when there is a pageLoadError you get a ton of logging, 2x every audit. #9236 moved that logging from "Required x gatherer encountered an error" to "Required x gatherer did not run", but you still get a lot of it.
This PR
"a gatherer didn't run due to an error"and"a gatherer didn't run due to not being included in the config". Currently those cases are logged the exact same way, and not until the whole LH run has completed, which sucks if you just messed up your config.