Safe check for setImmediate and clearImmediate #916

renatoalencar · 2021-03-25T15:15:01Z

What:

Avoid crashing when setImmediate and clearImmediate is not defined, which is now the case for JSDOM.

Why:

See #914

How:

Safe check suggested by @gaearon

Checklist:

Documentation added to the
docs site
Tests
Typescript definitions updated
Ready to be merged

Jest is planning to sandbox setImmediate, and this would not work since setImmediate and clearImmediate would be undefined.

codesandbox-ci · 2021-03-25T15:16:28Z

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 014ed78:

Sandbox	Source
react-testing-library-examples	Configuration

gaearon · 2021-03-25T15:24:39Z

It would be good to check if this is enough to fix jestjs/jest#11222.

renatoalencar · 2021-03-25T15:26:01Z

It would be good to check if this is enough to fix facebook/jest#11222.

That's great, I'm trying to find a reasonable way to add tests for this.

gaearon · 2021-03-25T16:23:45Z

Looks like jestjs/jest#11222 (comment) works!

codecov · 2021-03-25T16:24:29Z

Codecov Report

Merging #916 (2781ebc) into master (c6acb0a) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 2781ebc differs from pull request most recent head 014ed78. Consider uploading reports for the commit 014ed78 to get more accurate results

@@            Coverage Diff            @@
##            master      #916   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          944       948    +4     
  Branches       285       287    +2     
=========================================
+ Hits           944       948    +4

Flag	Coverage Δ
node-10.14.2	`100.00% <100.00%> (ø)`
node-12	`100.00% <100.00%> (ø)`
node-14	`100.00% <100.00%> (ø)`
node-15	`100.00% <100.00%> (ø)`

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

Impacted Files	Coverage Δ
src/helpers.js	`100.00% <100.00%> (ø)`

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 c6acb0a...014ed78. Read the comment docs.

renatoalencar · 2021-03-25T16:26:32Z

Looks like facebook/jest#11222 (comment) works!

That's awesome! It just finished testing it.

eps1lon

Haven't looked into the issue but this seems ok. Just some minor issues with the testing approach to consider.

src/__tests__/helpers.js

Make sure that if the test breaks, it would not break other tests either by properly restoring those global variables in an afterEach call.

renatoalencar · 2021-03-25T17:03:59Z

This PR would close #914?

Yeah. I also tested temporarily deleting setImmediate from tests/setup-env.js to see if everything would still works and it looks like that's fine.

kentcdodds

LGTM 👍 And with the 👍👍 from several folks, I'd say we can merge this.

src/helpers.js

kentcdodds · 2021-03-25T18:13:30Z

Hope you don't mind I added a little comment to explain why we have the extra check.

kentcdodds · 2021-03-25T18:14:06Z

@all-contributors please add @renatoalencar for code and tests

allcontributors · 2021-03-25T18:14:15Z

@kentcdodds

I've put up a pull request to add @renatoalencar! 🎉

renatoalencar · 2021-03-25T18:14:43Z

Hope you don't mind I added a little comment to explain why we have the extra check.

That's awesome, thanks.

kentcdodds · 2021-03-25T18:15:06Z

@all-contributors please add @gaearon for bugs and review

allcontributors · 2021-03-25T18:15:15Z

@kentcdodds

I've put up a pull request to add @gaearon! 🎉

github-actions · 2021-03-25T18:20:14Z

🎉 This PR is included in version 7.30.1 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

gaearon · 2021-03-25T18:39:18Z

thanks!

fix: safe check for setImmediate and clearImmediate

aa1309c

Jest is planning to sandbox setImmediate, and this would not work since setImmediate and clearImmediate would be undefined.

gaearon mentioned this pull request Mar 25, 2021

fix(env-jsdom): remove setImmediate and clearImmediate jestjs/jest#11222

Merged

tests: add tests reproducing testing-library#914

4580d71

renatoalencar changed the title ~~Draft: Safe check for setImmediate and clearImmediate~~ Safe check for setImmediate and clearImmediate Mar 25, 2021

eps1lon reviewed Mar 25, 2021

View reviewed changes

src/__tests__/helpers.js Outdated Show resolved Hide resolved

src/__tests__/helpers.js Outdated Show resolved Hide resolved

src/__tests__/helpers.js Outdated Show resolved Hide resolved

tests: properly restoration of global variables

2781ebc

Make sure that if the test breaks, it would not break other tests either by properly restoring those global variables in an afterEach call.

This comment has been minimized.

Sign in to view

eps1lon linked an issue Mar 25, 2021 that may be closed by this pull request

When running in Jest, code assumes setImmediate and clearImmediate #914

Closed

kentcdodds previously approved these changes Mar 25, 2021

View reviewed changes

kentcdodds reviewed Mar 25, 2021

View reviewed changes

src/helpers.js Show resolved Hide resolved

Update src/helpers.js

014ed78

kentcdodds dismissed their stale review via 014ed78 March 25, 2021 18:13

kentcdodds merged commit 14788b6 into testing-library:master Mar 25, 2021

allcontributors bot mentioned this pull request Mar 25, 2021

docs: add renatoalencar as a contributor #918

Merged

allcontributors bot mentioned this pull request Mar 25, 2021

docs: add gaearon as a contributor #920

Merged

github-actions bot added the released label Mar 25, 2021

renatoalencar deleted the fix-setimmediate branch March 29, 2021 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Safe check for setImmediate and clearImmediate #916

Safe check for setImmediate and clearImmediate #916

renatoalencar commented Mar 25, 2021 •

edited

codesandbox-ci bot commented Mar 25, 2021 •

edited

gaearon commented Mar 25, 2021

renatoalencar commented Mar 25, 2021

gaearon commented Mar 25, 2021

codecov bot commented Mar 25, 2021 •

edited

renatoalencar commented Mar 25, 2021

eps1lon left a comment

This comment has been minimized.

This comment has been minimized.

renatoalencar commented Mar 25, 2021

kentcdodds left a comment

kentcdodds commented Mar 25, 2021

kentcdodds commented Mar 25, 2021

allcontributors bot commented Mar 25, 2021

renatoalencar commented Mar 25, 2021

kentcdodds commented Mar 25, 2021

allcontributors bot commented Mar 25, 2021

github-actions bot commented Mar 25, 2021

gaearon commented Mar 25, 2021

Safe check for setImmediate and clearImmediate #916

Safe check for setImmediate and clearImmediate #916

Conversation

renatoalencar commented Mar 25, 2021 • edited

codesandbox-ci bot commented Mar 25, 2021 • edited

gaearon commented Mar 25, 2021

renatoalencar commented Mar 25, 2021

gaearon commented Mar 25, 2021

codecov bot commented Mar 25, 2021 • edited

Codecov Report

renatoalencar commented Mar 25, 2021

eps1lon left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

renatoalencar commented Mar 25, 2021

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Mar 25, 2021

kentcdodds commented Mar 25, 2021

allcontributors bot commented Mar 25, 2021

renatoalencar commented Mar 25, 2021

kentcdodds commented Mar 25, 2021

allcontributors bot commented Mar 25, 2021

github-actions bot commented Mar 25, 2021

gaearon commented Mar 25, 2021

renatoalencar commented Mar 25, 2021 •

edited

codesandbox-ci bot commented Mar 25, 2021 •

edited

codecov bot commented Mar 25, 2021 •

edited