-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests: better times for generated network requests #14714
Conversation
@@ -55,7 +55,7 @@ describe('Performance: preload-lcp audit', () => { | |||
priority: 'High', | |||
isLinkPreload: false, | |||
startTime: 0, | |||
endTime: 0.5, | |||
endTime: 500, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
startTime
0 + receiveHeadersEnd
500 is well after an endTime
of 0.5 :)
endTime: 1000, | ||
timing: {sslStart: 50, sslEnd: 100, connectStart: 50, connectEnd: 100}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these timing
entries aren't possible before an endTime
of 0.0001, but going back to #13452, main document is always included (so doesn't need to be before FCP), and the next (incomplete) request can be any time after FCP, so these changes still hit the check added in that PR
*/ | ||
function mockRequest(partial) { | ||
return Object.assign(new NetworkRequest(), { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assigning to a new NetworkRequest
makes for a weird network request (lots of -1s), and then passing it through networkRecordsToDevtoolsLog
just breaks things. Instead, just use networkRecordsToDevtoolsLog
and NetworkRecorder
to make the NetworkRequest
s
endTime: 2000, | ||
startTime: 50, | ||
responseReceivedTime: 1000, | ||
endTime: 2000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test change from #14574 to verify this works for that
* Takes the partial network timing and fills in the missing values so at least | ||
* time is linear (if not realistic). The main timing properties on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responseReceivedTime, | ||
endTime, | ||
timing: { | ||
// TODO: other `timing` properties could have default values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be expanded, e.g. no DNS for follow up requests, no SSL for non-https, etc, but seemed unnecessary for now
// Note: since redirectResponseTimestamp (aka the redirect's endTime) is | ||
// used as a last resort, some redirected requests may start before the | ||
// redirect ends. Up to the caller to override if this matters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't possible to fix without figuring out timing for all requests at once instead of individually, so the extra complexity didn't seem worth it (afaik there's no real correctness problem, just might be surprising)
// Default request startTime if none provided. Do not use 0 due to guard against | ||
// bad network records starting at 0. See https://github.com/GoogleChrome/lighthouse/pull/6780 | ||
const defaultStart = 1000; | ||
const defaultTimingOffset = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two values just allow us to be lazy when making mock requests and the results will look reasonable, right?
If the true numbers arent provided, then the req starts at 1000 and then two of the phases take at least 1s.. ya?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, exactly. I left a comment saying basically that down on getNormalizedRequestTiming
but could put something up here as well if it would help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems great.
* are up to the caller to order correctly. If no absolute times are provided, | ||
* starts at `defaultStart`, finishes receiving headers `defaultTimingOffset` ms | ||
* later, then endTime is `defaultTimingOffset` ms after that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* are up to the caller to order correctly. If no absolute times are provided, | |
* starts at `defaultStart`, finishes receiving headers `defaultTimingOffset` ms | |
* later, then endTime is `defaultTimingOffset` ms after that. | |
* are up to the caller to order correctly. | |
* If no absolute times are provided: starts at `defaultStart`, finishes receiving | |
* headers `defaultTimingOffset` ms later, then endTime is `defaultTimingOffset` ms after that. |
I'm having some trouble parsing this sentence. Trivial edit above, but hopefully you can improve further.
// redirect ends. Up to the caller to override if this matters. | ||
extractedTimes.redirectResponseTimestamp, | ||
]; | ||
const startTime = possibleStarts.filter(timeDefined)[0] ?? defaultStart; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
const startTime = possibleStarts.filter(timeDefined)[0] ?? defaultStart; | |
const startTime = possibleStarts.find(timeDefined) ?? defaultStart; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh nice
for (let j = i + 1; j < keys.length; j++) { | ||
const comparison = keys[j]; | ||
if (!timeDefined(values[comparison])) continue; | ||
assert(values[step] <= values[comparison], `'${step}' (${values[step]}) exceeds '${comparison}' (${values[comparison]}) in test network record`); // eslint-disable-line max-len |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a minor concern about how much this would generate the error string without ever needing to show it, but this path is hit <3000 times when running the core tests so not a big deal.
network-records-to-devtools-log
is very convenient, but it has always played fast and loose with network timings, doing pretty much the bare minimum to create partial network records with the given timings, even when those timings don't internally make sense (startTime
can be afterendTime
, etc). This worked because unit tests usually provided the properties needed for the unit being tested, but that's fragile and it finally caught up to us in #14574 when combined with the timing complexity of_recomputeTimesWithResourceTiming
.So, it's time to have
network-records-to-devtools-log
actually put in some effort into generating internally consistent timing. Unfortunately the many interrelations between the timings make this a little complex, but they're pretty easily testable, expandable if new properties are needed, and AFAIK the fundamentals haven't changed for the lifetime of Lighthouse so should be pretty stable. There's also a bunch of new assertions to assist authoring to let you know if you've asked for an impossible network request (which we've done a few times).The approach is to look at each partial network request individually, extract any timing properties provided, assert they're internally consistent, and then fill in the rest of the timing properties with values that are at least possible (under the assumption that if more probable values are needed, the caller would need to provide them). This produces a normalized timing object, which is then used for any subsequent uses of timing data so that the logic is all in one place instead of spread out in each function producing a devtoolsLog entry, as it was before.