garris / BackstopJS

Catch CSS curve balls.
http://backstopjs.org
MIT License
6.66k stars 602 forks source link

PATCH: Feature/1525 headless mode deprecation #1535

Closed dgrebb closed 6 months ago

dgrebb commented 6 months ago

Summary

Puppeteer defaults to "new" headless mode, as per the feature request in #1525, also discussed in #1485.

Playwright, on the other hand, will remain headless: true by default, as "new" mode is not supported without some configuration overhead. And speaking of Playwright...

[!IMPORTANT]

1534 is fixed, which I found while exploring the above-mentioned caveats of "new" mode.

This should be considered for an immediate patch

In the end, we support "new" headless mode in Playwright, but only when set by a Backstop configuration file.

I also addressed a fix for GitHub Actions npm run integration-test error: Error: Process completed with exit code 254..

Any and all input welcome! We want to be safe, but at the same time, want to support the cutting-edge and standards for Playwright, Puppeteer, and the various browser vendors and teams.

Details

Puppeteer

Playwright

From what I've gathered, Playwright is not planning to support "new" mode at this point in time. There are performance considerations for their users, so they make it more challenging to use the new mode.

Once Playwright supports "new" mode, we should discuss enforcing this default for Playwright, but until then, this is a user configuration.

Problem

Playwright expects a boolean for headless mode.

Solution

Override Playwrights default args.

We evaluate whether a user is trying to pass headless as a string, and include ignoreDefaultArgs: ["--headless"], which allows a string to be passed.

We set the property headless: true if "new" is passed, and include --headless=new in the final playwrightArgs sent to the runner.

{
 args: [ '--no-sandbox', '--headless=new' ],
 headless: true,
 ignoreDefaultArgs: [ '--headless' ]
}

Notes

Example Playwright headless: 'new' error

Creating Browser
No Playwright browser specified, assuming Chromium.
COMMAND | Command "test" ended with an error after [0.004s]
COMMAND | browserType.launch: headless: expected boolean, got string

Extra

I started adding inline documentation, which are a nice addition to overall developer experience. The workarounds for Playwright "new" headless mode are commented on for future reference.

Pasted image 20231224000613

dgrebb commented 6 months ago

@garris this addresses new headless mode. We can default Playwright to use new if that is the goal, but because it's a little hacky, I didn't do so (yet).

Please let me know if you'd like an npm script and GitHub Action test for this as well. I think it's worth it but didn't want to overload the PR more than it already may be.

garris commented 6 months ago

@dgrebb Thanks Dan for doing the extra research on this. It sounds like you are setting defaults that are in-line with the philosophies of the different frameworks -- which makes sense.

I don't think this new/old/true/false oddness needs to be elevated to a GH test -- I'd prefer to keep GH testing space for high-level critical-path stuff e.g. sanity, smoke, integration, basic UI backstop testing with your cool fixtures idea.

Thank you!