feature/use-original-setTimeout-when-used-in-a-test #305

LaurensBosscher · 2019-07-04T11:04:15Z

What:

Utilizes the original (non-mocked) setTimeout for waitForDomChange, waitForElement and waitForElementToBeRemoved

Why:

waitForDomChange, waitForElement and waitForElementToBeRemoved will now work as expected when the user utilizes this library in a test that uses jest.useFakeTimers(). See: #300

How:

I've implemented the workaround used by wait-for-expect: https://github.com/TheBrainFamily/wait-for-expect/pull/5/files. I've added a global (global.useFakeTimers) so that we can still utilize fake timers in our test.

getSetTimeout and getClearTimeout utilize dependency injection because it's not possible to set window to undefined in jest. I would love feedback on the tests that I've added, let me know if you think there's something that could be improved.

Checklist:

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

… and getClearTimeout

…hat environments withouth window will work as expected

LaurensBosscher · 2019-07-04T12:16:06Z

src/helpers.js

+ *
+ * see: https://github.com/testing-library/dom-testing-library/issues/300
+ */
+function getSetTimeout(windowObject) {


I've injected the windowObject as a dependency here because (as far as I know) there's no way in Jest to set the window object to be undefined. This seemed to be the best way to make this code testable.

Instead of this, you could add a test for this case to https://github.com/testing-library/dom-testing-library/tree/master/src/__node_tests__

Other than that, this looks pretty good to me.

@kentcdodds Do you mean instead of dependency injection?

The node tests are still run within Jest and the main issue I faced was that in Jest window === globals (see: jestjs/jest#3692 (comment)) and I can't set globals (and thus window) to undefined.

Is the a reason the code can't be:

return typeof window === 'undefined' ? global.setTimeout : window.setTimeout

What I was saying is you no longer need the windowObject argument with the code I provided.

Hmm, I get that but how would I be able to test that: "returns original getSetTimeout from global if window is undefined" without passing windowObject?

Perhaps we could chat about what the best approach to test that case would be?

returns original getSetTimeout from global if window is undefined

You can add another test to the node environment tests, as I said earlier:

Instead of this, you could add a test for this case to https://github.com/testing-library/dom-testing-library/tree/master/src/__node_tests

Ahhhhh, it finally clicked this morning. The node tests are run without JSDOM and thus without a window object. I'm new to this project and the deeper internals of Jest, that wasn't clear to me.

I've updated the PR, removed the dependency injection of window and updated the tests for the node environment. Let me know what you think!

…in-a-test

…for window

kentcdodds

Thanks for this! Just a few suggestions/comments.

kentcdodds · 2019-07-10T13:54:39Z

src/__tests__/helpers.js


+describe('getSetTimeout', () => {
+  it('returns mocked setTimeout if global.useFakeTimers is set and jest.useFakeTimers is used', () => {
+    // global.useFakeTimers is set to true for all tests in tests/setup-env.js


If that's the case, then isn't jest.useFakeTimers() on the next line unnecessary and jest.useRealTimers() at the end of this test breaking test isolation?

Actually, now that I think of it, why are we setting useFakeTimers to true in setup-env? I think it'd be better if most of our tests run without fake timers wouldn't it?

global.useFakeTimers is a boolean that enables fake timers if someone uses jest.useFakeTimers(). So all our tests should behave exactly the same as they did before this PR :)

I think we can improve upon the name of the boolean. How about global.returnMockedTimersWhenUsingJestFakeTimers? or perhaps global.isDomTestingLibraryInternalTests?

Or would you prefer that we leave this undefined by default and only set this to true in our tests that use fake timers (https://github.com/testing-library/dom-testing-library/search?q=jest.useFakeTimers&unscoped_q=jest.useFakeTimers)?

Doing that will mean that jest.useFakeTimers will not behave as expected which might confuse contributors which are unfamiliar with the code base. I figured it would be best to follow the principe of least surprise here.

Oh wait. Are you saying that if people want to take advantage of this feature they have to set global.useFakeTimers? I'd really prefer to find another way to detect whether fake timers are happening... Maybe I don't understand what's going on/what the purpose of this is 😬

Nop, nop, exactly the opposite! :) This behaviour is enabled by default for everyone who uses dom-testing-library in their application tests, but it's disabled for dom-testing-library internal tests.

I do think that I'm explaining and documenting this rather poorly. Let me explain it to you first with a bunch of examples, I hope that clears things up! Perhaps than you give me some pointers on how exactly I can communicate this behaviour better? Your suggestions are very much welcome!

If I, as a user, use dom-testing-library, all the dom-testing library methods that I use in my application tests will use a non-mocked setTimeout. So if I write:

test('My awesome application!', async (done) => const { findByText } = render(<MyApplication/>); jest.useFakeTimers(); await findByText('My application is awesome!'); done(); );

dom-testing-library will check for 4.5 seconds if the text 'Awesome Text' is in my application.

Without this PR, the above test will most likely fail, because findByText uses a mocked setTimeout which reduces the timeout to 1 tick at which point my application is not ready.

If I, as a contributor to dom-testing-library, write a new test, for dom-testing-library, and if I write the following test:

test('dom-testing-library internal test example', async (done) => ... jest.useFakeTimers(); setTimeOut(async () => { await findByText('Dom testing library is awesome!') done(); , 999}; );

This test will use the mocked setTimeout and thus fail/succeed within <10 msecs. This is exactly the same behaviour as before this PR.

If I, as a contributor to dom-testing-library, write a new test, for dom-testing-library, and if I write the following test:

test('dom-testing-library internal test example 2', async (done) => ... setTimeOut(async () => { await findByText('Dom testing library is awesome!') done(); , 999}; );

This test will run for 999 msec and then run findByText and fail/succeed.

The global is used to determine if we're running the dom-testing-library internal tests.

I hope this clears things up :)

The fix for wait-for-expect is a lot smaller because they don't have any internal tests that use jest.useFakeTimers() which greatly reduces the complexity.

Oh, I see what you're saying. So the whole reason that we have the global thing is for internal testing purposes.

Hmm... I'm not sure I understand why it's necessary though. Why can't DOM Testing Library's tests work the same way that users of DTL test?

Ok, I think I've worked it out. I'm going to make a contribution to this to improve things I think.

src/helpers.js

Co-Authored-By: Kent C. Dodds <kent+github@doddsfamily.us>

src/helpers.js

kentcdodds · 2019-07-10T16:13:25Z

src/__tests__/helpers.js


+describe('getSetTimeout', () => {
+  it('returns mocked setTimeout if global.useFakeTimers is set and jest.useFakeTimers is used', () => {
+    // global.useFakeTimers is set to true for all tests in tests/setup-env.js


Actually, now that I think of it, why are we setting useFakeTimers to true in setup-env? I think it'd be better if most of our tests run without fake timers wouldn't it?

…in-a-test

Co-Authored-By: Kent C. Dodds <kent+github@doddsfamily.us>

kentcdodds · 2019-07-11T22:20:03Z

Here we go. I think this will be better :) Requires some fanciness in the test, but I'd rather put the fanciness in the tests than in the source code :)

kentcdodds · 2019-07-11T22:21:12Z

src/__tests__/fake-timers.js

@@ -0,0 +1,85 @@
+import {render} from './helpers/test-utils'


We've moved all the fake timers tests to this file. The only reason we're using fake timers is for testing timeouts and we don't want this test to take forever.

kentcdodds · 2019-07-11T22:21:42Z

src/helpers.js

@@ -1,5 +1,17 @@
 import MutationObserver from '@sheerun/mutationobserver-shim'


Simplifying this quite a bit by not using getters and instead just creating variables. Similar to how wait-for-expect does it.

kentcdodds

I'm happy with this.

kentcdodds · 2019-07-11T22:42:51Z

🎉 This PR is included in version 5.5.3 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

Laurens Bosscher added 3 commits July 4, 2019 12:58

feature/use-original-setTimeout-when-used-in-a-test

d76ca27

also utilize the original clearTimeout, added tests for getSetTimeout…

22f915c

… and getClearTimeout

added extra check to getSetTimeout and getClearTimeout to make sure t…

cf2fb03

…hat environments withouth window will work as expected

LaurensBosscher commented Jul 4, 2019

View reviewed changes

Laurens Bosscher and others added 4 commits July 9, 2019 10:28

simplified getSetTimeout and getClearTimeout

ce83f8f

Merge branch 'master' into feature/use-original-setTimeout-when-used-…

0e1c612

…in-a-test

updated tests for the Node environment, removed dependency injection …

060eafd

…for window

updated test name

9d7f087

kentcdodds reviewed Jul 10, 2019

View reviewed changes

Update src/helpers.js

a13396a

Co-Authored-By: Kent C. Dodds <kent+github@doddsfamily.us>

kentcdodds reviewed Jul 10, 2019

View reviewed changes

LaurensBosscher and others added 4 commits July 11, 2019 17:17

Merge branch 'master' into feature/use-original-setTimeout-when-used-…

1b3c31a

…in-a-test

Update src/helpers.js

8eccbb4

Co-Authored-By: Kent C. Dodds <kent+github@doddsfamily.us>

updated with feedback from Kent

0293c37

do some fancy magic stuff

39357b6

no longer needed

f79a2d5

kentcdodds previously approved these changes Jul 11, 2019

View reviewed changes

fix coverage

f53c214

kentcdodds dismissed their stale review via f53c214 July 11, 2019 22:28

kentcdodds approved these changes Jul 11, 2019

View reviewed changes

kentcdodds merged commit dae25d5 into testing-library:master Jul 11, 2019

kentcdodds added the released label Jul 11, 2019

		@@ -1,5 +1,17 @@
		import MutationObserver from '@sheerun/mutationobserver-shim'

feature/use-original-setTimeout-when-used-in-a-test #305

feature/use-original-setTimeout-when-used-in-a-test #305

Uh oh!

Conversation

LaurensBosscher commented Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kentcdodds Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaurensBosscher Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaurensBosscher Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaurensBosscher Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kentcdodds commented Jul 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

kentcdodds commented Jul 11, 2019

Uh oh!

Uh oh!

LaurensBosscher commented Jul 4, 2019 •

edited

Loading

kentcdodds Jul 9, 2019 •

edited

Loading

LaurensBosscher Jul 10, 2019 •

edited

Loading

LaurensBosscher Jul 11, 2019 •

edited

Loading

LaurensBosscher Jul 11, 2019 •

edited

Loading