-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Inital setup of visual regression tests. #209
Inital setup of visual regression tests. #209
Conversation
1e469df
to
e6743c8
Compare
e6743c8
to
3de246a
Compare
Update: this is ready for review now! I have separated out the config from the e2e tests, and added a dedicated script ( I have also added the generated snapshots folder to The tests are currently only running on pages with mostly static content (assuming a dedicated test environment where new content isn't added all the time, and the only things that change with frequency are dashboard contents and update notifications) but we can probably include more pages as long as we explicitly ignore the dynamic elements in them. |
Another thing we'll probably want to add soon is tests for mobile breakpoints; currently we're only testing 1000px wide screens. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what's going on, but I had issues running the tests locally. I used the --puppeteer-interactive
command and it looks like the tests couldn't login, which is unusual:
When I tested manually I didn't notice any issues with the environment. So I don't know what's happening. 😬
Would be great to see some docs on how the tests work (I imagine you have to generate some initial snapshots first and then test against that on a feature branch?) and what options the command takes (can I specify a port for my environment?). Not sure where such docs would go though. 😄
The main README does have a few briefer docs.
On the whole though, it looks like really good progress, just wish I could get it working!
} ); | ||
} | ||
} | ||
await new Promise( ( resolve ) => setTimeout( resolve, 1000 ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative would be page.waitForTimeout( 1000 )
:
https://github.com/puppeteer/puppeteer/blob/v5.5.0/docs/api.md#pagewaitfortimeoutmilliseconds
But was wondering why it's needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The styling changes take a little time to apply, so without the timeout, the screenshot would be taken before the elements were actually hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, page.waitForTimeout
is too new an API for our version of puppeteer 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it might just be page.waitFor
in this version. They deprecated that more recently in favour of separate waitForTimeout
and waitForSelector
functions.
@@ -78,3 +78,6 @@ wp-tests-config.php | |||
|
|||
# Files for local environment config | |||
/docker-compose.override.yml | |||
|
|||
# Visual regression test diffs | |||
tests/visual-regression/specs/__image_snapshots__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how, but I guess this also has to be added to the upstream SVN repo, like an svn:ignore equivalent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good question 😅 hopefully someone knows the answer to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the folder will need to be excluded upstream when committing by updating svn props
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and the changed svn props can also be committed afterwards, if easier. As far as I see svn:ignore __image_snapshots__
will have to be added to the parent dir, tests/visual-regression/specs
.
@tellthemachines, I had a sudden brainwave. I think this might be related to the version of puppeteer in core—it's pretty outdated compared to Gutenberg, and IIRC the major changes were around Thankfully updating should be an easy task as there are no tests to update 😄 Edit: I tried updating to the latest version locally, but still get that issue. |
Hmm, that's funny, it works fine for me locally with
That's the expected workflow with this setup, because testing across different environments is incredibly painful, but that's one of the things I'm looking for feedback on! Agree we should publish some docs when this is ready. Maybe in the handbook testing section? |
The issue also happens without the --puppeteer-interactive flag, I used that to determine what was going wrong. Would be good to get more folks testing to see if it's just me. If it is just me then I know I need to spend some time fixing my dodgy local environment 😄 |
hi @talldan, I just checked this out. The tests are running, producing visual diffs etc. both with & without Let me know if you need any info about my setup for comparison. |
9a3d1f0
to
a0e6e66
Compare
Thanks for mentioning that @danfarrow, could just be me then. |
I tested this and it works great |
Results of running locally. Env:
Attempt 1: First Run - TimeoutWhen first launching
Attempt 2: Applying PR against
|
Update: Steps before running the tests:
Results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is ready. It lays the first baby step towards the ability to do visual regression testing, i.e. first locally and then later through the CI.
🤣 |
938e9d8
to
1847e0c
Compare
1847e0c
to
7d26424
Compare
8158d1d
to
358c3f3
Compare
Results of running locally. Env: I ran these commands:
Then I ran this command
I don't really get the why of this error, as I clear the /node_modules folder and ran the visual test command again, but still got the same error. |
ecea489
to
8b25e11
Compare
8b25e11
to
8ad655b
Compare
8ad655b
to
a4a1293
Compare
Re-ran
Preparing this PR for commit. |
npm build failures seem to be due to the |
Committed via changeset https://core.trac.wordpress.org/changeset/51989. |
Trac ticket: 49606
Adding visual regression tests for wp-admin pages. Uses jest-image-snapshot, which integrates seamlessly with our Jest/Puppeteer setup for e2e tests.
One thing to consider is that these tests are a little slow to run. If we find it's not useful to run them as part of the e2e tests every time, we might want to split them out so they can be run separately.
I have added only snapshots of static pages for now. All pages with dynamic content will need a custom setup where we specify areas of the page where changes should be ignored. (It's pretty straightforward to do, but I thought it better to keep the initial changeset simple.)
If we find the tests are throwing a lot of false positives, we can also fiddle with the resolution until they pass.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.