add visual regression coverage #264

juliannzhou · 2022-07-28T20:55:11Z

This PR adds visual regression coverage tests by using add-on storyshot from storybook. Storyshot renders a test UI with jest upon testing and compares the previously generated jest snapshot from stories with the render.

SLAP=J-2269
TEST=manual

Tests were able to run after executing the following command to show visual regression
npm run coverage:visual for visual-regression coverage

Snyk issues: approved by legal

coveralls · 2022-07-28T21:48:48Z

Coverage increased (+0.5%) to 84.983% when pulling 6a4e520 on dev/visual-regression-test into 67e6aa9 on main.

package.json

tests/components/storyshot.test.ts

nmanu1 · 2022-07-29T13:52:19Z

tests/components/__snapshots__/storyshot.test.ts.snap

@@ -0,0 +1,8687 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP


do we use this snapshot file somewhere? if not and it's just auto-generated, maybe we could remove it

Yes, the storyshot add-on renders generates a jest snapshot from stories. It should be committed so it will compare the future renders in pr with the snapshot as part of the testing. The snapshot file needs to be updated by running the storyshot command and committed every time there's an approved change in code implementation, so we might need a github action to do that. More information here: https://jestjs.io/docs/snapshot-testing

package.json

juliannzhou · 2022-07-29T14:24:37Z

Are we planning on having more than one storyshot test, or is it one storyshot test that tests all components? If it's the latter, would it be a good idea to shorten the command to maybe npm run storyshot?

It's the one storyshot test that tests all components. The npm run storyshot storyshot command is to show the storyshot coverage instead of the regular jest test coverage.

tmeyer2115 · 2022-07-29T15:50:17Z

When we're collecting the coverage, what files under src are we checking? Are we generating coverage numbers for all of the files in src? If so, we shouldn't. We're only explicitly testing the contents of the src/components directory. We don't (and won't) have Storybook tests for the contents of src/models for instance.

…nd move storyshot config to tests folder

This reverts commit 32746ce.

…coverage scripts

jest.unit.config.js

oshi97 · 2022-08-02T17:47:12Z

tests/hooks/useRecentSearches.test.tsx

@@ -1,6 +1,5 @@
 import { useRecentSearches } from '../../src/hooks/useRecentSearches';


(putting the comment here so we can have a comment chain, but asking about storyshot.test.ts.snap) What is storyshot.test.ts.snap?

storyshot.test.ts.snap is a jest snapshot generated with stories using the storyshot add-on and would be used to compare against future renders of test UI in pr. (https://jestjs.io/docs/snapshot-testing) It needs to be updated by running npm run coverage:visual and committed if there is an approved change in implementation for the tests to pass.

Is it possible to use the Percy snapshots instead of a local one? If not, no worries. But, I think we need to include a GH Action that will make a PR updating the Snapshot when appropriate. Without that action, people will inevitably forget to generate a new Snapshot and check it in.

Do we need to add actual snapshot testing here? I thought all we wanted was the coverage, we don't need to see if the snapshot has changed.

If we do need this file and DO need it up to date I would rather generate it on the fly instead of checking it into the repo. I'd like to avoid adding additional noise to the repo if possible. By generate on the fly I mean, as part of running the visual coverage, we would first check out the base branch of the PR, generate storyshot.test.ts.snap, and then checkout the branch with the actual changes

Also is storyshot.test.ts.snap the default name for the file? It feels a little wordy

tests/storyshot.test.ts

…or base coverage test in combined-test pr

package.json

.storybook/main.js

package.json

tmeyer2115 · 2022-08-03T13:52:38Z

tests/hooks/useRecentSearches.test.tsx

@@ -1,6 +1,5 @@
 import { useRecentSearches } from '../../src/hooks/useRecentSearches';


Is it possible to use the Percy snapshots instead of a local one? If not, no worries. But, I think we need to include a GH Action that will make a PR updating the Snapshot when appropriate. Without that action, people will inevitably forget to generate a new Snapshot and check it in.

tmeyer2115 · 2022-08-03T13:54:32Z

tests/hooks/useSynchronizedRequest.test.tsx

@@ -1,5 +1,5 @@
 import { useSynchronizedRequest } from '../../src/hooks/useSynchronizedRequest';
-import { renderHook } from '@testing-library/react-hooks';
+import { renderHook } from '@testing-library/react-hooks/dom';


I checked with a couple other Slapshot folks, we weren't able to see this warning. It may be related to your NPM/Node version. I'd rather we avoid using the dom version of renderHook if possible. Depending on the dom version can lead to other, nuanced issues.

juliannzhou · 2022-08-03T14:18:01Z

Is it possible to use the Percy snapshots instead of a local one? If not, no worries. But, I think we need to include a GH Action that will make a PR updating the Snapshot when appropriate. Without that action, people will inevitably forget to generate a new Snapshot and check it in.

I couldn't find anything on coverage report with Percy snapshot, but there is another option codecov which works in github but not locally and supports visual, jest, and merge coverage reports. We would need to sign up for an account with them to use the service.

oshi97 · 2022-08-03T14:41:57Z

package.json

+    "coverage:visual": "jest --projects tests/jest.visual.config.js --coverage",
+    "coverage:clean": "rm -rf .nyc_output && rm -rf coverage",
+    "coverage:merge": "istanbul-merge --out coverage/merged/coverage-final.json ./coverage/unit/coverage-final.json  ./coverage/visual-regression/coverage-final.json",
+    "coverage:merge-report": "nyc report --reporter=lcov --reporter=text --temp-dir=./coverage/merged --report-dir=./coverage/merged",


For my edification what is coverage:merge-report doing? Which lcov files is it merging together?

merge-report is only displaying the coverage on screen with the merge json file generated with the merge command

oshi97 · 2022-08-03T14:59:42Z

I couldn't find anything on coverage report with Percy snapshot, but there is another option codecov which works in github but not locally and supports visual, jest, and merge coverage reports. We would need to sign up for an account with them to use the service.

I think Codecov does basically the same thing as Coveralls. The reason we ended up choosing Coveralls over Codecov was a security breach that happened a little bit before we started looking at cloud coverage providers https://blog.gitguardian.com/codecov-supply-chain-breach/

yen-tt · 2022-08-03T21:02:11Z

After digging into visual regression coverage test approaches with Julianne, looks like storybook is moving away from storyshots and to @storybook/test-runner with a code coverage generation option. This requires less setup compare to storyshot (no snapshots needed). We made another PR here: #275

Add a GH workflow to check for WCAG violations. If the check fails, the PR will be blocked from merging into main. There are some WCAG violations that we ignore, namely color contrast failures on stories where components are in a "loading" state. Using config options, that rule is disabled for those particular stories. To get the automated WCAG checks, Storybook has to be running while the `npm run wcag` command is run. Note, Jest had to be downgraded to v27 to work with `@storybook/test-runner`. But, it looks like they will be adding support for v28 shortly (storybookjs/test-runner#154), so we should be able to upgrade again soon. In the meantime, we haven't been using any features from v28, so the downgrade didn't affect any tests. Also, the Snyk checks are failing, but the new license and one of new vulnerabilities are both approved by legal in Juliann's [PR](#264 (comment)). The other new vulnerability is also introduced only through Storybook, so we wouldn't be susceptible to it. J=SLAP-2289 TEST=auto See that the check runs as expected on this PR and passes.

Kwun Ying (Juliann) Zhou added 6 commits July 27, 2022 10:20

commit untrack files

3cb129b

require jest local storage mock

8254532

commit untracked files

e3d0e3d

revert previous changes and set up visual regression tests

a1788a0

fix storyshot initialization issues

50fd776

fix renderhook act mismatch warning

95a009f

juliannzhou requested a review from a team as a code owner July 28, 2022 20:55

Kwun Ying (Juliann) Zhou added 9 commits July 28, 2022 17:00

fix styling issue

152116e

remove unnecessary installs

f1bb473

remove unnecessary configs and installs

b2a6e44

remove unnecessary storybook config

d32a2e7

add update snapshot to command

b3a14fd

remove update snapshot command

a85d3ac

remove local snapshots

69f0b91

Merge branch 'main' into dev/visual-regression-test

aba181e

add snapshot

290fe3c

nmanu1 reviewed Jul 29, 2022

View reviewed changes

alextaing reviewed Jul 29, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

Kwun Ying (Juliann) Zhou added 4 commits July 29, 2022 14:12

update command line to only include components in storyshot testing a…

0bed846

…nd move storyshot config to tests folder

Merge branch 'main' into dev/visual-regression-test

ff823a2

update snapshot due to preview.js change in main

fa7001d

add combined coverage

32746ce

juliannzhou changed the title ~~add visual regression coverage tests~~ add visual regression coverage and combined coverage tests Jul 29, 2022

Kwun Ying (Juliann) Zhou added 2 commits July 29, 2022 17:35

sync package-lock

24a4f84

Revert "add combined coverage"

79ca399

This reverts commit 32746ce.

juliannzhou requested review from alextaing and nmanu1 August 1, 2022 14:52

Kwun Ying (Juliann) Zhou added 5 commits August 2, 2022 11:54

change test script name for consistency with other tests in combined …

fe789a0

…coverage scripts

use jest projects to define different configurations for best practice

e84a3a7

correct styling issues

c380dd3

change workflow test script for coverall

2dc9a0e

correct test script in workflow

768e7d6

juliannzhou requested review from tmeyer2115 and yen-tt August 2, 2022 17:08

juliannzhou added the ready for review again label Aug 2, 2022

oshi97 reviewed Aug 2, 2022

View reviewed changes

update jest config files with a base config and update package.json f…

0b6c17c

…or base coverage test in combined-test pr

tmeyer2115 reviewed Aug 2, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

nmanu1 mentioned this pull request Aug 3, 2022

Add WCAG PR check #274

Merged

Kwun Ying (Juliann) Zhou added 6 commits August 2, 2022 22:20

change directory and use base config

8aa7c01

change coverage directory

df35d98

use require

d51ebe2

fix styling and change coverage file path

770238e

correct coverage file path

46170c4

change coverage directory

868d39c

tmeyer2115 reviewed Aug 3, 2022

View reviewed changes

remove dom from import and edit stories test match

f8ad154

oshi97 reviewed Aug 3, 2022

View reviewed changes

script clean up

6a4e520

yen-tt closed this Aug 3, 2022

juliannzhou removed the ready for review again label Aug 3, 2022

juliannzhou deleted the dev/visual-regression-test branch August 18, 2022 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add visual regression coverage #264

add visual regression coverage #264

juliannzhou commented Jul 28, 2022 •

edited

coveralls commented Jul 28, 2022 •

edited

nmanu1 Jul 29, 2022

juliannzhou Jul 29, 2022 •

edited

juliannzhou commented Jul 29, 2022

tmeyer2115 commented Jul 29, 2022

oshi97 Aug 2, 2022

juliannzhou Aug 2, 2022 •

edited

tmeyer2115 Aug 3, 2022

oshi97 Aug 3, 2022 •

edited

oshi97 Aug 3, 2022

tmeyer2115 Aug 3, 2022

tmeyer2115 Aug 3, 2022

juliannzhou commented Aug 3, 2022

oshi97 Aug 3, 2022

juliannzhou Aug 3, 2022

oshi97 commented Aug 3, 2022

yen-tt commented Aug 3, 2022

		@@ -0,0 +1,8687 @@
		// Jest Snapshot v1, https://goo.gl/fbAQLP

		@@ -1,6 +1,5 @@
		import { useRecentSearches } from '../../src/hooks/useRecentSearches';

add visual regression coverage #264

add visual regression coverage #264

Conversation

juliannzhou commented Jul 28, 2022 • edited

coveralls commented Jul 28, 2022 • edited

Choose a reason for hiding this comment

juliannzhou Jul 29, 2022 • edited

Choose a reason for hiding this comment

juliannzhou commented Jul 29, 2022

tmeyer2115 commented Jul 29, 2022

Choose a reason for hiding this comment

juliannzhou Aug 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshi97 Aug 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliannzhou commented Aug 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshi97 commented Aug 3, 2022

yen-tt commented Aug 3, 2022

juliannzhou commented Jul 28, 2022 •

edited

coveralls commented Jul 28, 2022 •

edited

juliannzhou Jul 29, 2022 •

edited

juliannzhou Aug 2, 2022 •

edited

oshi97 Aug 3, 2022 •

edited