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

chore: rewrite tests #1081

Merged
merged 24 commits into from
Dec 27, 2022
Merged

chore: rewrite tests #1081

merged 24 commits into from
Dec 27, 2022

Conversation

ph-fritsche
Copy link
Member

@ph-fritsche ph-fritsche commented Nov 23, 2022

What:

Edit tests to work outside of Jest.
Add script and CI to run tests in multiple environments.

Why:

This allows us to discover conflicts with environments.

#1019

How:

Eliminate dependencies on Jest specific features.
Use @ph.fritsche/toolbox to build, serve and test the project code in Chrome and Node+Jsdom.

Issue with code coverage:

Without additional work this results in Jest only running one set of dependencies. Therefore the resulting coverage lacks the cases specific to React17.
The coverage threshold has to be removed. We have codecov/codecov-action that still warns us about drops in code coverage.

The coverage produced by @ph.fritsche/toolbox (per @swc/core and istanbul-lib-instrument) differs from the coverage produced by kcd-scripts test (per @babel and ?).
It seems that it reports stricter coverage e.g. on nullish coalescing. I'm not sure if it's safe to just merge these with the coverage from kcd-scripts.

In the end the coverage threshold doesn't tell us if the tests are sufficient. Running the tests in other environments provides more value than the coverage threshold, I'd merge this so that we can address the pending PRs as we now have the feedback if they introduce more/new problems in real browsers.

Checklist:

  • Tests
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 23, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 11c6b16:

Sandbox Source
userEvent-dom Configuration
userEvent-react Configuration

@codecov
Copy link

codecov bot commented Nov 24, 2022

Codecov Report

Merging #1081 (11c6b16) into main (1aa2027) will decrease coverage by 0.53%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##              main    #1081      +/-   ##
===========================================
- Coverage   100.00%   99.46%   -0.54%     
===========================================
  Files           88       92       +4     
  Lines         2061     2070       +9     
  Branches       691      694       +3     
===========================================
- Hits          2061     2059       -2     
- Misses           0       11      +11     
Impacted Files Coverage Δ
src/event/eventMap.ts 100.00% <ø> (ø)
src/setup/setup.ts 100.00% <ø> (ø)
src/_interop/dtl.ts 100.00% <100.00%> (ø)
src/_interop/dtlEventMap.ts 100.00% <100.00%> (ø)
src/_interop/dtlHelpers.ts 100.00% <100.00%> (ø)
src/event/wrapEvent.ts 100.00% <100.00%> (ø)
src/setup/api.ts 100.00% <100.00%> (ø)
src/setup/wrapAsync.ts 100.00% <100.00%> (ø)
src/utility/selectOptions.ts 100.00% <100.00%> (ø)
src/utils/dataTransfer/Clipboard.ts 100.00% <100.00%> (ø)
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ph-fritsche ph-fritsche marked this pull request as ready for review December 14, 2022 12:09
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

I didn't review all files, but I like the idea behind this PR.
But, I reviewed the CI output, and here I saw two things that need some investigation:

  • the build step warns for a circular dependency
  • the test step has failed tests, but the step is green

@ph-fritsche
Copy link
Member Author

Thanks for the review.

  • the build step warns for a circular dependency

Because they end up in the same IIFE, this isn't a problem here.
The circular dependency probably should be resolved, but this is an issue for rollup-plugin-node-builtins - for our use case it's good.

  • the test step has failed tests, but the step is green

Yes, I thought about this for a while.
We'll need time to investigate and fix the issues we have in different environments and a failing CI step would block us until all those issues are resolved. So for now the step is successful if it can create a report, no matter if the report includes failed tests or even errors in the test code.
Also the Toolbox isn't mature enough to replace Jest, so I'd keep the "Test" step with Jest although it should provide a duplicate result of the Node, DTL8, React18 run.

@ph-fritsche ph-fritsche merged commit e93a5af into main Dec 27, 2022
@ph-fritsche ph-fritsche deleted the test branch December 27, 2022 11:32
@github-actions
Copy link

🎉 This PR is included in version 14.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@@ -0,0 +1,5 @@
/* eslint-disable @typescript-eslint/no-unnecessary-condition */

import def, * as named from '@testing-library/dom'
Copy link

Choose a reason for hiding this comment

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

This seems to cause some problems since there is no default export from @testing-library/dom. I haven't seen this kind of approach before, just curious, what is the reason for writing it this way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants