core: include finished state on hidden network-requests audit #10530
Conversation
| }, | ||
| { | ||
| url: 'http://localhost:10200/favicon.ico', | ||
| finished: true, |
There was a problem hiding this comment.
it's nice to have smoke coverage, but clearly we're not too worried about covering this audit's entire output given the assertions here (and two other places where we assert just details.items.length), so this could be removed if it feels excessive
There was a problem hiding this comment.
I like the idea of asserting these somewhere so seems fine 🤷♂
| @@ -62,6 +63,7 @@ class NetworkRequests extends Audit { | |||
| }; | |||
| }); | |||
|
|
|||
| // NOTE(i18n): this audit is only for debug info in the LHR and does not appear in the report. | |||
There was a problem hiding this comment.
the other options are:
- actually i18n this file. Many of these column names already exist in our strings, but some (
'Status Code','MIME Type', etc) do not and will have to be translated. Seems not worth it? - remove the headings and make this
debugdata, which it really already is. Seems like there's benefit to having it be the common table format, though, and the headings kind of serve as documentation for the columns?
I'm fine with any of these.
There was a problem hiding this comment.
I like what we have here, continue to leave it un-i18n'd 👍
| const networkRecordsToDevtoolsLog = require('../network-records-to-devtools-log.js'); | ||
|
|
||
| const acceptableDevToolsLog = require('../fixtures/traces/progressive-app-m60.devtools.log.json'); | ||
| const cutoffLoadDevtoolsLog = require('../fixtures/traces/cutoff-load-m83.devtoolslog.json'); |
There was a problem hiding this comment.
introduced a new test devtoolsLog/trace of a load of the report viewer, devtools throttling, cut off at 1500ms (via --max-wait-for-load). Six network requests, three unfinished.
Seems handy to have a kind of gross load like that with relatively small fixture file sizes.
| @@ -27,4 +28,16 @@ expect.extend({ | |||
|
|
|||
| return {actual, message, pass}; | |||
| }, | |||
|
|
|||
| // Expose toBeCloseTo() so it can be used as an asymmetric matcher. | |||
| toBeApproximately(...args) { | |||
There was a problem hiding this comment.
it's really dumb you can't use toBeCloseTo in toMatchObject, so this takes care of that :)
| /** | ||
| * Asserts that an i18n string (using en-US) matches an expected pattern. | ||
| */ | ||
| toBeDisplayString: (pattern: RegExp) => R; |
There was a problem hiding this comment.
below toBeApproximately is just indent fix and correcting s/object/R
|
(there's not much interesting happening in this audit or its tests, so replaced the basic test with the new cutoff-devtoolsLog test rather than accumulating file-loading slowish tests that do the same thing) |
| }, | ||
| { | ||
| url: 'http://localhost:10200/byte-efficiency/script.js?gzip=1', | ||
| transferSize: '1100 +/- 100', | ||
| resourceSize: '53000 +/- 1000', | ||
| finished: false, |
There was a problem hiding this comment.
wait, why is this false? 😕
There was a problem hiding this comment.
oh phew it's failing, this is true :)
There was a problem hiding this comment.
wait, why is this false? 😕
Whoops, was checking that it really was asserting and pushed that version :)
| }, | ||
| { | ||
| url: 'http://localhost:10200/favicon.ico', | ||
| finished: true, |
There was a problem hiding this comment.
I like the idea of asserting these somewhere so seems fine 🤷♂
| @@ -62,6 +63,7 @@ class NetworkRequests extends Audit { | |||
| }; | |||
| }); | |||
|
|
|||
| // NOTE(i18n): this audit is only for debug info in the LHR and does not appear in the report. | |||
There was a problem hiding this comment.
I like what we have here, continue to leave it un-i18n'd 👍
part of #10527 (comment)