-
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(devtools): ensure device emulation is ready #14431
Conversation
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.
Makes general sense to me, but does this better approximate how users will run Lighthouse in real world use? Like will the emulation model typically be initialized by the time a human hits "analyze"?
LGTM but maybe @connorjclark or @paulirish should take a look for DT lifecycle specifics?
core/scripts/pptr-run-devtools.js
Outdated
@@ -183,6 +183,11 @@ async function waitForLighthouseReady() { | |||
} | |||
} | |||
} | |||
|
|||
// Ensure the emulation model is ready before Lighthouse starts by enabling device emulation. | |||
// `waitForLighthouseReady` can be run multiple times, so this should be the final action. |
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.
I don't have enough background to follow the reasoning here. Can you explain more?
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.
waitForLighthouseReady
is run in a loop until it returns successfully without throwing an error. If an error occurred after this line, we would toggle the device emulation multiple times. Toggling device emulation is not idempotent so this could be a problem.
Maybe theres a better function we could use that's not a toggle.
This is my understanding. We are triggering things too quickly for DevTools to keep up. A less efficient solution would be to add a timeout between opening the page + DevTools and starting Lighthouse. |
…house into dt-emulate-before-run
Should fix #14428
I noticed that the emulation model was not fully initialized when we try to start emulation for Lighthouse:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/models/emulation/DeviceModeModel.ts;l=267
This PR forces the emulation model to be ready by enabling device emulation before Lighthouse starts. I also noticed that undocking DT as early as possible helped.