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

[FTR] Switch to new browser headless mode #153828

Merged
merged 16 commits into from
Apr 4, 2023
Merged

Conversation

pheyos
Copy link
Member

@pheyos pheyos commented Mar 28, 2023

Summary

This PR updates the way how we start the headless browser for testing.

The current way of starting in headless mode is eventually going away and the new headless mode offers more capabilities and stability, see https://www.selenium.dev/blog/2023/headless-is-going-away/ and https://developer.chrome.com/articles/new-headless/.

Test adjustments

All the adjusted discover, dashboard, maps and infra tests showed the same pattern during failure investigation that's around the fact that the new headless mode is closer to the regular / non-headless mode:

  • Tests passed with the old headless mode
  • Tests failed in regular / non-headless mode the same way they failed in new headless mode
  • The failure reasons were mostly around slightly different font rendering and slightly different browser sizes

@pheyos pheyos self-assigned this Mar 28, 2023
@pheyos
Copy link
Member Author

pheyos commented Mar 30, 2023

Checking stability of the adjusted discover, dashboard and maps tests in a flaky test runner job ... no failure in 50 runs each ✔️

@pheyos
Copy link
Member Author

pheyos commented Mar 30, 2023

@elasticmachine merge upstream

@pheyos
Copy link
Member Author

pheyos commented Mar 31, 2023

@elasticmachine merge upstream

@pheyos
Copy link
Member Author

pheyos commented Mar 31, 2023

@elasticmachine merge upstream

@pheyos
Copy link
Member Author

pheyos commented Apr 3, 2023

@elasticmachine merge upstream

@pheyos
Copy link
Member Author

pheyos commented Apr 3, 2023

Recent failures were unrelated to this change (API integration tests don't use the headless browser).
This will hopefully be resolved soon, still opening this PR for reviews.

@pheyos pheyos added v8.8.0 release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) test_ui_functional labels Apr 3, 2023
@pheyos pheyos marked this pull request as ready for review April 3, 2023 11:11
@pheyos pheyos requested review from a team as code owners April 3, 2023 11:11
@pheyos pheyos requested a review from dmlemeshko April 3, 2023 11:11
Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

Data Discovery changes LGTM 👍

@@ -99,9 +99,12 @@ function initChromiumOptions(browserType: Browsers, acceptInsecureCerts: boolean
}

if (headlessBrowser === '1') {
// Using the new headless mode (instead of `options.headless()`)
// See: https://www.selenium.dev/blog/2023/headless-is-going-away/
Copy link
Member

Choose a reason for hiding this comment

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

Smart adding the link in the comment! 🎉

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Could you provide some details into why these test changes are required? Did the screen size change for functional tests or something?

@pheyos
Copy link
Member Author

pheyos commented Apr 3, 2023

@nreese Indeed, the size of the browser content area changed with the new headless mode. Reading the article that describes how the new headless mode shares code with the headful version and also seeing how running in headful mode gives the same browser content dimensions (same as new headless, different from old headless), makes me suspect the following (although I haven't found a clear statement about it):

When setting the browser window size during tests,

  • the old headless mode (a separate, alternate browser implementation) sets the content area to that size
  • the new headless mode (same as the regular headful mode) sets the browser window to that size, so the actual content area is smaller (window decorations, menu bar, ...)

I hope that helps. Let me know if you want me to add more details.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

kibana-gis changes LGTM
code review

@dmlemeshko
Copy link
Member

Code LGTM, Tested locally on Mac OS:

  • Chrome head: all changed tests passed
  • Chrome headless: maps related tests are failing, but I think we had the Mac OS issue with WebGL for a while (correct?)
  • Firefox head: few tests are failing due to changes

@pheyos pheyos requested a review from a team as a code owner April 4, 2023 11:54
@pheyos
Copy link
Member Author

pheyos commented Apr 4, 2023

An infra test started failing on new headless mode after a recent test change, again due to the slightly different browser size. Adjusted the browser size for this suite and started a flaky test runner job for the modified infra suite to check test stability... no failure in 50 runs ✔️

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 432 435 +3

Total ESLint disabled count

id before after diff
securitySolution 512 515 +3

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @pheyos

Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

Infra UI changes look good, thanks 🙂

tonyghiani

This comment was marked as duplicate.

@pheyos pheyos merged commit 98df0c2 into elastic:main Apr 4, 2023
@pheyos pheyos deleted the new_headless_mode branch April 4, 2023 13:54
@pheyos pheyos removed the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Apr 4, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 4, 2023
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.7 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 153828

Questions ?

Please refer to the Backport tool documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes test_ui_functional v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants