fix: deflake TTI audit by extending trace #1785
Conversation
lighthouse-core/config/default.json
Outdated
| @@ -3,6 +3,7 @@ | |||
| "passName": "defaultPass", | |||
| "recordNetwork": true, | |||
| "recordTrace": true, | |||
| "pauseAfterTraceMs": 500, | |||
There was a problem hiding this comment.
the pause is previous to ending the trace, so i'd think we'd call it something likerecordTraceEndDelay. wdyt?
There was a problem hiding this comment.
Yeah true, just was very similar in spirit to the pauseAfterLoadMs so wanted to be consistent. How about pauseBeforeTraceEnd? Nix the Ms either way?
There was a problem hiding this comment.
yah. i actually wrote that first. :) and then revised so it looks more similar to recordTrace since they're buddies
but yeah let's go with pauseBeforeTraceEnd since we both went there.
lighthouse-core/gather/driver.js
Outdated
| /** | ||
| * @param {number=} pauseAfterTraceMs Wait this many milliseconds before ending the trace | ||
| */ | ||
| endTrace(pauseAfterTraceMs = 500) { |
There was a problem hiding this comment.
dont you want a default of 0, since you're setting the config specifically for this pass?
There was a problem hiding this comment.
I guess depends on if we want previous custom configs to be opt-in or opt-out of the fix. The myriad of ways to run lighthouse gets thorny with all these options :) I'm fine with 0 too since I'm guessing custom config usage is quite, quite low
There was a problem hiding this comment.
yah. let's do 0.
but since you mentioned it. can you add pauseBeforeTraceEnd into the perf.json as well? :)
outside of that one, custom configs are pretty rare yeah. so far we have no evidence anyone other than us are using them.
lighthouse-core/gather/driver.js
Outdated
| @@ -606,7 +609,7 @@ class Driver { | |||
| }); | |||
|
|
|||
| // Issue the command to stop tracing. | |||
| this.sendCommand('Tracing.end').catch(reject); | |||
| setTimeout(() => this.sendCommand('Tracing.end').catch(reject), pauseAfterTraceMs); | |||
There was a problem hiding this comment.
let's just remind ourselves why this was neccessary. :) something like..
Stop tracing, after an optional delay. Audits like TTI may require having a slightly longer trace
done |
R: @paulirish all
fixes #1784