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.

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 Lighthouse starts in smoke tests #15459

Merged
merged 11 commits into from
Sep 18, 2023

Conversation

adamraine
Copy link
Member

A lot of our flaky DevTools smoke failures are timeouts on the protocol command that waits for Lighthouse to finish (example). After some investigation, I discovered that the Lighthouse run never actually starts in these cases. For whatever reason, clicking the start button just once isn't 100% reliable.

This PR adds a loop that clicks the start button every 100ms until we detect the run has started via JS sniffer.

I also added some code that lets us view a screenshot of the DT frontend when a timeout occurs. This screenshot diagnostic is already used in DT e2e tests, and was super helpful for debugging this problem.

@adamraine adamraine requested a review from a team as a code owner September 15, 2023 00:28
@adamraine adamraine requested review from brendankenny and removed request for a team September 15, 2023 00:28
@@ -328,6 +341,16 @@ async function testUrlFromDevtools(url, options = {}) {
const result = await evaluateInSession(inspectorSession, runLighthouse, [addSniffer]);

return {...result, logs};
} catch (err) {
if (options.detailedError && inspectorSession) {
Copy link
Collaborator

@connorjclark connorjclark Sep 18, 2023

Choose a reason for hiding this comment

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

why not always do this? debug mode is more about reducing output noise, but giving context on failure is always useful

Copy link
Member Author

Choose a reason for hiding this comment

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

The screenshot URL is very long compared to other output. This isn't really "noise" for the reason you mentioned but it is extra debug information. Since this is for errors and not expectation differences it makes more sense to always show the screenshot.

Copy link
Collaborator

@connorjclark connorjclark Sep 18, 2023

Choose a reason for hiding this comment

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

Could it save to disk in .tmp and just print the path? And then debug mode would print the contents too (for purposes of CI..) ... or we could upload a folder called .tmp/debug-info.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ehh I like having the screenshot easily accessible in CI logs. I don't want to have to download CI failure artifacts if I don't have to.

Plus, this file is organized to be called by a smoke runner but isn't aware of any smoke artifact folders itself.

@adamraine adamraine merged commit dac2762 into main Sep 18, 2023
29 checks passed
@adamraine adamraine deleted the print-dt-smoke-logs branch September 18, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants