core(fr): limit scope of audits to applicable modes #12764
Conversation
| try { | ||
| return await this._audit(artifacts, context); | ||
| } catch (err) { | ||
| const noFramesErrors = new Set([ |
There was a problem hiding this comment.
nit: Set seems like overkill for this
There was a problem hiding this comment.
overkill to employ a data structure that matches the semantics? :)
is there a particular concern addressed by using an array?
There was a problem hiding this comment.
I guess it boils down to a style choice, so we can leave it as is.
There was a problem hiding this comment.
SGTM!
just out of curiosity though, is there a particular concern you feel is addressed by using an array?
e.g. Sets are still too unfamiliar to most developers and take longer to understand the initialization than a plain array does
There was a problem hiding this comment.
No, I think the code is easily understandable. I do think it would look cleaner if we used the array directly.
| const {largestContentfulPaintEvt} = await ProcessedNavigation.request(processedTrace, context); | ||
| const {largestContentfulPaintEvt} = await ProcessedNavigation | ||
| .request(processedTrace, context) | ||
| .catch(err => { |
There was a problem hiding this comment.
yep! good catch
| @@ -194,7 +194,7 @@ describe('Fraggle Rock API', () => { | |||
| const {lhr} = result; | |||
| const {auditResults, failedAudits, erroredAudits} = getAuditsBreakdown(lhr); | |||
| // TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached. | |||
| expect(auditResults.length).toMatchInlineSnapshot(`152`); | |||
There was a problem hiding this comment.
yep, the inheriting supportedModes array is [] so it was never run
| expect(notApplicableAudits.map(audit => audit.id)).toContain('server-response-time'); | ||
|
|
||
| // TODO(FR-COMPAT): Reduce this number by handling the error, making N/A, or removing timespan support. | ||
| expect(erroredAudits.length).toMatchInlineSnapshot(`22`); | ||
| expect(erroredAudits.length).toMatchInlineSnapshot(`12`); |
Summary
Updates several more audits to accurately reflect their supported gather modes. Most of this is restricting the supportedMode to navigation even if the artifacts are available, a few optional notApplicable states, and then minor gatherer fixes too.
The remaining ones are due to...
simulatethrottlingMethod still being default in timespan (tracked by Flow Support (Fraggle Rock) #11313)Related Issues/PRs
ref #11313