Skip to content

test: Update tests to be compatible with Jest 27 #639

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

Merged
merged 2 commits into from
Jun 20, 2021
Merged

test: Update tests to be compatible with Jest 27 #639

merged 2 commits into from
Jun 20, 2021

Conversation

mpeyper
Copy link
Member

@mpeyper mpeyper commented Jun 18, 2021

What:

Update test suite to be compatible with jest 27.

Why:

The PR to update kcd-scripts is failing because of some changes in jest 27 that prevent beforeEach and afterEach hooks being added after the tests have started to run. In our test suite, we were doing this in order to test the auto cleanup and error suppression features.

How:

Most of the changes here involve moving the setup and require statements outside of the beforeEach/beforeAll hooks they were bing called it (i.e. after the tests were running) into the describe blocks. While researching how to deal with this, I discovered that jest runs each test file in it's own sandbox, so much of the protection I thought using the test hooks were giving us is there within the file which was sufficient for most of the affected tests.

I also took the opportunity to review how the imports were occurring and found a few and were using require when a regular import would work for that case. I also found a few unused imports to clean up.

Checklist:

  • Tests
  • Ready to be merged

I really want to look at removing the duplication in the test suite.

Bumps [kcd-scripts](https://github.com/kentcdodds/kcd-scripts) from 10.0.0 to 11.1.0.
- [Release notes](https://github.com/kentcdodds/kcd-scripts/releases)
- [Changelog](https://github.com/kentcdodds/kcd-scripts/blob/main/CHANGELOG.md)
- [Commits](kentcdodds/kcd-scripts@v10.0.0...v11.1.0)

---
updated-dependencies:
- dependency-name: kcd-scripts
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
@mpeyper mpeyper requested a review from joshuaellis June 18, 2021 13:02
@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #639 (bf0326b) into main (3bf9e5c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #639   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          235       235           
  Branches        29        29           
=========================================
  Hits           235       235           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bf9e5c...bf0326b. Read the comment docs.

Comment on lines +10 to +11
const expectedRenderer = require('../native/pure') as ReactHooksRenderer
const actualRenderer = require('../pure') as ReactHooksRenderer
Copy link
Member Author

Choose a reason for hiding this comment

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

this was the only test where I felt the requires in the tests were still appropriate. I worked around the issue by changing to use the pure imports instead. This left the non-pure import uncovered, which is why I introduced the simplified test file for it.

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Looks like a fairly straight forward refactor.

RE. Duplication, I guess this is where it would be useful to have a set of tests that are called with each renderer type. Could we do forEach loops with the specific functions from the library to avoid duplication? Although not exactly "pretty", it could work.

I suppose in terms of structure we'd then hoist tests out of the target and into a specific test folder.

@mpeyper
Copy link
Member Author

mpeyper commented Jun 20, 2021

RE. Duplication, I guess this is where it would be useful to have a set of tests that are called with each renderer type. Could we do forEach loops with the specific functions from the library to avoid duplication? Although not exactly "pretty", it could work.

Yeah, there's also jest-in-case or the built in test.each that could be used instead of a forEach loop. In any case (pun intended), this is likely what we'll end up doing.

In my ideal world, the test file would execute multiple times with different renderers so that the import of on doesn't affect the others (e.g. the cleanup and error suppression with be set up and torn down multiple times), but I've been looking into that it is seems to either be very difficult right now. jest-runner-groups seems about the closest, but doesn't expose any ability to know which group is running to switch out the renderer with.

I suppose in terms of structure we'd then hoist tests out of the target and into a specific test folder.

I was thinking we could just put them in src/__tests__ with the default renderer tests? I don't mind having a seperate test root for them. We actually had that before we duplicated everything for the multiple renderers, so bringing it back when we merge the tests again makes sense.

@mpeyper mpeyper merged commit 79b5559 into main Jun 20, 2021
chris110408 pushed a commit to chris110408/react-hooks-testing-library that referenced this pull request Jun 21, 2021
* chore(deps-dev): bump kcd-scripts from 10.0.0 to 11.1.0

Bumps [kcd-scripts](https://github.com/kentcdodds/kcd-scripts) from 10.0.0 to 11.1.0.
- [Release notes](https://github.com/kentcdodds/kcd-scripts/releases)
- [Changelog](https://github.com/kentcdodds/kcd-scripts/blob/main/CHANGELOG.md)
- [Commits](kentcdodds/kcd-scripts@v10.0.0...v11.1.0)

---
updated-dependencies:
- dependency-name: kcd-scripts
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>

* test: Update tests to be compatible with Jest 27

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@joshuaellis
Copy link
Member

I was thinking we could just put them in src/tests with the default renderer tests? I don't mind having a seperate test root for them. We actually had that before we duplicated everything for the multiple renderers, so bringing it back when we merge the tests again makes sense.

Yeah i think if we're merging them that's the direction to take

@mpeyper mpeyper deleted the jest-27 branch June 21, 2021 08:36
@github-actions
Copy link

🎉 This PR is included in version 7.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

2 participants