Skip to content
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.

Sign up for GitHub

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

Jump to bottom

tests: add smoketest to capture microtask regression #8379

Merged
merged 9 commits into from
Apr 30, 2019
Merged

tests: add smoketest to capture microtask regression #8379

merged 9 commits into from
Apr 30, 2019

Conversation

patrickhulce
Copy link
Collaborator

Summary
There was a recent Chrome change where we lost all CPU time that was spent in microtasks. It's being fixed (https://bugs.chromium.org/p/chromium/issues/detail?id=953914), but we should have a test that would've caught this earlier.

@brendankenny
Copy link
Member

needs lint fixes

connorjclark
connorjclark reviewed Apr 19, 2019
View reviewed changes
lighthouse-cli/test/smokehouse/perf/lantern-expectations.js
@@ -31,9 +31,9 @@ module.exports = [
requestedUrl: 'http://localhost:10200/tricky-main-thread.html?setTimeout',
finalUrl: 'http://localhost:10200/tricky-main-thread.html?setTimeout',
audits: {
interactive: {
'interactive': {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmmmmmmmmmmmm do we have to? :neckbeard:

brendankenny
brendankenny reviewed Apr 22, 2019
View reviewed changes
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrickhulce what do you want to do here? Is the smoke test failure expected or is something else still in progress?

lighthouse-cli/test/smokehouse/perf/lantern-expectations.js Outdated
details: {
items: {
0: {
// TODO: requires async stacks, https://github.com/GoogleChrome/lighthouse/pull/5504
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like we don't need the TODO anymore? (// requires async stacks might be worth keeping, though)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah, I'll update this, but turns out async stacks are just one required part for this particular bug 😞

I'll file a new one to track progress.

@patrickhulce
Copy link
Collaborator Author

what do you want to do here? Is the smoke test failure expected or is something else still in progress?

The smoketest is accurately capturing the failure mode this test would have alerted us to, so that's good news.

The bad news is Travis is running with Google Chrome 73.0.3683.103 and shouldn't have the commit we bisected to that caused it. Further investigation reveals that it's apparently been broken before. I don't have the exact commit range, but it looks like the timeline was somewhere along the lines of the following

July 2018 and earlier (example: 68.0.3410.0)
Microtask work nested inside toplevel event

Fall 2018 - February 2019 (examples: 70.0.3538.0, 71.0.3551.0, 73.0.3683.103)
Microtask work no longer nested inside toplevel event (I suspect https://bugs.chromium.org/p/chromium/issues/detail?id=871204 but haven't done an actual bisect)

? - March 19 2019 (example: 74.0.3729.0)
Microtask work nested inside toplevel event

March 20 2019 - April 18 2019 (example: 75.0.3766.0)
Microtask work no longer nested inside toplevel event

April 18 2019 - Present (example: 76.0.3773.0)
Microtask work nested inside toplevel event

Good news is that because it was broken and fixed within 75 before branch I think we just need to wait for m74 until this can be merged (next Chrome stable).

@patrickhulce patrickhulce mentioned this pull request Apr 30, 2019
Merged
brendankenny
brendankenny approved these changes Apr 30, 2019
View reviewed changes
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

soooooo, Travis is testing m74 and all seems good now? Good to merge?

@patrickhulce
Copy link
Collaborator Author

Travis is testing m74 and all seems good now? Good to merge?

Yeppers!

@patrickhulce patrickhulce merged commit 2350dd9 into master Apr 30, 2019
@patrickhulce patrickhulce deleted the include_microtasks branch April 30, 2019 18:32
@connorjclark
Copy link
Collaborator

connorjclark commented May 2, 2019 •
edited
Loading

Could we define the assertions for interactive relative to numericValue instead of score? This smoke test is failing in LR:

image

A score of 0.94 vs the expected <0.2 doesn't tell me as much as a numericValue of 3250 vs the expected value of Y would. I know I can calculate Y after determining how the score is calculated, but ew maths :)

@patrickhulce
Copy link
Collaborator Author

yeah sure @hoten , also chatting with paul the version of m75 that's in LR was indeed a broken (just a few hours before the fix IIRC) so it is catching good things

@patrickhulce patrickhulce mentioned this pull request May 2, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@connorjclark connorjclark connorjclark left review comments

@brendankenny brendankenny brendankenny approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@patrickhulce @brendankenny @connorjclark