Make WordPress Core

Opened 23 months ago

Closed 3 months ago

Last modified 6 weeks ago

#49606 closed enhancement (fixed)

Add visual regression testing to core

Reported by: isabel_brison Owned by: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch commit
Focuses: ui, css Cc:

Description (last modified by SergeyBiryukov)

This idea came up during discussion about the upcoming CSS audit #49582. If we have a suite of visual regression tests in place, it will give us confidence to make changes to existing CSS without breaking anything.

What tools to use?

We're in the process of adding e2e tests to core: #49507 and these are using Puppeteer and Jest, so it would be nice for visual regression testing to follow along the same lines. There are a few options to look into:

Change History (53)

#1 @SergeyBiryukov
23 months ago

  • Description modified (diff)

This ticket was mentioned in Slack in #core-js by aduth. View the logs.


23 months ago

#3 @netweb
23 months ago

This comment is really about me subscribing to this ticket for notification to be honest, but I am keen to see the results of https://github.com/americanexpress/jest-image-snapshot testing

#4 @isabel_brison
23 months ago

Update: discussion in #core-js https://wordpress.slack.com/archives/C5UNMSU4R/p1583849715154400 revealed there's a PR in Gutenberg that explores adding visual regression testing with percy.io https://github.com/WordPress/gutenberg/pull/18797

Worth investigating if this is a viable option for us, given it's a commercial service. The main advantage would be not having to store the test snapshots in our repo, as they might be quite large.

#5 @netweb
23 months ago

Thanks @isabel_brison, I’ll take a read of that later, and sure repo size of storing visual snapshots might be an issue.

What I’m hoping for is that visual regression testing by way of Jest can be extended into @wordpress/scripts package so that developers using the existing integration and e2e already offered in that package can also benefit from this also.

I know a lot of the projects and devs I work with are all considering how best to add visual regression testing right now, so being able to do implement that without the use of a commercial service would be beneficial for the wider community

This ticket was mentioned in Slack in #core-css by isabelbrison. View the logs.


23 months ago

This ticket was mentioned in Slack in #design by notlaura. View the logs.


23 months ago

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


22 months ago

This ticket was mentioned in PR #209 on WordPress/wordpress-develop by tellthemachines.


22 months ago

<!--
Hi there! Thanks for contributing to WordPress!

Pull Requests in this GitHub repository must be linked to a ticket in the WordPress Core Trac instance (https://core.trac.wordpress.org), and are only used for code review. No pull requests will be merged on GitHub.

See the WordPress Handbook page on using PRs for Code Review more information: https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/

If this is your first time contributing, you may also find reviewing these guides first to be helpful:

-->

Trac ticket: [49606](https://core.trac.wordpress.org/ticket/49606)

Adding visual regression tests for wp-admin pages. Uses [jest-image-snapshot](https://github.com/americanexpress/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 ticket was mentioned in Slack in #core-css by isabelbrison. View the logs.


22 months ago

#11 @isabel_brison
18 months ago

Update on this: I did some experiments on the linked GitHub PR and concluded it's virtually impossible for the tests to pass when comparing snapshots generated locally vs on Travis. This is probably due to resolution differences, but I tried fiddling with all the variables provided and no luck.

Also, because these tests would mostly be useful when refactoring CSS, I'm not convinced we should run them as part of the Core e2e test suite. So my idea is we could have a separate setup script that would allow anyone doing refactoring work to run them locally as needed.

This ticket was mentioned in Slack in #core-css by notlaura. View the logs.


15 months ago

This ticket was mentioned in Slack in #core-css by notlaura. View the logs.


15 months ago

This ticket was mentioned in Slack in #core-css by isabelbrison. View the logs.


15 months ago

This ticket was mentioned in Slack in #core-css by notlaura. View the logs.


15 months ago

This ticket was mentioned in Slack in #core-css by ryelle. View the logs.


14 months ago

This ticket was mentioned in Slack in #core-css by notlaura. View the logs.


13 months ago

#18 @prbot
13 months ago

tellthemachines commented on PR #209:

Update: this is ready for review now!

I have separated out the config from the e2e tests, and added a dedicated script (test:visual) so we can run these tests independently, and they're not automatically part of CI checks for all code changes.

I have also added the generated snapshots folder to .gitignore, so that snapshots are only stored locally. The tests don't really work cross-environment as even tiny differences between envs throw up too many false positives. If we do want to run them on CI at any point, we can set them up to run first on trunk to generate the snapshots, and then diff with the feature branch. That also solves the problem of storing huge .png files in the repo 😄

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.

#19 @prbot
13 months ago

tellthemachines commented on PR #209:

Another thing we'll probably want to add soon is tests for mobile breakpoints; currently we're only testing 1000px wide screens.

This ticket was mentioned in Slack in #core-css by notlaura. View the logs.


13 months ago

This ticket was mentioned in Slack in #core-css by notlaura. View the logs.


12 months ago

#22 @prbot
12 months ago

talldan commented on PR #209:

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:

@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 page.waitForNavigation.

Thankfully updating should be an easy task as there are no tests to update 😄

#23 @prbot
12 months ago

tellthemachines commented on PR #209:

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:

Hmm, that's funny, it works fine for me locally with npm run test:visual -- --puppeteer-interactive. Can you get it working ok without the interactive flag?

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?)

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?

#24 @prbot
12 months ago

talldan commented on PR #209:

Hmm, that's funny, it works fine for me locally with npm run test:visual -- --puppeteer-interactive. Can you get it working ok without the interactive flag?

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 😄

#25 @prbot
12 months ago

danfarrow commented on PR #209:

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 smile

hi @talldan, I just checked this out. The tests are running, producing visual diffs etc. both with & without --puppeteer-interactive.

Let me know if you need any info about my setup for comparison.

#26 @prbot
12 months ago

talldan commented on PR #209:

Thanks for mentioning that @danfarrow, could just be me then.

This ticket was mentioned in Slack in #core-css by notlaura. View the logs.


12 months ago

This ticket was mentioned in Slack in #core-css by notlaura. View the logs.


11 months ago

This ticket was mentioned in Slack in #core-css by notlaura. View the logs.


11 months ago

#30 @prbot
11 months ago

draganescu commented on PR #209:

I tested this and it works great

#31 @andraganescu
11 months ago

What should we do here to move this forward? As a setup it seems to function. More tests will have to be written for it, as this only adds the possibility for visual regression testing, and also the logic to test against trunk has to be added in CI on GH actions.

Could we land this so that we can move on with building on it? :)

#32 @prbot
10 months ago

hellofromtonya commented on PR #209:

Results of running locally.

Env:

  • Using Docker image
  • OS: Mac Big Sur
  • Started freshed
    • Cleared Docker cache
    • Removed node_modules and cleared its cache
    • Removed vendor and cleared its cache
    • Ran:
      npm install
      composer install
      npm run build
      npm run build:dev
      npm run env:start
      npm run env:install
      npm run test:visual
      

### Attempt 1: First Run - Timeout

When first launching npm run test:visual (with PR and against master), fails for timeout:

 FAIL  tests/visual-regression/specs/visual-snapshots.test.js (62.582s)
  Admin Visual Snapshots
    ✕ All Posts (5007ms)
    ✓ Categories (2581ms)
    ✓ Tags (2561ms)
    ✓ Media Library (2670ms)
    ✓ Add New Media (2525ms)
    ✓ All Pages (2545ms)
    ✓ Comments (2646ms)
    ✓ Widgets (2972ms)
    ✓ Menus (2611ms)
    ✓ Plugins (2694ms)
    ✓ All Users (2571ms)
    ✓ Add New User (2628ms)
    ✓ Your Profile (2809ms)
    ✓ Available Tools (2461ms)
    ✓ Import (2692ms)
    ✓ Export (2495ms)
    ✓ Export Personal Data (2483ms)
    ✓ Erase Personal Data (2484ms)
    ✓ Reading Settings (2546ms)
    ✓ Discussion Settings (2697ms)
    ✓ Media Settings (2553ms)
    ✓ Privacy Settings (2490ms)

  ● Admin Visual Snapshots › All Posts

    : Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Error:

      42 | 	} );
      43 |
    > 44 | 	it( 'All Posts', async () => {
         | 	^
      45 | 		await visitAdminPage( '/edit.php' );
      46 | 		await hideElementVisibility( elementsToHide );
      47 | 		await removeElementFromLayout( elementsToRemove );

      at new Spec (../../node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
      at Suite.<anonymous> (specs/visual-snapshots.test.js:44:2)

 › 22 snapshots written.
Snapshot Summary
 › 22 snapshots written from 1 test suite.

Test Suites: 1 failed, 1 total
Tests:       1 failed, 21 passed, 22 total
Snapshots:   22 written, 22 total
Time:        62.789s, estimated 63s

### Attempt 2: Applying PR against master

  • Env: Applied the PR's against master and then rebuilding
  • Result: Categories and Your Profile tests fail for image size
  Admin Visual Snapshots
    ✓ All Posts (3820 ms)
    ✕ Categories (2892 ms)
    ✓ Tags (2662 ms)
    ✓ Media Library (2769 ms)
    ✓ Add New Media (2618 ms)
    ✓ All Pages (2630 ms)
    ✓ Comments (2749 ms)
    ✓ Widgets (3202 ms)
    ✓ Menus (2699 ms)
    ✓ Plugins (2631 ms)
    ✓ All Users (2607 ms)
    ✓ Add New User (2741 ms)
    ✕ Your Profile (3444 ms)
    ✓ Available Tools (2588 ms)
    ✓ Import (2657 ms)
    ✓ Export (2600 ms)
    ✓ Export Personal Data (2594 ms)
    ✓ Erase Personal Data (2596 ms)
    ✓ Reading Settings (2652 ms)
    ✓ Discussion Settings (3065 ms)
    ✓ Media Settings (2655 ms)
    ✓ Privacy Settings (2610 ms)

  ● Admin Visual Snapshots › Categories

    Expected image to be the same size as the snapshot (1000x750), but was different (1000x853).
    See diff for details: .../wordpress-develop/tests/visual-regression/specs/__image_snapshots__/__diff_output__/visual-snapshots-test-js-admin-visual-snapshots-categories-1-diff.png

      55 | 		await removeElementFromLayout( elementsToRemove );
      56 | 		const image = await page.screenshot( screenshotOptions );
    > 57 | 		expect( image ).toMatchImageSnapshot();
         | 		                ^
      58 | 	} );
      59 |
      60 | 	it( 'Tags', async () => {

      at Object.<anonymous> (specs/visual-snapshots.test.js:57:19)

  ● Admin Visual Snapshots › Your Profile

    Expected image to be the same size as the snapshot (1000x2161), but was different (1000x2182).
    See diff for details: ../wordpress-develop/tests/visual-regression/specs/__image_snapshots__/__diff_output__/visual-snapshots-test-js-admin-visual-snapshots-your-profile-1-diff.png

      146 | 		await removeElementFromLayout( elementsToRemove );
      147 | 		const image = await page.screenshot( screenshotOptions );
    > 148 | 		expect( image ).toMatchImageSnapshot();
          | 		                ^
      149 | 	} );
      150 |
      151 | 	it( 'Available Tools', async () => {

      at Object.<anonymous> (specs/visual-snapshots.test.js:148:19)

 › 2 snapshots failed.
Snapshot Summary
 › 2 snapshots failed from 1 test suite. Inspect your code changes or re-run jest with `-u` to update them.

Test Suites: 1 failed, 1 total
Tests:       2 failed, 20 passed, 22 total
Snapshots:   2 failed, 22 passed, 24 total
Time:        62.336 s, estimated 104 s

### Attempt 3: When pulling and checking out PR

  • Env: Pulled and checked out the PR and then rebuilt
  • Result: Categories and Your Profile tests fail for image size (same as Attempt 2).

#33 @prbot
10 months ago

hellofromtonya commented on PR #209:

Update:

Steps before running the tests:

  • destroyed the docker instance
  • cleaned docker's cache/data
  • restarted docker
  • then walked through the steps above

Results:

  • First try: timeout (same as Attempt 1 above)
  • Second try: passed 🎉

#34 @hellofromTonya
10 months ago

  • Keywords has-patch commit added
  • Milestone changed from Awaiting Review to 5.8

PR209 lays the first baby step towards the ability to do visual regression testing, i.e. first locally and then later through the CI. It's a starting point for the foundational pieces. Marking it for commit consideration.

Core committer note:

The artifacts folder, i.e. tests/visual-regression/specs/__image_snapshots__, excluded upstream when committing by updating svn props.

#35 @prbot
10 months ago

danfarrow commented on PR #209:

Expected image to match or be a close match... but was 0.0018458197611292075% different

🤣

#36 @isabel_brison
10 months ago

Ok, I think this is ready to go. I've added a readme with basic instructions on how to run the tests locally.

Next steps:

  • Decide if/when we should run these tests on CI (they're pretty slow so we shouldn't run them for any changes that don't affect visual rendering of the admin pages)
  • Write tests for the remaining pages on desktop breakpoint
  • Run tests on mobile breakpoint as well (may need some tweaks, especially to elements being hidden before the snapshot is taken)

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


10 months ago

#38 @prbot
10 months ago

JustinyAhin commented on PR #209:

Results of running locally.

Env:

Fresh WordPress Develop fork
Using Docker image
OS: Mac Big Sur


I ran these commands:

npm install
composer install
npm run build
npm run build:dev
npm run env:start
npm run env:install

Then I ran this command npm run test:visual but got a bunch of errors:

Error: Cannot find module 'puppeteer'
Require stack:
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/jest-environment-puppeteer/lib/readConfig.js
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/jest-environment-puppeteer/lib/global.js
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/jest-environment-puppeteer/setup.js
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/@jest/transform/build/ScriptTransformer.js
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/@jest/transform/build/index.js
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/@jest/core/node_modules/jest-runtime/build/index.js
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/@jest/core/build/cli/index.js
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/@jest/core/build/jest.js
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/jest/node_modules/jest-cli/build/cli/index.js
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/jest/node_modules/jest-cli/build/index.js
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/jest/build/jest.js
- /Users/segbedji/dev/wp/wordpress-develop/node_modules/@wordpress/scripts/scripts/test-e2e.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:815:15)
    at Function.Module._load (internal/modules/cjs/loader.js:667:27)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at getPuppeteer (/Users/segbedji/dev/wp/wordpress-develop/node_modules/jest-environment-puppeteer/lib/readConfig.js:58:14)
    at setup (/Users/segbedji/dev/wp/wordpress-develop/node_modules/jest-environment-puppeteer/lib/global.js:21:50)
    at async /Users/segbedji/dev/wp/wordpress-develop/node_modules/@jest/core/build/runGlobalHook.js:82:11
    at async waitForPromiseWithCleanup (/Users/segbedji/dev/wp/wordpress-develop/node_modules/@jest/transform/build/ScriptTransformer.js:207:5)
    at async /Users/segbedji/dev/wp/wordpress-develop/node_modules/@jest/core/build/runGlobalHook.js:72:9
    at async pEachSeries (/Users/segbedji/dev/wp/wordpress-develop/node_modules/p-each-series/index.js:8:23)
child_process.js:674
    throw err;
    ^

Error: Command failed: wp-scripts test-e2e --config tests/visual-regression/jest.config.js 
    at checkExecSyncError (child_process.js:635:11)
    at execSync (child_process.js:671:15)
    at Object.<anonymous> (/Users/segbedji/dev/wp/wordpress-develop/tests/visual-regression/run-tests.js:9:1)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47 {
  status: 1,
  signal: null,
  output: [ null, null, null ],
  pid: 68953,
  stdout: null,
  stderr: null
}
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! WordPress@5.8.0 test:visual: `node ./tests/visual-regression/run-tests.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the WordPress@5.8.0 test:visual script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/segbedji/.npm/_logs/2021-04-08T16_01_53_408Z-debug.log

I don't really get the why of this error, as npm install worked correctly initially.

I clear the /node_modules folder and ran the visual test command again, but still got the same error.

#39 @desrosj
8 months ago

  • Keywords needs-refresh added; commit removed
  • Milestone changed from 5.8 to Future Release

It looks like the PR will need a refresh as it no longer applies cleanly to trunk. I'm going to punt this to Future Release for now as the focus is on full site editing.

That said, if someone has the time to refresh the PR and review after the feature freeze deadline (25 May), it can be moved back and reconsidered. Because this is a test related enhancement, this can be added at any point of the release cycle (except during RC).

#40 @isabel_brison
8 months ago

Thanks for the heads up @desroj, PR refreshed!

#41 @isabel_brison
8 months ago

  • Keywords needs-refresh removed

#42 @isabel_brison
8 months ago

This patch will need refreshing every time an npm package changes in trunk, as that causes conflicts in the package-lock file. So the best approach to save time and effort is to refresh it only when we're about to commit it. I'm happy to do the refresh (it's just a matter of rebasing and re-running npm install but can't commit so please feel free to ping me before committing so I can refresh the patch.

This ticket was mentioned in Slack in #core-css by notlaura. View the logs.


8 months ago

#44 @hellofromTonya
3 months ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 5.9

Pulling this into 5.9 as it's part of the Test Roadmap.

In talking with @isabel_brison more about this, the idea is to introduce the ability to run visual regression locally, not as part of the CI. This first step is an experiment to learn.

  • From these learnings, plans can be crafted for how to build it into an automated CI process.
  • The challenges for the CI are storage of the artifacts and unreliability of testing these across different environments. A third party service may be possibility to explore in the future.

I think PR 209 can be committed once it's refreshed and ready. @isabel_brison can you do a refresh? And then when done, please mark commit and remove needs-refresh. Then I'll get it committed.

#45 @hellofromTonya
3 months ago

  • Owner set to hellofromTonya
  • Status changed from new to accepted

#46 @isabel_brison
3 months ago

  • Keywords commit added; needs-refresh removed

#47 @prbot
3 months ago

hellofromtonya commented on PR #209:

Re-ran npm run test:visual locally and still passes.

 PASS  tests/visual-regression/specs/visual-snapshots.test.js (56.001 s)
  Admin Visual Snapshots
    ✓ All Posts (4034 ms)
    ✓ Categories (2343 ms)
    ✓ Tags (2300 ms)
    ✓ Media Library (2395 ms)
    ✓ Add New Media (2349 ms)
    ✓ All Pages (2316 ms)
    ✓ Comments (2337 ms)
    ✓ Widgets (3819 ms)
    ✓ Menus (2425 ms)
    ✓ Plugins (2507 ms)
    ✓ All Users (2312 ms)
    ✓ Add New User (2353 ms)
    ✓ Your Profile (2447 ms)
    ✓ Available Tools (2294 ms)
    ✓ Import (2486 ms)
    ✓ Export (2298 ms)
    ✓ Export Personal Data (2298 ms)
    ✓ Erase Personal Data (2299 ms)
    ✓ Reading Settings (2300 ms)
    ✓ Discussion Settings (2409 ms)
    ✓ Media Settings (2290 ms)
    ✓ Privacy Settings (2299 ms)

 › 22 snapshots written.
Snapshot Summary
 › 22 snapshots written from 1 test suite.

Test Suites: 1 passed, 1 total
Tests:       22 passed, 22 total
Snapshots:   22 written, 22 total
Time:        56.062 s

Preparing this PR for commit.

#48 @prbot
3 months ago

hellofromtonya commented on PR #209:

npm build failures seem to be due to the package-lock.json changes. As an experiment (done in PR #1803), I reverted that file, deleted the node_modules folder, and then did npm install to regenerate the lock file with the package deps added by this PR. npm builds passed 🎉

#49 @hellofromTonya
3 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 51989:

Build/Test Tools: Introduce local visual regression testing.

Adds the ability to locally run visual regression testing for wp-admin pages via npm run test:visual. Snapshots are stored on contributors' local machines.

Note:
Wiring to the CI is not included. Why? The challenges for the CI are storage of the artifacts and unreliability of testing these across different environments.

This commit is a first step towards visual regression testing. Running it locally provides a learning opportunity which could help to craft how to build it into the automated CI process.

Props isabel_brison, andraganescu, azaozz, danfarrow, desrosj, hellofromTonya, justinahinon, netweb, talldanwp.
Fixes #49606.

#51 @hellofromTonya
3 months ago

Thank you to everyone who contributed!

Visual regression testing is now available to run on your local machine via npm run test:visual. The snapshots are stored locally in the tests/visual-regression/specs/__image_snapshots_ folder.

Happy testing!

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-css by notlaura. View the logs.


6 weeks ago

Note: See TracTickets for help on using tickets.