-
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: remove more mock computed artifacts #6195
Conversation
@@ -28,19 +28,4 @@ describe('Performance: speed-index audit', () => { | |||
assert.equal(result.rawValue, 605); | |||
}); | |||
}, 10000); | |||
|
|||
it('scores speed index of 845 as 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.
the test above already tests that a score of 605 yields a passing score. This seemed redundant (and difficult to fake)
timing = {...networkRecord.timing}; | ||
if (timing.requestTime === undefined) { | ||
timing.requestTime = networkRecord.startTime || 0; | ||
} |
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.
a few tweaks to get this to work well with existing tests. e.g. not overwriting timing
or initiator
as tests sometimes reuse the object and expect it to be unchanged
Oh, and remember that all the const artifacts = Object.assign(Runner.instantiateComputedArtifacts(), {
// ...
}); after the refactor will become just const artifacts = {
// ...
}; noisy but keeps the tests passing without a 2000 line PR or whatever :) |
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 😎
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.
LGTM. Always like making tests better 🎉
Follow up to #6171, using the
networkRecordsToDevtoolsLog
utility and passing in the resulting devtoolsLog instead of mocking network records directly. Also removed some other low hangingrequest*()
mocks in these files.Sorry for the test noise :)