fix: Stop calling `waitFor` callback after timeout

KevinBon commented 1 year ago

What:

Prevent waitFor callback from being invoked even after it resolved, on waitFor timeout.

Why:

More context can be found in this issue, but with that context in mind, it's preventing test side-effects:

afterEach should be enough to "clean" the previous test from any side-effect. However, because of the callback leak issue, window.variable will remain to be 123 even after being set back to 1 for a short period of time.

afterEach(() => {
 window.variable = 1;
})
it('my test', () => {
  return waitFor(() => {
    window.variable = 123;
    throw new Error('I want it to timeout');
  })
})

How:

When the clock is mocked, only call checkCallback when finished is false

Checklist:

[ ] Documentation added to the docs site: N/A
[x] Tests
[ ] TypeScript definitions updated: N/A
[x] Ready to be merged

resolves https://github.com/testing-library/dom-testing-library/issues/1270

codesandbox-ci[bot] commented 1 year ago

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 10a1ac7c52e8c027959bf7ff1c7368b77bd56168:

Sandbox	Source
react-testing-library-examples	Configuration

codecov[bot] commented 1 year ago

Codecov Report

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

Comparison is base (a7b7252) 100.00% compared to head (10a1ac7) 100.00%. Report is 4 commits behind head on alpha.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## alpha #1271 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 24 24 Lines 1038 1041 +3 Branches 346 347 +1 ========================================= + Hits 1038 1041 +3 ``` | [Flag](https://app.codecov.io/gh/testing-library/dom-testing-library/pull/1271/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=testing-library) | Coverage Δ | | |---|---|---| | [node-14](https://app.codecov.io/gh/testing-library/dom-testing-library/pull/1271/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=testing-library) | `100.00% <100.00%> (ø)` | | | [node-16](https://app.codecov.io/gh/testing-library/dom-testing-library/pull/1271/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=testing-library) | `?` | | | [node-18](https://app.codecov.io/gh/testing-library/dom-testing-library/pull/1271/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=testing-library) | `100.00% <100.00%> (ø)` | | | [node-20](https://app.codecov.io/gh/testing-library/dom-testing-library/pull/1271/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=testing-library) | `100.00% <100.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=testing-library#carryforward-flags-in-the-pull-request-comment) to find out more.

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

KevinBon commented 1 year ago

@eps1lon I changed the solution implementation.

You can find a reproduction example on this repository.

Output example:

It's been a while since I have used jest, so I'm not sure how to test "this behavior" on jest to satisfy the test code coverage

Don't hesitate if you have any other questions 🙇

github-actions[bot] commented 1 year ago

Uh oh! @KevinBon, the image you shared is missing helpful alt text. Check https://github.com/testing-library/dom-testing-library/pull/1271#issuecomment-1745413251.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

KevinBon commented 1 year ago

I created a branch to show-case that applying this patch will fix this behavior: https://github.com/KevinBon/jasmine-tl-waitfor-leak/pull/1

KevinBon commented 1 year ago

@eps1lon I added a reproducible test for jest through https://github.com/testing-library/dom-testing-library/pull/1271/commits/ce11c0f9f51f51f1e84cb790016d83f44502ec87

KevinBon commented 1 year ago

@eps1lon let me know if you need additional information or are unhappy with my changes 🙇

KevinBon commented 1 year ago

@eps1lon bumping, let me know if you need anything from me. Thank you!