tests(fr): snapshot audit id lists in api test #13994
Conversation
| @@ -50,8 +50,7 @@ describe('Fraggle Rock API', () => { | |||
| expect(accessibility.score).toBeLessThan(1); | |||
|
|
|||
| const {auditResults, erroredAudits, failedAudits} = getAuditsBreakdown(lhr); | |||
| // TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached. | |||
| expect(auditResults.length).toMatchInlineSnapshot(`73`); | |||
| expect(auditResults.map(audit => audit.id)).toMatchSnapshot(); | |||
There was a problem hiding this comment.
can some of these be removed? Like is this assertion accomplishing anything anymore?
There was a problem hiding this comment.
I personally think it's a good guard against regressions that remove snapshot support for certain audits.
|
|
||
| expect(notApplicableAudits.length).toMatchInlineSnapshot(`5`); | ||
| expect(notApplicableAudits.map(audit => audit.id)).toMatchSnapshot(); |
There was a problem hiding this comment.
at least some of these notApplicable don't seem to have anything to do with FR-ness, just that they were notApplicable for this run. Seems like this will be covered by other tests now
There was a problem hiding this comment.
I think the reasoning behind this is that the N/A audits would vary a lot depending on if any network requests were fetched during the timespan.
I guess the counterargument is that this is the closest thing to a smoke test for some of this code right now. In the spirit of those old FR-COMPAT comments, I would like it if we were still moving to a place where I don't have to double check |
This is definitely true. If we treat this as a smoke test, I think it's good to have a condition that catches mode-compatibility regressions. I feel like these conditions have saved me from making such errors. |
|
@adamraine re-merge with master to be sure test still passes |
Makes it easier to investigate regressions in this test.