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

Playwright screenshots on MacOS creates different file names #1030

Closed
marcellmueller opened this issue Mar 5, 2024 · 11 comments
Closed

Playwright screenshots on MacOS creates different file names #1030

marcellmueller opened this issue Mar 5, 2024 · 11 comments
Assignees
Labels

Comments

@marcellmueller
Copy link
Contributor

Describe the Bug:

Generating Playwright snapshots on MacOS appends a different OS name:
Screenshot 2024-03-04 at 1 22 46 PM

A couple possible solutions are:

Removing the os suffix is likely the easiest for everyone though there is the possibility we will get diffs at some point in the future.

Probability (how likely the bug is to happen, scored from 1-5): 2
(For example, probability of 5 is something like "it happens to all users every time they log in."
Probability of 1 "only happens to certain users when a really specific and unlikely path is followed.")

Effect (how bad the bug is when it does happen, scored from 1-5): 4
(For example, effect of 5 is "the entire app crashes and makes it unusable for all users" or "the bug causes the wrong data to be saved, with critical information (e.g. payment) being affected."
Effect of 1 is "It makes some styling look a little bit weird.")

Steps to reproduce the behaviour:

Screenshots:

Additional information:

  • OS: MacOS
  • Browser: [e.g. chrome, safari]
  • Version: [e.g. 22]
  • Additional context
@shon-button
Copy link
Contributor

@marcellmueller nice exploration of the issue!

To me, it seems Use docker to generate screenshots is the way to go.

@marcellmueller marcellmueller self-assigned this Mar 5, 2024
@marcellmueller
Copy link
Contributor Author

For your consideration: I've been trying for awhile and having lots of issues with the docker command, looks like it needs additional setup to communicate with my backend. At this point it seems like it will be faster and simpler to use a virtual machine with Ubuntu Multipass or else set this project up on my Thinkpad which runs Linux and update screenshots from there.

As a Mac user none of these options sound very appealing and just too much hassle to update simple snapshot diffs. Personally I would prefer at least trying to remove the OS suffix as it is by far the easiest and no need to deal with docker or virtual machines.

@shon-button
Copy link
Contributor

@marcellmueller

Valid points about the hassle for snapshots update and a potential simpler suffix solution.

Although, perhaps a DevOpsy view point could provide further input on the Docker command as this seems the most stable solution.

This is the command I have so far- seems close?

docker run -v $PWD:/e2e -w /e2e --rm -it mcr.microsoft.com/playwright:v1.40.0-jammy /bin/bash -c "npm i && yarn build && yarn start && poetry run python manage.py migrate && poetry run gunicorn --access-logfile - bc_obps.wsgi:application --timeout 200 --workers 3 --bind '0.0.0.0:8000' && npx playwright test --update-snapshots"

@marcellmueller
Copy link
Contributor Author

marcellmueller commented Mar 5, 2024

This is the command I have so far- seems close?

docker run -v $PWD:/e2e -w /e2e --rm -it mcr.microsoft.com/playwright:v1.40.0-jammy /bin/bash -c "npm i && yarn build && yarn start && poetry run python manage.py migrate && poetry run gunicorn --access-logfile - bc_obps.wsgi:application --timeout 200 --workers 3 --bind '0.0.0.0:8000' && npx playwright test --update-snapshots"

Thanks for this, I think it is close but it doesn't run the && npx playwright test --update-snapshots command since the previous command runs a server:

Edit: sorry it appears to be the yarn start command it finishes on since it runs the front end build server
Screenshot 2024-03-05 at 2 23 58 PM

@marcellmueller
Copy link
Contributor Author

marcellmueller commented Mar 6, 2024

@shon-button

I've put some thought and effort into this over the past couple days and really am starting to think that this is not something that developers should have to commit from their local machines for a few reasons:

  • This operating system issue
  • Our personal IDIR that we add will create diffs based on our different names (this could be worked around I'm sure with a dev IDIR or some other workaround ie not capturing that part of the screen in the screenshot)
  • In the current state running npx playwright test --update-snapshots seems broken and some tests need to be manually passed using yarn e2e:ui creating further toil
  • If a developer makes front end changes and notices CI fails we lose a lot of time running CI, having to locally run e2e and generate new snapshots and then run CI again you are down almost 45 minutes

Comparing screenshot diffs in CI would work around this and ensure an identical setup each time. Personally I really enjoyed using Happo which we have an issue for #192. We did run into some issues with it but I believe most of them were due to Cypress dom rendering and some of our own flakey tests.

It was nice being able to view details and compare diffs right from GitHub and approve them if there was a failure due to intentional changes. Though if you believe that Playwright screenshots are superior maybe there is a way we could cache the screenshots in CI and test against those when we run e2e in CI.

Anyways thanks for reading my thoughts on this and I would love hear what you think and discuss it further.

https://docs.happo.io/docs/playwright

Edit: Not sure if you have been on a project with Happo but I found a failing example on cas-cif. If you click on the failing Happo job here:
bcgov/cas-cif#1874

https://happo.io/a/336/p/1010/compare/09d3c682ee3ea149a5a0299f3ac6c6a0ab39d337/97e6972c6a5905055aeced8af92d800e91426386

@shon-button
Copy link
Contributor

@marcellmueller
Really appreciate your thoughts and insights.
Perhaps we can do "tete a tete" to discuss together.

@shon-button
Copy link
Contributor

shon-button commented Mar 7, 2024

POC CI Snapshots

  • Store snapshots between CI runs
  • Trigger snapshot updates

How to Run Snapshot Tests in CI/CD with Playwright

Related:
Cache Playwright Binaries...to reduce Playwright test runs in CI
How To Cache Playwright Browser On Github Actions
Is there a way for GitHub Action to Cache the Playwright Browser Binaries?

@marcellmueller
Copy link
Contributor Author

POC CI Snapshots

  • Store snapshots between CI runs
  • Trigger snapshot updates

Nice I just noticed your PR, thanks for playing with this idea. My questions that I didn't consider when I brought up caching the screenshots are how will we approve intentional diffs on failure? It looks like it generates an html report so I think we can view the diffs?

@tmastrom
Copy link

Hi @marcellmueller and @shon-button I'm running into this issue as well! I updated the screenshots using docker but the diffs are still relatively significant (338 pixels, a ratio 0.01 of all image pixels are different). Relevant PR is here I will update you when I figure this out. Say hi to CAS for me 👋

@marcellmueller
Copy link
Contributor Author

Hey @tmastrom! Thanks for this. After some discussion we decided that developers shouldn't have to commit snapshots locally and that it should be handled entirely in CI.

Shon did some exploration with caching Playwright screenshots in CI though ran into some issues and we ended up going with Happo again.

@tmastrom
Copy link

Thanks for the update! I'm looking at chromatic as an alternative to happo but the playwright integration is still in beta so I'm going to hold off for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants