MNT Apply changes requested from peer review #1058
Conversation
| // in this case that provides a false negative result (confirmed by trying a timeout | ||
| // with queryByRole in the "Header should not contain a Tooltip for a broken element" test | ||
| // which causes that test to fail) | ||
| const tooltip = await screen.findByRole('tooltip', null, { onTimeout: () => {} }); |
There was a problem hiding this comment.
Won't this wait for the default 5000 milliseconds rather 500 milliseconds? Means test suite is now way slower
There was a problem hiding this comment.
This is the only combination of this test and the previous test that gave consistent results. If, for example, I tried the waitFor(() => null, { timeout: 500 }); and then check screen.queryByText('File') in the previous test, it said that element didn't exist. Which meant this test would actually always pass.
There was a problem hiding this comment.
The test is supposed to pass though because there isn't supposed to be a tooltip? i.e. It would fail the previous test if there was a tooltip in the DOM - meaning the test is useful for detecting regressions?
Not sure what what was wrong with the previous test? Happy to change it to fileByRole() though otherwise not happy with the 5000 millisecond delay on the test.
There was a problem hiding this comment.
and then check screen.queryByText('File') in the previous test
Oh - previous testing meaning a test where there is supposed to be a tooltip? Meaning the way the test was meant it didn't detect tooltips when they were supposed to be there?
There was a problem hiding this comment.
Yup. Sorry, I should have said "the test above"
There was a problem hiding this comment.
The updated code in the PR also doesn't seem to work with the test above
This should work:
await screen.findByRole('tooltip', {}, { timeout: 500, onTimeout: () => null });
GuySartorelli
force-pushed
the
pulls/5.0/post-pr-pr
branch
from
85724b7 to
91b0eb2
Compare
Issue