-
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
core(bootup-time): exclude _lighthouse-eval.js #15678
Conversation
@@ -109,6 +109,9 @@ class BootupTime extends Audit { | |||
settings.throttling.cpuSlowdownMultiplier : 1; | |||
|
|||
const executionTimings = getExecutionTimingsByURL(tasks, networkRecords); | |||
// Exclude our own tasks. | |||
executionTimings.delete('_lighthouse-eval.js'); |
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.
Can we move this inside getExecutionTImingsByURL
?
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.
not sure. i think @brendankenny wanted this data intact for work-for-interaction. also, getExecutionTimingsByURL is in a lib
folder, so we should try not to add overly lh-specific things there.
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.
also, getExecutionTimingsByURL is in a
lib
folder, so we should try not to add overly lh-specific things there
Yes, this basically sums it up, but my opinion isn't strongly held. It's a balance between the expectation of _lighthouse-eval.js
being filtered out and the expectation of all tasks in a period of a trace being accounted for. If there really was six seconds of a trace blocked by evaluate
but it's filtered out, you might have trouble tracking down what's going on in the resulting audits, while if _lighthouse-eval.js
isn't filtered out, it announces itself in the audit results.
This has not been released to PSI yet. |
Fixes issue reported here.