core(fr): convert base artifacts to gatherer artifacts #12129
Conversation
patrickhulce
requested review from
connorjclark
and removed request for
a team
connorjclark
left a comment
connorjclark
left a comment
There was a problem hiding this comment.
LGTM, just a couple comments. but since this is so big I think a second person should review too.
Co-authored-by: Connor Clark <[email protected]>
…e into fr_base_artifacts
|
@adamraine you interested in giving this a second look? :) |
| const {auditResults, erroredAudits, failedAudits} = getAuditsBreakdown(lhr); | ||
| // TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached. | ||
| expect(auditResults.length).toMatchInlineSnapshot(`65`); | ||
| expect(auditResults.length).toMatchInlineSnapshot(`67`); |
There was a problem hiding this comment.
Weren't there 3 gatherers added that can do snapshot mode? InstallabilityErrors, Stacks, and WebAppManifest?
There was a problem hiding this comment.
yes great catch! turns out quite a few of the artifact ids were misspelled. we have 7 more artifacts now 🎉
There was a problem hiding this comment.
I added typechecking to the new default-config that will ensure all artifact IDs used in strings are typechecked against our usages
| const {auditResults, erroredAudits, failedAudits} = getAuditsBreakdown(lhr); | ||
| // TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached. | ||
| expect(auditResults.length).toMatchInlineSnapshot(`67`); | ||
| expect(auditResults.length).toMatchInlineSnapshot(`72`); |
There was a problem hiding this comment.
There were 4 misspellings but this jumped from 67 to 72. Is that because more than 1 audit use the same artifact?
4 participants
Summary
Converts most of the base artifacts so that they're runnable in fraggle rock as regular gatherers, but preserves the current base behavior in the legacy path as well.
environment.jsoff of driver.urlas a shared property of the FRTransitionalContextRelated Issues/PRs
ref #11313