-
Notifications
You must be signed in to change notification settings - Fork 328
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
Add Percy visual regression testing #2551
Conversation
Nice! Will the Percy screenshots and diffs be open to the public? |
I love this so, so much. Left a comment about findability. |
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.
Exciting stuff! Have you by any chance looked into how many snapshots you think we might end up taking in an average month?
run: npm test | ||
run: node_modules/.bin/percy exec -- npm test | ||
env: | ||
PERCY_TOKEN: ${{ secrets.PERCY_TOKEN }} |
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.
This secret won't be available to PRs from forks, so we'll need to think about how this works for external contributors.
At the very least, we'll need to make sure this fails gracefully (the tests still run and pass) – based on the documentation you added I'm guessing it will still run the tests with '[percy] Percy is not running, disabling snapshots' but would be good to check this.
We'll also need to think about how this works if we aren't able to check for visual regressions in external PRs until after they've been merged.
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 good point. I've checked that behaviour, and you're right, the tests are run without Percy generating snapshots and you see this in the output:
[percy] Skipping visual tests
[percy] Error: Missing Percy token
[percy] Running "npm test"
.... [normal test output here]
I'll add some documentation for that, and have a think about what the process might look like for that..
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.
@36degrees I've been digging into this a bit more, and I think the fact that the checks can't run on a PR from a fork means that we can't make Percy a required check either (it'll never run for PRs from forks, and I can't find a way to manually trigger any kind of Percy build from our side).
I think that means our only option is to not mark Percy as a required check and just be disciplined in trying to review the builds when we review. Can you think of any other alternatives?
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.
It would be significantly more work, but if we wanted to we could explore using the pull_request_target
or workflow_run
events, which run in the context of the base of the pull request (likely main
) and have access to secrets.
A workflow might do something like:
- When a PR is opened, a workflow triggered by the
pull_request
event compile the JS and CSS from that branch and saves them as an artefact - Kick off another workflow using the
workflow_run
event, which runs in the context of the base branch and has access to secrets - That workflow downloads the built JS and CSS and runs the review app using the compiled JS and CSS, then runs the tests to take snapshots
- Pass the context of the original PR to Percy to allow it to associate it with the PR, add the status checks etc.
We'd need to be very careful to ensure that we're not introducing potential vulnerabilities to our workflows – there's a good article on this here: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
My initial thought is we should probably try the simple thing first and explore this route in the future if we find it's causing friction?
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.
This is great and should hopefully give us a bit more confidence that our changes aren't breaking unexpected things.
One concern is that Percy.io appears to be fully broken when JavaScript is disabled, so I could foresee potential issues in reaching and approving changes during patchy internet times (and also JS is just more likely to fail).
Happy to see this being trialled!
@36degrees In terms of estimated snapshots per month, I did do a quick calculation when I raised the PR last week. Each Percy build generates approx 62 snapshots * 2 (browsers we have enabled) = 124 snapshots per run. I think the 62 snapshots already accounts for generating it across 2 screen sizes. Looking at the number of PRs we've had open so far in Feb, and looking at the approx number of pushes, that's around 70 Percy builds in half a month = 8,680. So for a full month we may be looking at around 17,360 snapshots. We get 100,000 screenshots a month (for the whole Percy account). Taking GOV.UK usage into account, which is fairly low, we should still be well under the maximum each month even if the 62 snapshots above doesn't account for screen sizes and/or we have busier months. |
5fe7353
to
c16972f
Compare
This installs the @percy/cli that we need to run our tests using Percy. Follows the Percy documentation https://docs.percy.io/docs/puppeteer This commit does not include setting up our Puppeteer tests to capture any Percy snapshots - this commit installs what we need, ready to go.
Adds a test which takes a snapshot of each component 'default' example.
Enhances the test we added in the previous commit, so that we're now generating snapshots of each component 'default' example when JavaScript is enabled and when it's disabled. We need to make sure each snapshot has a unique name, so we prefix the snapshot name with text to indicate whether it was taken with JavaScript enabled or disabled.
Add documentation to explain how we use Percy, who is responsible for reviewing it, and what screenshots are taken.
c16972f
to
8b09a33
Compare
Thanks @EoinShaughnessy , I think I've addressed those comments if you want to take a second look. |
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.
@vanitabarrett Left one last tiny (optional!) suggestion - but yeah, content looks great!
f005c9e
to
a2fc893
Compare
Part of #2070
What
This PR adds Percy visual regression testing to GOV.UK Frontend. Screenshots are generated for each default example for every component, both with JavaScript enabled and disabled.
Proposal
As discussed in dev catch-up, we generally think visual regression tools may help us catch unintended visual changes when we're working. There were some concerns around hitting screenshot limits, Percy checks being ignored if approvals aren't required (PRs can get rushed through into
main
without checking the screenshots), and generally it not being a replacement for manual testing as it doesn't cover older browsers, high contrast mode etc.I'm proposing a trial period with this PR. I suggest we:
initially make Percy a required checkEdit to add: unfortunately, because PRs from forks don't have access to Github secrets, Percy won't run on these PRs. We therefore can't make it a required check, as it'll never run/pass for these PRs.With all of the above, we should revisit decisions earlier than April if they're causing us issues and slowing down development.
Why
Visual regression tools can help us catch visual changes which we weren't expecting, for example: if we make changes to one area of the codebase, without realising that it will have an effect elsewhere.