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: remove flaky Chrome launch from unit-cli #12359

Merged
merged 2 commits into from
Apr 13, 2021
Merged

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Apr 13, 2021

In the spirit of improving flaky tests, unit-cli has loooong had a flake based on a Chrome launch sometimes timing out. You know the one:

●  process.exit called with "1"

      84 | function printConnectionErrorAndExit() {
      85 |   console.error('Unable to connect to Chrome');
    > 86 |   return process.exit(_RUNTIME_ERROR_CODE);
         |                  ^
      87 | }
      88 | 
      89 | /** @return {never} */

      at printConnectionErrorAndExit (lighthouse-cli/run.js:86:18)

(for example: https://github.com/GoogleChrome/lighthouse/runs/2249363555?check_suite_focus=true)

The issue is that this is more or less a smoke test without any of the functionality we've built into smokehouse to handle dealing with launching and testing with Chrome in a CI environment.

Looking way back, the original point of the test (from #2602) was to make sure that Lighthouse can run when lighthouse-cli is accessed directly as a node module, since our smoke tests load Lighthouse via CLI. Very similar to the smoke tests, but we have no other test ensuring we don't break this particular use case. Since then, the assertions have grown to check a few other aspects of the run.

However the overlap between this and smokehouse's execution path is almost total (they both pass through runLighthouse in run.js), so it seems sufficient for this test to run lighthouse with --auditMode and the sample artifacts to make sure it's runnable, and trust smokehouse to verify run.js is exercising the gathering steps.

After this I believe there's no other Chrome launch in unit-cli, so it should be stable going forward.

@brendankenny brendankenny requested a review from a team as a code owner April 13, 2021 20:22
@brendankenny brendankenny requested review from patrickhulce and removed request for a team April 13, 2021 20:22
@google-cla google-cla bot added the cla: yes label Apr 13, 2021
@connorjclark
Copy link
Collaborator

want to also revert the run: yarn unit-cli || yarn unit-cli --onlyFailures bit?

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

so glad to see this flake gone for good :)

@brendankenny
Copy link
Member Author

want to also revert the run: yarn unit-cli || yarn unit-cli --onlyFailures bit?

I like that confidence :) We can see how it goes

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