-
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
test: use predictable numbers in golden LHR #6404
Conversation
Can we rename |
been wanting to for a while. I'll do it after this PR |
what, it's not obvious what |
for test failures, see #6405 :) |
const lhr = JSON.parse(lhrString); | ||
delete lhr.configSettings.auditMode; |
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.
was there an issue with having this previously? Was it only because we were generating it more manually?
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.
its just this:
auditMode": "./lighthouse-core/test/results/artifacts",
hopefully it isn't an absolute path in some cases?
lhr.timing.total = 12345.6789; | ||
lhr.timing.entries.forEach((entry, i) => { | ||
// @ts-ignore | ||
entry.duration = 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.
why are these ignored? readonly
? Maybe we don't want them to descend from PerformanceEntry
after all :)
Should put a reason why it's being ignored. e.g. // @ts-ignore - write to readonly property
or whatever
// Set to predictable and typical value | ||
lhr.configSettings.auditMode = false; | ||
|
||
// Set timing values, which change from run-to-run, to predicable 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.
no -
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.
if we're jumping on the comments there's also a t
missing from predictable
:)
// @ts-ignore | ||
entry.duration = 100; | ||
// @ts-ignore | ||
entry.startTime = 100 * i + i; // 1ms gap between them |
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.
if we add new entries this is going to cause some noise. What about based on length of the name or some kind of hash of the name or something?
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.
0a013ac
to
e4039b3
Compare
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.
Need to rebase to get rid of that short name audit, but otherwise LGTM!
145d73a
to
e4817a8
Compare
No more excluding items from the golden LHR.