-
Notifications
You must be signed in to change notification settings - Fork 113
test: remove await from userEvent usage #182
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
Conversation
The checkbox group test fails if called synchronously :(
What should we be using, Vue.nextTick(), flushPromises(), or waitFor()?
What should we be using,
Vue.nextTick(),flushPromises(), orwaitFor()?
To keep it relevant to Testing Library users... I'd say waitFor? All these options would do the trick, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure it's better for users to learn when/why they should use waitFor instead of just awaiting userEvent? (I honestly don't know 😂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you mean userEvent, not userSelect. I don't think we should encourage users to await click because it's not async, and awaiting it implies that the test code will wait for Vue to render (not necessarily true as far as I understand, because Vue renders asyncronously). Awaiting anything before this line (even await null) will pass the test. So while await userEvent.click(...) is simpler, I don't think it's correct, and await waitFor(...) better expresses that the assertion will pass some time after clicking. Alternatively we could use one of the other functions I suggested or make user event properly asyncronous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deal 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to merge this as is, or do you prefer any of the alternatives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's fine as is 😃 I wonder if ppl will understand/know the difference between "test is failing because Vue hasn't updated the DOM yet" and "test is failing because my component is actually wrong", but that's something beyond our control (as long as Vue updates async)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make userEvent async for Vue users, this can be reverted, and "test is failing because Vue hasn't updated the DOM yet" should be more obvious when an await is missing (some linters can even warn you about not awaiting an async function).
Codecov Report
Merging #182 (9c565ef) into master (3dbb1c2) will not change coverage.
The diff coverage isn/a.
@@ Coverage Diff @@ ## master #182 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 1 1 Lines 100 100 Branches 35 35 ========================================= Hits 100 100
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 3dbb1c2...9c565ef. Read the comment docs.
userEventis entirely synchronous (except fortype'sdelayoption which isn't used), so we should use it withoutawaitand encourage users to do the same.