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

feat: add warnings when globals are missing #1244

Merged
merged 5 commits into from
Nov 8, 2023

Conversation

MatanBobi
Copy link
Member

What: Adds a warning when we weren't able to setup logic through test runner globals (afterEach, beforeAll, afterAll).

Why: This was raised in #1240 and I believe it's worth adding because some stuff won't work for users so they should have a warning stating why.

How:

Adding else blocks in the relevant places.

Checklist:

  • [ ] Documentation added to the
    docs site
  • Tests
  • TypeScript definitions updated
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 13, 2023

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 39fdc6c:

Sandbox Source
React Configuration
react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #1244 (39fdc6c) into main (c04b8f0) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main     #1244   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          192       192           
  Branches        38        40    +2     
=========================================
  Hits           192       192           
Flag Coverage Δ
canary 100.00% <ø> (ø)
experimental 100.00% <ø> (ø)
latest 100.00% <ø> (ø)

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

Files Coverage Δ
src/index.js 100.00% <ø> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

timdeschryver
timdeschryver previously approved these changes Nov 6, 2023
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 like the idea!
I'm probably "stealing" this for Angular Testing Library :)

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
@MatanBobi MatanBobi changed the title chore: add warnings when globals are missing feat: add warnings when globals are missing Nov 7, 2023
@MatanBobi MatanBobi merged commit d80319f into main Nov 8, 2023
13 checks passed
@MatanBobi MatanBobi deleted the chores/add-warnings-when-missing-globals branch November 8, 2023 06:51
@ITenthusiasm
Copy link

@MatanBobi (and @timdeschryver, if you're still interested in Angular's implementation), it seems like this change could potentially run the risk of cluttering the logs for developers who intentionally aren't using the automatic cleanup functions. (For example, I'm using vitest, which by default doesn't have globals enabled. But I'm sticking with Vitest's defaults intentionally.) Particularly, this warning will be logged for every single test file that tests React components.

Is it possible to only display this warning once per call to jest/vitest/etc.? For instance, is there a way that an ENV var (e.g., *TL_GLOBAL_WARNING_LOGGED) could be set (which only lasts the lifetime of npx jest, npx vitest, etc.)? Or maybe something else would be possible?

@timdeschryver
Copy link
Member

Thanks @ITenthusiasm that's a valid point, we'll look into this.

@@ -20,6 +20,10 @@ if (typeof process === 'undefined' || !process.env?.RTL_SKIP_AUTO_CLEANUP) {
teardown(() => {
cleanup()
})
} else {
console.warn(
`The current test runner does not support afterEach/teardown hooks. This means we won't be able to run automatic cleanup and you should be calling cleanup() manually.`,
Copy link
Member

Choose a reason for hiding this comment

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

We should add instructions on how to fix this warning i.e. import from /pure instead or better, ensure that you always resolve to /pure in your test environment.

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.

None yet

4 participants