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 combined Jest+Cypress code coverage reports #5262

Merged
merged 9 commits into from
Oct 20, 2021

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Oct 12, 2021

Summary

While working on #5261, I wanted to calculate combined coverage to make sure I wasn't losing any uncovered lines when converting the focus behavior tests to Cypress. This PR adds a Cypress code coverage plugin and then a yarn combine-test-coverage script that combines our Cypress+Jest output into a single HTML report. It additionally adds wiki documentation for how to access our HTML code coverage reports.

I recommend following along by commit, as I added some misc. cleanup as part of this PR as well.

Example

I cherry-picked #5261 into this branch locally to get an example of how updating a subsection of Jest tests to Cypress would affect final combined coverage. Coverage remained approximately the same (with a 1% increase from adding Cypress!)

Before with just Jest coverage

90.41%

After

Jest coverage:
82.19%

Cypress coverage:
67.12%

Combined coverage:
91.78%

QAing locally

  • checkout this branch
  • yarn to install new dependencies
  • Run yarn test-unit --coverage to generate Jest coverage reports
  • Run yarn test-cypress which will automatically generate Cypress coverage reports
  • Run yarn combine-test-coverage, which should automatically open the main coverage report HTML page in your browser

Resources

Reference links to the resources I used while I was setting this up:

  • Cypress code coverage reports: [1], [2]
  • Jest+Cypress combined coverage reports: [1]
  • Storyboook+Jest+Cypress combined coverage, if we're ever interested in that someday: [1]

Checklist

N/A, internal/dev-only change

- Ignore new `.nyc_output` dir generated by cypress/istanbul

- Improve Jest config collectCoverageFrom !ignores:
  - DRY out component+services folder patterns with a {,} glob
  - Ignore new Cypress .spec files and .testenv files for jest coverage

- eslintignore the cypress folder, since they're all .js anyway and it has a decent amount of idiosyncracies
+ cleanup unused eslint-disable
+ cleanup cypress comment boilerplate
+ remove unused paths from webpack config
@cee-chen cee-chen force-pushed the cypress-code-coverage branch from ff885b1 to 6716e63 Compare October 12, 2021 20:48
package.json Outdated Show resolved Hide resolved
scripts/combine-coverage.js Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5262/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM, looks like we want to pull the nyc config to a js file before merging?

@cee-chen cee-chen enabled auto-merge (squash) October 20, 2021 15:39
@thompsongl
Copy link
Contributor

I ran yarn combine-test-coverage locally and it errored out. So are yarn test-unit --coverage and yarn test-cypress prerequisites (it worked after running both)?
Any thought on having yarn combine-test-coverage run the prereqs also? Or adding another command that will run all three?

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5262/

@cee-chen
Copy link
Member Author

cee-chen commented Oct 20, 2021

I ran yarn combine-test-coverage locally and it errored out. So are yarn test-unit --coverage and yarn test-cypress prerequisites (it worked after running both)?

Yes, that's documented in both in the new wiki docs I added and the script itself:

# NOTE: `yarn test-cypress` and `yarn test-unit --coverage` must have already been run
# in order for the correct coverage JSON files to exist and be combined

Any thought on having yarn combine-test-coverage run the prereqs also?

Not realistically. That would literally require running every single jest test and cypress test which would result in the command potentially taking over 30 minutes. All the combine script does is combine the existing coverage reports that already exist in your local reports/ folder.

Additionally, making the command agnostic allows devs to only run coverage for specific tests that they're developing on (e.g. if you're working on just the focus_trap component, you'd do yarn test-unit focus_trap --coverage && yarn test-cypress --spec focus_trap.spec.tsx && yarn combine-coverage and then you would obtain coverage for only the component you care about without wasting extra time.

It's fairly manual, but this PR at least gets us in the right direction. IMO, I think the most helpful dev QOL step from here would be to add some kind of --watch flag using nodemon or other watch plugin, and dynamically respond/update the combined coverage whenever reports/jest-coverage/coverage-final.json and reports/cypress-coverage/coverage-final.json change.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification! It would help if I read the docs before trying to use the thing 😅

@cee-chen cee-chen merged commit f69e88d into elastic:master Oct 20, 2021
@cee-chen cee-chen deleted the cypress-code-coverage branch October 20, 2021 16:21
@cee-chen
Copy link
Member Author

No worries!! Also whoops, I just realized I mashed auto merge without an actual approval, super sorry y'all 🙈 I'm totally out of wack this morning and somehow mistook Chandler's comment for an approval, my bad!

@thompsongl
Copy link
Contributor

My last comment was an approval! All good ✅

chandlerprall pushed a commit to chandlerprall/eui that referenced this pull request Oct 26, 2021
* Install Cypress code coverage dependencies

* Set up required babel+istanbul config

* Set up Cypress code coverage config

@see https://github.com/cypress-io/code-coverage#instrument-unit-tests

* Add script for combining cypress & jest code coverage
-into a single json & html report

* Misc cleanup/ignores

- Ignore new `.nyc_output` dir generated by cypress/istanbul

- Improve Jest config collectCoverageFrom !ignores:
  - DRY out component+services folder patterns with a {,} glob
  - Ignore new Cypress .spec files and .testenv files for jest coverage

- eslintignore the cypress folder, since they're all .js anyway and it has a decent amount of idiosyncracies
+ cleanup unused eslint-disable
+ cleanup cypress comment boilerplate
+ remove unused paths from webpack config

* Add wiki documentation

* [PR feedback] Move nyc config to own file

* [PR feedback] Switch combine coverage script to .sh
ym pushed a commit to ym/eui that referenced this pull request Oct 29, 2021
* Install Cypress code coverage dependencies

* Set up required babel+istanbul config

* Set up Cypress code coverage config

@see https://github.com/cypress-io/code-coverage#instrument-unit-tests

* Add script for combining cypress & jest code coverage
-into a single json & html report

* Misc cleanup/ignores

- Ignore new `.nyc_output` dir generated by cypress/istanbul

- Improve Jest config collectCoverageFrom !ignores:
  - DRY out component+services folder patterns with a {,} glob
  - Ignore new Cypress .spec files and .testenv files for jest coverage

- eslintignore the cypress folder, since they're all .js anyway and it has a decent amount of idiosyncracies
+ cleanup unused eslint-disable
+ cleanup cypress comment boilerplate
+ remove unused paths from webpack config

* Add wiki documentation

* [PR feedback] Move nyc config to own file

* [PR feedback] Switch combine coverage script to .sh
chandlerprall added a commit that referenced this pull request Nov 5, 2021
* Provide rows for datagrid cells to be owned by

* changelog

* undoing things

* working except virtualized container

* working row wrapper implementation

* Create datagrid row elements on-demand and render cells via portals

* Provide rows for datagrid cells to be owned by

* changelog

* undoing things

* working except virtualized container

* working row wrapper implementation

* Create datagrid row elements on-demand and render cells via portals

* Small style cleanup

* updated changelog

* fixing a bad changelog merge

* oh that style was important

* Update src/components/datagrid/body/data_grid_row_manager.ts

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

* Update src/components/datagrid/data_grid_types.ts

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

* Ensure unique ID in combobox with prepended or appended labels (#5229)

* Making an explicit check for ID prop in EUI Combobox

* Checking for a user-passed ID prop
* If no user-passed ID, we'll use the `rootId()` class method to set one
* Prepend and append props need an ID to relate label to the input

* Adding example for Combobox with prepended label
* Confirmed the label `for` attribute is referencing input `id`
* Added styles to round the edges of the child `<div>` when we prepend a
  label
* Bumped package.json

* Adding CHANGELOG entry and bumping version

* Update combo_box.tsx

* Updating styles to match other forms with prepend and apppend.

* Finishing Combobox CSS and test updates
* Simplified the Amsterdam overrides for prepended / appended labels
* Added conditional classes to `combo_box.tsx`
* Added unit tests for user passed ID, prepended and appended classes

* Reordering props in child component.

* Final adjustment to ID check in ComboBox

* Final updates to styling and documentation
* Added CSS rules for compressed, prepend and append border radius
* Updated the Display Toggles to include append
* Removed a duplicated instruction paragraph
* Simplified the logic for a unique ID in combobox.ts

* Adding Combobox unique ID to master changelog

* Updated Form Control Layout SCSS
* The borders were still rounded with prepend and append enabled
* Added two more rules to make borders square for full height and compressed

* Move to _combo_box.scss

Co-authored-by: cchaos <caroline.horn@elastic.co>

* update i18ntokens

* 40.0.0

* Updated documentation.

* Add combined Jest+Cypress code coverage reports (#5262)

* Install Cypress code coverage dependencies

* Set up required babel+istanbul config

* Set up Cypress code coverage config

@see https://github.com/cypress-io/code-coverage#instrument-unit-tests

* Add script for combining cypress & jest code coverage
-into a single json & html report

* Misc cleanup/ignores

- Ignore new `.nyc_output` dir generated by cypress/istanbul

- Improve Jest config collectCoverageFrom !ignores:
  - DRY out component+services folder patterns with a {,} glob
  - Ignore new Cypress .spec files and .testenv files for jest coverage

- eslintignore the cypress folder, since they're all .js anyway and it has a decent amount of idiosyncracies
+ cleanup unused eslint-disable
+ cleanup cypress comment boilerplate
+ remove unused paths from webpack config

* Add wiki documentation

* [PR feedback] Move nyc config to own file

* [PR feedback] Switch combine coverage script to .sh

* [EuiMarkdownEditor] Add `remark-breaks` and line break plugin (#5272)

* Add remark-breaks and line break plugin to MarkdownEditor

* Update changelog

* [Docs] Separated out Borders to its own page & [EuiTableRowCell] fixes (#5283)

* Moved Border tokens to its own page
* Figured out a way to get subsections in the sidenav working
* Added the new `_values` under `customizing/`

* [EuiTableRowCell] Actually support `valign` property manually
* [EuiTable*] Fixing some props and mobile options
* Slightly better responsive table styles

* Fix CL from #5272

* [Docs] Update EuiDatePicker types (#5318)

* omit unsupported types

* resolve console errors

* CL

* [Cypress] Add flakey test retries + harden intermittent context menu failures (#5317)

* Add Cypress failed test retries to CI run mode

* Increase cy.wait on flaky context menu tests (just in case)

* More hardening

* [Docs] Fix Colors guidelines (#5316)

* use useJsonVars

* use variable

* Update dependency @elastic/charts to ^38.1.0 (#5321)

* PR feedback

* Added a cypress test for datagrid row rendering

* Revert changes(?) to docs/

* revert changelog reformats

* re-ignore a couple datagrid example pages when running a11y tests

* Alternate way to know if the datagrid cells have rendered

* PR feedback

* changelog

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Trevor Pierce <1Copenut@users.noreply.github.com>
Co-authored-by: cchaos <caroline.horn@elastic.co>
Co-authored-by: ian moersen <ian@union.io>
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@thompsongl thompsongl mentioned this pull request Dec 6, 2021
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants