Skip to content

Fix devtools test #3

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

Merged
merged 2 commits into from
Nov 28, 2021
Merged

Fix devtools test #3

merged 2 commits into from
Nov 28, 2021

Conversation

prateek3255
Copy link

No description provided.

@prateek3255
Copy link
Author

@TkDodo Since we upgraded react query to the latest version getByRole only works with actual buttons now, not with elements with role button hence using the getByLabelText instead. (might be related testing-library/react-testing-library#835)

@TkDodo
Copy link
Owner

TkDodo commented Nov 27, 2021

hmm, I'm not sure the issues are related, as we use the "right" role. A div with the role button and a proper aria-label should be found with the role selector, like in this testing-library playground

screen.getByRole('button', {
  name: /foo/i
})

works 🤔

@prateek3255
Copy link
Author

That is weird, although I tried investigating the issue as well, and I thought maybe it has something to do with the quotes in the query hash not getting appropriately escaped, but I replaced it with a static label, and it still didn't work.

Also testing library shows all the elements with that role for suggestions, and our div wasn't listed in it.

image

Finally, since we are using the v13.0.0-alpha.4 of @testing-library-react, it could be a bug/breaking change in it. The testing-playground uses v^10.4.9, which is similar to v^10.4.7 that we were using before this, and with that version, the tests were passing for us as well.

@TkDodo
Copy link
Owner

TkDodo commented Nov 28, 2021

so this test works fine for me on the react-18 branch, so I don't think that testing-library has a bug:

  it('test', () => {
    const onClick = jest.fn()
    function App() {
      return (
        <div>
          <div role="button" aria-label="clickMe" onClick={onClick} />
        </div>
      )
    }

    const rendered = renderWithClient(queryClient, <App />)

    rendered.getByRole('button', { name: /clickMe/i }).click()

    expect(onClick).toHaveBeenCalledTimes(1)

I think the real issue is waiting for 1${currentQuery?.queryHash}. it works if I wait for the button to be there instead:

- await screen.findByText(getByTextContent(`1${currentQuery?.queryHash}`))
+ await screen.findByRole('button', {
+   name: `Open query details for ${currentQuery?.queryHash}`,
+ })

so I think thats the better fix. also, isn't it a bit we don't have any expect in the whole test? I think at the end, we should at least have something like:

- await screen.findByText(/query details/i)
+ await waitFor(() => {
+   expect(screen.queryByText(/query details/i)).toBeInTheDocument()
+ })

what do you think?

@prateek3255
Copy link
Author

I think the real issue is waiting for 1${currentQuery?.queryHash}. it works if I wait for the button to be there instead:

Sorry, my bad. You are right. It does work if I use findByRole with await, what's weird and sent me off was the test was working with getByLabelText and not working with getByRole, also the button was visible in the screen.debug() output, so it made me think that either there is something wrong with the role behavior or there is some breaking change with the library. Fixed it now 🙌🏼

also, isn't it a bit we don't have any expect in the whole test? I think at the end, we should at least have something like:

Do we really need to use expect? Because the findBy* and getBy* methods fail the tests with very descriptive errors if they cannot find the respective element. Also, I am not sure using queryByText like this would be a good practice, as Kent recommends in his article, *queryBy methods are only supposed to be used when we don't expect something to be in the document.

@TkDodo
Copy link
Owner

TkDodo commented Nov 28, 2021

Do we really need to use expect?

I think you are right. I was coming from the eslint rule that requires each test to have an expect, but the think the testing-library waiting methods are good here 👍

@TkDodo TkDodo merged commit d7d3dc8 into TkDodo:feature/react-18 Nov 28, 2021
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

Successfully merging this pull request may close these issues.

2 participants