fides icon indicating copy to clipboard operation
fides copied to clipboard

Add automated E2E code coverage for client applications

Open NevilleS opened this issue 1 year ago • 5 comments

Description Of Changes

Experimenting with using istanbul, @cypress/code-coverage, and codecov together to pull automated coverage reports for the client codebases 👍

Code Changes

  • [X] Add istanbul plugin to admin-ui and privacy-center projects, but only enable it in "test" builds used for Cypress (ie. cy:start)
  • [X] Use Cypress' code-coverage plugin to publish coverage reports while running specs
  • [X] Update GitHub CI (frontend_checks.yml) to collect coverage reports and upload to CodeCov
  • [X] Modify CodeCov actions to tag pytest coverage with a "backend" flag
  • [X] Modify CodeCov configuration with separate coverage targets for "backend" vs "admin-ui" vs "privacy-center"

Steps to Confirm

  • [X] View the CodeCov checks attached to this PR: image

Pre-Merge Checklist

NevilleS avatar Jan 20 '24 23:01 NevilleS

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 29, 2024 10:40pm

vercel[bot] avatar Jan 20 '24 23:01 vercel[bot]

Passing run #6098 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 30a866a7effcaaac8c97d6b96789cdc783e4dca6 into acc2541a03e872c1dce86ec8cf5a...
Project: fides Commit: 8fe49616b0 ℹ️
Status: Passed Duration: 00:34 💡
Started: Jan 29, 2024 10:49 PM Ended: Jan 29, 2024 10:50 PM

Review all test suite changes for PR #4563 ↗︎

cypress[bot] avatar Jan 20 '24 23:01 cypress[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.52%. Comparing base (13a040e) to head (30a866a). Report is 657 commits behind head on main.

:exclamation: There is a different number of reports uploaded between BASE (13a040e) and HEAD (30a866a). Click for more details.

HEAD has 12 uploads less than BASE
Flag BASE (13a040e) HEAD (30a866a)
12 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4563      +/-   ##
==========================================
- Coverage   86.79%   78.52%   -8.28%     
==========================================
  Files         329      824     +495     
  Lines       19816    32088   +12272     
  Branches     2546     6215    +3669     
==========================================
+ Hits        17199    25196    +7997     
- Misses       2150     6361    +4211     
- Partials      467      531      +64     
Flag Coverage Δ
admin-ui 64.65% <ø> (?)
backend 86.76% <ø> (?)
privacy-center 69.62% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 21 '24 00:01 codecov[bot]

Amazing! A few comments:

  • Do you know if there's a way to also instrument our unit tests and combine coverage with cypress? We have some unit tests—not a lot, but some that cover nitty gritty logic of helper funcs—which would be nice to have covered as well. Though again, there aren't that many (more so in fides.js) so I don't think this is a high priority

Yes, that's possible. For the pytest coverage we upload multiple reports to codecov and it aggregates it all together - although I find that kinda hard to "grok". A simpler way would be to run the unit tests & cypress tests from the same job so that the coverage report would aggregate locally on the GitHub runner and then upload together.

  • I wonder if this can work to cover fides.js at all, given that its tests are in a different folder....

I tried really briefly to do this. It was easy enough to add rollup-plugin-istanbul to fides.js, but had a couple issues:

  • it immediately explodes the bundle size and our build pipeline was very upset with me
  • coverage reports didn't work when I ran Cypress tests from /clients/privacy-center; I have to assume it was confused by directories saying /clients/fides-js or something

Overall I punted on it pretty quickly.

  • I never added tests to the Regulations page (I know, I know! I'm a monster, but time was short and I'll do it as part of https://ethyca.atlassian.net/browse/PROD-1563 😄 ). it uses many of the same components as the Locations page, but I noticed that regulations.tsx doesn't show up in the code coverage report at all, probably because no cypress test visits it. So just flagging that our % may be inflated if there are parts of the app that aren't covered, but are also never visited by cypress, it looks like they just won't be included, as opposed to lowering our %

Interesting... I'd have hoped that coverage reports should include all files, especially those that aren't exercised by tests. I wonder if this might be caused less by the lack of a Cypress test and more if cy:start doesn't force those files to actually get built.

  • overall, great increment! :shipit:

😄

NevilleS avatar Jan 29 '24 21:01 NevilleS

Just pushed an update that'll now pull all the source files into the coverage report, and fully untested files show up as semi-transparent entries with 0% coverage: image

However, it's still misleading - Istanbul doesn't include these untested files in the total coverage statistics, which you can basically see in that screenshot above... it's showing "3/14 lines" covered but that's only because it's not including any of the untested files and all their LoC! This isn't just us, it seems like an open issue with Istanbul in general: https://github.com/istanbuljs/nyc/issues/1415

...overall I hesitate to merge something in that is actively misleading because it doesn't include untested files... 🤔.

NevilleS avatar Jan 29 '24 22:01 NevilleS