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: use --headless=new for all smoketests #14419

Merged
merged 20 commits into from
Oct 18, 2023
Merged

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Oct 3, 2022

I've wanted to use headless for CLI smoketests to avoid windows popping up and stealing focus. But as we know, some test assertions fail.

The --headless=chrome flag provides something much more like Chrome than what the --headless flag launches. And it now has support across windows/linux/mac.

edit: feb 2023: the flag was renamed to --headless-new. GoogleChrome/chrome-launcher#290

🔒 http://go/rooxt and http://go/wkigd

@paulirish paulirish requested a review from a team as a code owner October 3, 2022 19:43
@paulirish paulirish requested review from brendankenny and removed request for a team October 3, 2022 19:43
@paulirish paulirish changed the title tests: use native headless for smoketests tests: use native headless for cli smoketests Oct 3, 2022
@@ -61,6 +61,8 @@ async function internalRun(url, tmpPath, configJson, options) {
`${url}`,
`--output-path=${outputPath}`,
'--output=json',
// native headless, as full headless has several behavior differences that break tests. http://go/rooxt
'--chrome-flags="--headless=chrome"',
Copy link
Collaborator

@connorjclark connorjclark Oct 3, 2022

Choose a reason for hiding this comment

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

Looking at my (stale) branch doing the same, here are some things to consider:

  • there is also lighthouse-runners/bundle.js to update
  • headless-chrome.md could mention this new mode. what I had:

+There is also the new --headless=chrome option, which includes functionality that
+was explicitly omitted from the original headless browser.
+

CLI (xvfb)

Last time I tried this, there were issues re: the protocol hanging on screenshots, IIRC.

Copy link
Member Author

Choose a reason for hiding this comment

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

SG. also i wanted to experiment with using this in CI and getting rid of xvfb, which should (in theory) work.

but i'll do this part in a followup as i expect it'll be slightly more involved

ill address your two bullets in this one

@connorjclark connorjclark changed the title tests: use native headless for cli smoketests tests: use --headless=chrome for cli smoketests Oct 3, 2022
Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

Could we add a way to force headfull mode via the smokehouse CLI? --force-headfull perhaps?

@paulirish
Copy link
Member Author

Could we add a way to force headfull mode via the smokehouse CLI? --force-headfull perhaps?

done.


also. i suspect theres still failures here. I'm gonna pick this PR back up post 10.0 probably.

@adamraine adamraine changed the title tests: use --headless=chrome for cli smoketests tests: use --headless=new for all smoketests Oct 18, 2023
/** If true, performs extra logging from the test runs. */
isDebug?: boolean;
/** Launch Chrome in typical desktop headful mode, rather than our default of `--headless=new`. */
forceHeadful?: boolean;
Copy link
Collaborator

@connorjclark connorjclark Oct 18, 2023

Choose a reason for hiding this comment

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

nit: to match puppeteer and be a bit more straight-forward, could this be headless?: boolean (which defaults to true when not specified)?

The cli invocation for not headless would then be --no-headless (I believe yargs handles that for us, if we define headless to be a boolean flag)

Copy link
Member

@adamraine adamraine Oct 18, 2023

Choose a reason for hiding this comment

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

Edit: didn't see your edit

Would that still be --force-headful on the CLI though? If that's the way we are naming the flag I would prefer to just keep the option as forceHeadful throughout.

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.

5 participants