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.

Sign up for GitHub

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

Jump to bottom

Add Percy visual regression testing #2551

Merged
merged 6 commits into from
Feb 24, 2022
Merged

Add Percy visual regression testing #2551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
978a5d3
Install @percy/cli and run tests using percy
Feb 24, 2022
b7e1d19
Install @percy/puppeteer and setup tests to capture snapshots
Jan 28, 2022
d246a09
Include snapshots with JavaScript disabled
Feb 18, 2022
c35fb2c
Add documentation for Percy visual regression testing
Feb 18, 2022
8b09a33
Add documentation for PRs from forks
Feb 23, 2022
a2fc893
Docs improvements from review
Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,6 @@ jobs:
run: npm ci

- name: Run tests
run: npm test
run: node_modules/.bin/percy exec -- npm test
env:
PERCY_TOKEN: ${{ secrets.PERCY_TOKEN }}
Copy link
Contributor

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.

Copy link
Contributor Author

@vanitabarrett vanitabarrett Feb 23, 2022 •
edited
Loading

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..

Copy link
Contributor Author

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?

Copy link
Contributor

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:

  1. 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
  2. Kick off another workflow using the workflow_run event, which runs in the context of the base branch and has access to secrets
  3. 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
  4. 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?

22 changes: 21 additions & 1 deletion docs/releasing/testing-and-linting.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Testing and linting

The CI lints SASS and JavaScript, and runs unit and functional tests with Node.
The CI lints SASS and JavaScript, runs unit and functional tests with Node, and generates snapshots for [visual regression testing](https://www.browserstack.com/percy/visual-testing).

## Running all tests locally

Expand Down Expand Up @@ -47,3 +47,23 @@ If a snapshot test fails, review the difference in the console. If the change is
`npm test -- -u src/govuk/components/button`

This will update the snapshot file. Commit this file separately with a commit message that explains you're updating the snapshot file and an explanation of what caused the change.

## Visual regression testing with Percy
vanitabarrett marked this conversation as resolved.
Show resolved Hide resolved
EoinShaughnessy marked this conversation as resolved.
Show resolved Hide resolved

[Percy](https://percy.io/) is a visual regression testing tool. We use it to generate and store screenshots of our components to help us check for any unintended visual changes.

We generate 2 screenshots for each default example of every component. One example has JavaScript enabled, the other has JavaScript disabled. Screenshots are not taken for all the different variations of each component. This tool is not a replacement for manual testing.

The screenshots are public, so you can check them without logging in. A BrowserStack account is needed to approve or reject any changes (if you don't have access, ask your tech lead for help). If you're the reviewer of the pull request code, it's your responsibility to approve or request changes for any visual changes Percy highlights.

When you run the tests locally (for example, using `npm test`), Percy commands are ignored and Percy does not generate any screenshots. You will see the following message in your command line output: `[percy] Percy is not running, disabling snapshots`.

### PRs from forks
When Github Actions is running against a PR from a fork, the Percy secret is not available and Percy does not generate any screenshots. Other tests will continue to run as normal. You will see the following messages in the output:

```
[percy] Skipping visual tests
[percy] Error: Missing Percy token
```

Being unable to run Percy on PRs from forks is the reason we're unable to make Percy a required check for this repo. However, we should continue to act as if a Percy check is required. If the Percy build fails, do not merge the pull request even though GitHub would let you. Continue to review the changes, and approve or reject them as you would normally.
Loading