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

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

Open
tnyo43 opened this issue Sep 17, 2023 · 4 comments

Comments

@tnyo43
Copy link

tnyo43 commented Sep 17, 2023

Describe the feature you'd like:

My suggestion is to remove the restriction that the element need to be present at the beginning of waitForElementToBeRemoved.

With the latest version of this library (v6.1.3), the target element of waitFoRElementToBeRemoved need to be present at the beginning.
Here is an example of my use case of waitForElementToBeRemoved. SomeComponent includes a message "some message" and a button to remove the mesage when it is clicked. (I'm sorry I implemented it with react, but it should work with other frameworks as well)

test('some executions after the message is removed works as expected', async () => {
  render(<SomeComponent />);
  const targetElement = getByText('some message');

  // an action to remove the message
  await user.click(getByRole('button', { name: 'remove message' }));

  // wait until the message is removed
  await waitForElementToBeRemoved(targetElement);

  // It is explicit that the following code is executed after the "some message" is removed
  doSomething();
})

The problem is waitForElementToBeRemoved causes an error on the initial check if the message is removed very soon after the button clicked.
Of course I could remove the sentence with waitForElementToBeRemoved, but I want to keep the sentence to make it clear that the following code is executed after the message is removed.

I understand that the reason you implemented the initial check is to make sure that the element IS REMOVED, not HAS NOT BEEN RENDERED (I saw this commend). But it may be overprotective.
In my example, it is obvious that the targetElement is rendered once because we get it with getByText('some message') query so we don't need to initial check.

I don't think we need to check the presence of the element (at least if the first argument is an element, even though I have no idea if it is a callback).

Suggested implementation:

Just removing the initialCheck for the element if the callback is not a function.

Describe alternatives you've considered:

We could use waitFor as like it is introduced in the document, but it is not a good match with expect.assertion.

Also, we could write a code as like await waitForElementToBeRemoved(queryByText('xxx')) and it passes even if it does not presence at the beginning with my suggestion. If we want to care this case, we may be able to create a eslint rule, as like "no-query-in-wait-for-element-to-be-removed"

Teachability, Documentation, Adoption, Migration Strategy:

@AngusMacrae
Copy link

The problem with your suggestion would be that it would require everyone to make an additional manual assertion like you have at the start of the test, to ensure the element exists before proceeding.

Otherwise if changed as suggested, simply doing

waitForElementToBeRemoved(screen.queryBy...)

Would no longer fail if the element never existed in the first place.

So basically it would be making the API less flexible, just for the sake of avoiding one extra check.

@tnyo43
Copy link
Author

tnyo43 commented Oct 2, 2023

it would be making the API less flexible

I think my suggestion makes the API requiring more effort, but also MORE flexible 😉


So how can we ensure that the element exists at the start of the waitForElementToBeRemoved?
If we use await waitForElementToBeRemoved(screen.queryBy...) after any actions to remove a element, we must consider the possibility that the element have already removed at the start of it unless we know it takes much time to complete the removing actions.

We should consider more carefully about the timing of asynchronous processing.

@cmorten
Copy link

cmorten commented Mar 30, 2024

As solution to this specific use-case, is something like…

test('some executions after the message is removed works as expected', async () => {
  render(<SomeComponent />);
  const targetElement = getByText('some message');

  const isRemovedPromise = waitForElementToBeRemoved(targetElement);

  // an action to remove the message
  await user.click(getByRole('button', { name: 'remove message' }));

  // wait until the message is removed
  await isRemovedPromise;

  // It is explicit that the following code is executed after the "some message" is removed
  doSomething();
})

passable? (Some care needed to handle the dangling promise with a subtle change if being strict with resource cleanup and for if the click throws)

@tnyo43
Copy link
Author

tnyo43 commented May 22, 2024

@cmorten
Thank you for your suggestion! That works on my cases.

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

No branches or pull requests

3 participants