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.

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 Visual Coverage Test + Combined Code Coverage for Coveralls #275

Merged
merged 38 commits into from
Aug 4, 2022

Conversation

yen-tt
Copy link
Contributor

@yen-tt yen-tt commented Aug 3, 2022

Use storybook test-runner and @storybook/addon-coverage to get visual coverage report. The setup requires spinning up storybook before generating the coverage numbers. Now, we have three test scripts:

  • test => combined test coverage
  • test:unit => jest test coverage only
  • test:visual => story book coverage only (note: this only collect coverage on src/components)

Updated coverage github action to also create/update comment which display code coverage percentage for unit, visual, and combined tests

the test-runner only works with jest v27 right now. As nidhi noted in her pr, storybook will be adding support for v28 shortly (storybookjs/test-runner#154).

playwright library is needed in the github action for test-storybook.
Screen Shot 2022-08-03 at 4 51 03 PM

NOTE: The report generated from nyc merge command may not be super accurate after some comparison between to the unit vs visual vs merge report (ex: github issue).

J=SLAP-2269 & SLAP-2270
TEST=manual&auto

  • See that running npm run test:visual generates lcov report in coverage/visual folder. And the report is printed out in terminal.
  • See that a comment is made to the PR about the three coverage percentages.
  • See that coverall percentage increase without changes to tests (increased due to combined test coverage)

@coveralls
Copy link

coveralls commented Aug 3, 2022

Coverage Status

Coverage increased (+0.7%) to 85.126% when pulling bddfa92 on dev/visual-coverage into cd9ea12 on main.

@yen-tt yen-tt changed the title wip Add Visual Coverage Test Aug 3, 2022
@yen-tt yen-tt marked this pull request as ready for review August 3, 2022 21:02
@yen-tt yen-tt requested a review from a team as a code owner August 3, 2022 21:02
@juliannzhou
Copy link
Contributor

license/snyk issue has been resolved with legal approving use of dependency big-integer. Tom also said to disregard security/snyk issue because it is used for testing purposes only.

@yext yext deleted a comment from github-actions bot Aug 4, 2022
@yen-tt yen-tt removed the wip label Aug 4, 2022
@yen-tt yen-tt changed the title Add Visual Coverage Test Add Visual Coverage Test + Combined Code Coverage for Coveralls Aug 4, 2022
@yen-tt
Copy link
Contributor Author

yen-tt commented Aug 4, 2022

license/snyk issue has been resolved with legal approving use of dependency big-integer. Tom also said to disregard security/snyk issue because it is used for testing purposes only.

Will resolve these issues on synk site once this is merge to main.

@oshi97
Copy link
Contributor

oshi97 commented Aug 4, 2022

Current unit coverage is 88.57374392220422% Current visual coverage is 78.16492450638792% Current combined coverage is 88.89789303079417%

Out of curiosity do you know why the get-coverage-percent coverage number is different than the coveralls number? I remember for regular coverage I was seeing a small percent difference and couldn't figure out exactly which numbers we wanted. It looks like the gap is a bit bigger now after including visual tests

@yen-tt
Copy link
Contributor Author

yen-tt commented Aug 4, 2022

get-coverage-percent coverage

I didn't look into the action get-coverage-percent coverage too much (versus how coveralls calculate things), so I'm not too sure. I can investigate for a bit to see if I find anything

@oshi97
Copy link
Contributor

oshi97 commented Aug 4, 2022

I didn't look into the action get-coverage-percent coverage too much (versus how coveralls calculate things), so I'm not too sure. I can investigate for a bit to see if I find anything

if it's a little different it's fine for now just if you happened to know already

.github/workflows/coverage.yml Show resolved Hide resolved
tests/scripts/visual-coverage.sh Outdated Show resolved Hide resolved
tests/scripts/combined-coverage.sh Outdated Show resolved Hide resolved
@nmanu1
Copy link
Contributor

nmanu1 commented Aug 4, 2022

license/snyk issue has been resolved with legal approving use of dependency big-integer. Tom also said to disregard security/snyk issue because it is used for testing purposes only.

in case you haven't already, can you add the newly approved license to our "Approved Licenses" spreadsheet?

@yen-tt yen-tt requested review from oshi97 and nmanu1 August 4, 2022 17:23
tmeyer2115
tmeyer2115 previously approved these changes Aug 4, 2022
tests/scripts/visual-coverage.sh Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
tests/scripts/visual-coverage.sh Show resolved Hide resolved
cp coverage/visual/coverage-storybook.json coverage/merge/coverage-storybook.json

nyc report --reporter=lcov --reporter=text -t coverage/merge --report-dir coverage/merge
cp coverage/merge/lcov.info coverage/lcov.info
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to check my understanding, the coverage check will fail if the combined coverage drops, right? or is it just if the unit test coverage drops?

Copy link
Contributor Author

@yen-tt yen-tt Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the coverage check uses the combined coverage file (I need to copy the combined lcov.info to top level because it's where the coverage github action expects it to be right now) so it would be when the combined coverage drops.

@yen-tt yen-tt merged commit f3c4ad0 into main Aug 4, 2022
@yen-tt yen-tt deleted the dev/visual-coverage branch August 4, 2022 18:33
yen-tt added a commit that referenced this pull request Sep 28, 2022
Use storybook test-runner and `@storybook/addon-coverage` to get visual coverage report. The setup requires spinning up storybook before generating the coverage numbers. Now, we have three test scripts:
- test => combined test coverage
- test:unit => jest test coverage only
- test:visual => story book coverage only (note: this only collect coverage on src/components)

Updated coverage github action to also create/update comment which display code coverage percentage for unit, visual, and combined tests

the test-runner only works with jest v27 right now. As nidhi noted in her [pr](#274), storybook will be adding support for v28 shortly (storybookjs/test-runner#154).

[playwright](https://www.npmjs.com/package/playwright) library is needed in the github action for test-storybook.
<img width="600" alt="Screen Shot 2022-08-03 at 4 51 03 PM" src="https://user-images.githubusercontent.com/36055303/182709211-0189bad6-e978-4f82-9ee4-ba62be350283.png">

NOTE: The report generated from `nyc` merge command may not be super accurate after some comparison between to the unit vs visual vs merge report (ex: [github issue](istanbuljs/nyc#1302)).

J=SLAP-2269 & SLAP-2270
TEST=manual&auto

- See that running `npm run test:visual` generates lcov report in coverage/visual folder. And the report is printed out in terminal.
- See that a comment is made to the PR about the three coverage percentages.
- See that coverall percentage increase without changes to tests (increased due to combined test coverage)
@nmanu1 nmanu1 mentioned this pull request Sep 29, 2022
nmanu1 added a commit that referenced this pull request Sep 30, 2022
Upgrade Jest from v27 to v29. We had [previously](#169) upgraded Jest to v28, but had to [downgrade](#275) to v27 because at the time, `@storybook/test-runner` only supported up to v27. In v0.7 of `@storybook/test-runner`, they changed Jest to be an internal dependency instead of a peer dependency, allowing us to use whatever version of Jest we want.

There was only one breaking change in v29 that affected our code, besides those that already needed to be made when upgrading from v27 to v28 (see previous PR [description](#169 (comment))). This was the `jsdom` upgrade from v19 to v20 in `jest-environment-jsdom`, which requires the `typescript` version to be 4.5 or higher.

With the new support added in v29 and `uuid` v9, we are now able to remove the manual resolving needed in `tests/__setup__/resolver.ts`. For more context on why this was initially needed with Jest v28, see #169.

J=SLAP-2376
TEST=manual

See that existing tests still run properly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants