Wait for element to be removed #218

Tolsee · 2019-02-22T18:50:19Z

What: Adding waitForElementToBeRemoved feature.

Why: As discussed in #206, it would be easier to use waitForElementToBeRemoved to check whether the element is removed or not.

How: It is similar to waitForElement. Can be used as waitForElementToBeRemoved(() => getByTestId('data-testid')), actually just like waitForElement with any other query functions.

Checklist:

kentcdodds

This is looking really good to me.

Could we add one test (maybe the first) that's a typical use case example?

kentcdodds · 2019-02-22T19:12:33Z

src/__tests__/wait-for-element-to-be-removed.js

+
+// Using `setTimeout` >30ms instead of `wait` here because `mutationobserver-shim` uses `setTimeout` ~30ms.
+const skipSomeTimeForMutationObserver = (delayMs = 50) =>
+  skipSomeTime(delayMs, 50)


Do you think we could use jest fake timers for this? I've found that tests that rely on setTimeout are pretty unreliable in CI environments especially.

Sounds good, I will give a shot.

I tried to use fakeTimer, but it does not seem to be an ideal place to use timeFakers.

As per I know, setTimeout are unreliable because of time-outs in CI right? How important do you think it is for this PR. If it's pretty important, I will try to look at it in more detail.

I just don't want tests failing randomly on CI. And that happens when relying on setTimeout quite a bit.

I have changed the tests. Can you please take a look.

There are other places such as wait-for-element.js, where setTimeout is heavily used(I know this because my test cases are mostly inspired by that). If that's something you want to change I can create another PR to get rid of setTimeout from tests.

sompylasar

Should it succeed or fail if the element did not exist in the first place? It may be confusing, "ToBeRemoved" imho assumes just the change from "is present" to "is not present".

kentcdodds · 2019-02-22T20:49:52Z

I think that's a good idea as well 👍

Tolsee · 2019-02-23T09:50:06Z

@sompylasar

Should it succeed or fail if the element did not exist in the first place? It may be confusing, "ToBeRemoved" imho assumes just the change from "is present" to "is not present".

I have changed it to throw Error when there is no element at starting.

alexkrolick · 2019-02-24T16:15:42Z

src/__tests__/wait-for-element-to-be-removed.js

@@ -73,9 +74,11 @@ test('it waits for the callback to throw error or a falsy value and only reacts
  for (const [mutationImpl, callbackImpl] of mutationsAndCallbacks) {
    callback.mockImplementation(callbackImpl)
    mutationImpl()
-    await skipSomeTimeForMutationObserver() // eslint-disable-line no-await-in-loop
+    skipSomeTimeForMutationObserver() // eslint-disable-line no-await-in-loop


Doesn't this still need the await?

I don't think so. Now the test runs synchronously because we're using jest fake timers.

alexkrolick · 2019-02-24T16:17:28Z

src/wait-for-element-to-be-removed.js

+  return new Promise((resolve, reject) => {
+    if (typeof callback !== 'function') {
+      reject(
+        'waitForElementToBeRemoved requires a callback as the first parameter',


Nit: requires a function. Callback is jargon.

kentcdodds · 2019-02-24T17:16:52Z

src/wait-for-element-to-be-removed.js

+
+    // Check if the element is not present synchronously,
+    // As the name waitForElementToBeRemoved should check `present` --> `removed`
+    ;(function checkElementPresent() {


Is there a reason this code needs to be in an IIFE?

Not much, I was just hoping to separate functionality.

Will it have a bad impact?

I guess not. Just not a coding style we have anywhere else in the codebase. Feels unnecessary to me.

Ok, will get rid of it.

kentcdodds

Getting close! Thanks for working on this!

kentcdodds · 2019-02-25T18:50:01Z

src/wait-for-element-to-be-removed.js

+  return new Promise((resolve, reject) => {
+    if (typeof callback !== 'function') {
+      reject(
+        'waitForElementToBeRemoved requires a function as the first parameter',


I think this should be an error object rather than just a string so we get a stacktrace.

kentcdodds · 2019-02-25T18:50:54Z

src/wait-for-element-to-be-removed.js

+    // As the name waitForElementToBeRemoved should check `present` --> `removed`
+    try {
+      const result = callback()
+      if (!result) {


Should we be more specific in checking that the result is a single element (or non-empty array of elements)? If it's not then the error can be more specific as well: "The callback function which was passed did not return an element or non-empty array of elements. waitForElementToBeRemoved requires that the element(s) exist before waiting for removal."

kentcdodds · 2019-02-25T18:52:40Z

src/wait-for-element-to-be-removed.js

+        onDone(new Error('Element is not present in the DOM.'), true)
+      }
+    } catch (error) {
+      onDone(new Error('Element is not present in the DOM.'), true)


I think this should be onDone(error) so we don't swallow the error. Also, what's the purpose of true as the second argument here?

@kentcdodds Won't it be confusing for users? by doing onDone(error)? Because then they would not know the error is telling them that waitForElementToBeRemoved requires that the element(s) exist before waiting for removal.

But, yeah, the error might have occurred due to other reasons as well and in that case, we should be sending back the same errors.

The perfect solution would have been to instanceof check for the GetElementError. But we do not have our own error class right?

I will just send back the error for now. But, making our own error class and checking for that, if the error is GetElementError send the above message else sending back the caught error itself seems a good solution to me.

kentcdodds · 2019-02-25T18:52:56Z

src/wait-for-element-to-be-removed.js

+    try {
+      const result = callback()
+      if (!result) {
+        onDone(new Error('Element is not present in the DOM.'), true)


What's the purpose of the second argument of true here?

kentcdodds · 2019-02-26T16:31:25Z

src/__tests__/__snapshots__/wait-for-element-to-be-removed.js.snap

@@ -0,0 +1,51 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP


I don't think these snapshots are at all necessary and just make it harder to know the intent of the tests. Could we change these to be regular assertions instead?

@kentcdodds I have almost removed the snapshot to inline snapshots(Which should be working as regular assertions). But, because we are also passing the error we got from the getByTestId etc, in that case, a snapshot will be a good thing I guess.

src/__tests__/__snapshots__/wait-for-element-to-be-removed.js.snap

src/wait-for-element-to-be-removed.js

…m/Tolsee/dom-testing-library into ts-wait-for-element-to-be-removed

kentcdodds

Thanks! We're getting closer!

kentcdodds · 2019-02-27T20:13:27Z

src/__tests__/wait-for-element-to-be-removed.js

+  await promise
+
+  expect(successHandler).toHaveBeenCalledTimes(1)
+  expect(successHandler.mock.calls[0]).toMatchInlineSnapshot(`


Any reason this couldn't be simplified to:

expect(successHandler).toHaveBeenCalledWith(true)

Or you could also replace this line and the one above with:

expect(successHandler.mock.calls).toEqual([true])

I don't really care either way, but using snapshots for this doesn't make any sense to me personally.

Yeah, make sense. I am a little 🆕 to jest that is the only reason.

kentcdodds · 2019-02-27T20:14:04Z

src/__tests__/wait-for-element-to-be-removed.js

+  expect(callbackForFalsy).toHaveBeenCalledTimes(1)
+  expect(successHandler).toHaveBeenCalledTimes(0)
+  expect(errorHandler).toHaveBeenCalledTimes(3)
+  expect(errorHandler.mock.calls[0]).toMatchSnapshot()


I guess I'm fine with this being a snapshot, but please make it inline.

Tolsee · 2019-02-28T15:24:40Z

Thanks! We're getting closer!

One of my first contribution to this library as well as open source. Really enjoying the journey.

kentcdodds

After looking closer at the tests I can't help but feel like they could be simplified. I hope you don't mind me working on that a little on my own.

kentcdodds · 2019-02-28T15:42:26Z

src/__tests__/wait-for-element-to-be-removed.js

@@ -0,0 +1,334 @@
+import {waitForElementToBeRemoved} from '../'
+// adds special assertions like toBeTruthy


toBeTruthy is built-into jest. This adds things like toBeInTheDocument. But I don't think we need this comment anyway. Please remove it.

Tolsee · 2019-02-28T15:50:02Z

@kentcdodds

After looking closer at the tests I can't help but feel like they could be simplified. I hope you don't mind me working on that a little on my own.

Of course not.

Tolsee · 2019-03-09T06:15:39Z

@kentcdodds Did you get a chance to review the tests? If you do not have enough time, you can give me a direction and I will work on the test. I want this to be merged 😄

kentcdodds · 2019-03-09T06:20:07Z

Sorry, been busy. I'll try to get this done this weekend.

kentcdodds · 2019-03-09T19:21:11Z

Alrighty. I'm happy with these tests now. Once the build passes then I'll merge/release it :D

kentcdodds · 2019-03-09T20:57:47Z

jest.config.js

-    {
-      ...baseConfig,
-      displayName: 'node',
-      testEnvironment: 'jest-environment-node',


Honestly, running all the exact same tests in node led to some poor testing practices and I didn't like the over-testing either. If someone really wants to have tests like this, then we can add a other/node/__tests__/ directory with a other/node/jest.config.js file. I want the tests to be different.

Also, I don't think it's worthwhile to have tests for node. I'm pretty sure there's a very small number of people who benefit from this.

kentcdodds · 2019-03-09T21:04:59Z

.travis.yml

@@ -6,7 +6,6 @@ cache:
 notifications:
  email: false
 node_js:
-  - 'node'


Doing this opens us up to stuff like this: https://stackoverflow.com/questions/55059748/travis-jest-typeerror-cannot-assign-to-read-only-property-symbolsymbol-tostr

I think the benefit of testing an unstable version of node isn't worth stuff like this.

kentcdodds

This is ready to go!

kentcdodds · 2019-03-09T21:09:11Z

🎉 This PR is included in version 3.17.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

Tolsee changed the title ~~Ts wait for element to be removed~~ Wait for element to be removed Feb 22, 2019

Tolsee added 5 commits February 23, 2019 00:40

Add tests and snapshots

f3eca1c

Implement waitForElementToBeRemoved method

c547962

Export waitForElementToBeRemoved

cc21522

Add as contributor

c75e492

Update README.md after rebase

a349725

Tolsee force-pushed the ts-wait-for-element-to-be-removed branch from cad86d5 to a349725 Compare February 22, 2019 19:01

kentcdodds reviewed Feb 22, 2019

View reviewed changes

sompylasar reviewed Feb 22, 2019

View reviewed changes

Throw error is the element is not present in the first place

d1d303d

Tolsee added 4 commits February 23, 2019 15:43

Add test for full coverage

39fea0e

User jest time faker

36c4e4c

Add typings

f5b081d

Cleanup

2dedb1f

alexkrolick reviewed Feb 24, 2019

View reviewed changes

kentcdodds reviewed Feb 24, 2019

View reviewed changes

Tolsee added 2 commits February 25, 2019 01:01

Update snapshot

54cbc89

Get rid of iife

999ff5c

kentcdodds reviewed Feb 25, 2019

View reviewed changes

Tolsee added 3 commits February 26, 2019 21:04

Cleanup from review

e7a018e

Check for empty array as well

0518e78

Check for error, falsy and empty to check whether the element is removed

d6150af

kentcdodds requested changes Feb 26, 2019

View reviewed changes

Tolsee added 5 commits February 27, 2019 21:01

error not defined error fix

4e2f81c

Change snapshot to inline snapshot

20ada81

Update snapshot

46a952f

Merge branch 'ts-wait-for-element-to-be-removed' of https://github.co…

618c240

…m/Tolsee/dom-testing-library into ts-wait-for-element-to-be-removed

Merge branch 'master' into ts-wait-for-element-to-be-removed

af327d1

Tolsee added 3 commits February 27, 2019 22:18

Only observe mutations if synchronous test passes

505d783

only observer if synchronous test passes fix

6fcfc83

Await for the respective promise to resolve in test

579c876

kentcdodds requested changes Feb 27, 2019

View reviewed changes

Snapshot test to normal toHaveBeenCalledWith assertions

2f7452d

Update comment

20cd110

kentcdodds reviewed Feb 28, 2019

View reviewed changes

Remove comment

bc2e3f2

test: improve tests for wait-for-element-to-be-removed

9834de2

test: fix everything

17f136f

kentcdodds force-pushed the ts-wait-for-element-to-be-removed branch from c9be668 to 17f136f Compare March 9, 2019 20:55

kentcdodds reviewed Mar 9, 2019

View reviewed changes

test: do not test unstable node versions

6664f0a

kentcdodds reviewed Mar 9, 2019

View reviewed changes

kentcdodds approved these changes Mar 9, 2019

View reviewed changes

kentcdodds merged commit 1727a49 into testing-library:master Mar 9, 2019

kentcdodds added the released label Mar 9, 2019

tnyo43 mentioned this pull request Sep 17, 2023

we may not need the initial check at the beginning of waitForElementToBeRemoved #1265

Open

		@@ -0,0 +1,334 @@
		import {waitForElementToBeRemoved} from '../'
		// adds special assertions like toBeTruthy

Wait for element to be removed #218

Wait for element to be removed #218

Conversation

Tolsee commented Feb 22, 2019 • edited Loading

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tolsee Feb 24, 2019 • edited Loading

Choose a reason for hiding this comment

sompylasar left a comment

Choose a reason for hiding this comment

kentcdodds commented Feb 22, 2019

Tolsee commented Feb 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tolsee Feb 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tolsee commented Feb 28, 2019

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tolsee commented Feb 28, 2019 • edited Loading

Tolsee commented Mar 9, 2019

kentcdodds commented Mar 9, 2019

kentcdodds commented Mar 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Mar 9, 2019

Tolsee commented Feb 22, 2019 •

edited

Loading

Tolsee Feb 24, 2019 •

edited

Loading

Tolsee commented Feb 23, 2019 •

edited

Loading

Tolsee Feb 26, 2019 •

edited

Loading

Tolsee commented Feb 28, 2019 •

edited

Loading