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

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 5 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 (Percy.io).
vanitabarrett marked this conversation as resolved.
Show resolved Hide resolved

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

We use [Percy](https://percy.io/), a visual regression testing tool, to generate and store screenshots of our components to help us check for any unintended visual changes.
vanitabarrett marked this conversation as resolved.
Show resolved Hide resolved

We generate two screenshots for each default example of every component: one with JavaScript enabled and one with JavaScript disabled. Screenshots are not taken for all the different variations of each component. This tool is not a replacement for manual testing.
vanitabarrett marked this conversation as resolved.
Show resolved Hide resolved

The screenshots are public, so they can be checked without logging in. A BrowserStack account is needed to approve or reject any changes. It's the responsibility of the person reviewing the pull request code to approve any visual changes that Percy highlights.
vanitabarrett marked this conversation as resolved.
Show resolved Hide resolved
vanitabarrett marked this conversation as resolved.
Show resolved Hide resolved

When running the tests locally (e.g: via `npm test`), Percy commands are ignored and no screenshots are generated. You will see the following message in your command line output: `[percy] Percy is not running, disabling snapshots`.
vanitabarrett marked this conversation as resolved.
Show resolved Hide resolved

### PRs from forks
When Github Actions is running against a PR from a fork, the Percy secret is not available and therefore no screenshots are generated. Other tests will continue to run as normal. You will see the following messages in the output:
vanitabarrett marked this conversation as resolved.
Show resolved Hide resolved

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

This is the reason why we are unable to make Percy a required check for this repo. However, we should continue to act as if it is required. Percy runs should be approved before merging, and we should treat any failures as blocking.
vanitabarrett marked this conversation as resolved.
Show resolved Hide resolved
Loading