Skip to content

Make sure query is refetched specified number of times #168

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 20 commits into from
Feb 28, 2020

Conversation

cherniavskii
Copy link
Contributor

If retry is set to 1, I would expect useQuery to try once again after fail

@cherniavskii
Copy link
Contributor Author

That's weird, tests are passing on my machine :/

@tannerlinsley
Copy link
Collaborator

Hmm Travis env issue maybe?

@cherniavskii
Copy link
Contributor Author

Can you check on your machine?

@tannerlinsley
Copy link
Collaborator

Passes 😂

@cherniavskii
Copy link
Contributor Author

Hey Travis, WTF?

@tannerlinsley
Copy link
Collaborator

Wait. I got it to fail locally. Try this:

const rendered = render(<Page />)

    await waitForElement(() => rendered.getByText('error'))

    rendered.debug()

    // query should fail `retry + 1` times, since first time isn't a "retry"
    await waitForElement(() => rendered.getByText('Failed 2 times'))

    rendered.debug()

@tannerlinsley
Copy link
Collaborator

Looks like it never flushes the error state because the delay is so small? Thoughts?

@cherniavskii
Copy link
Contributor Author

To minimize the delay i've set retryDelay to 1.
Will check if it works

@tannerlinsley
Copy link
Collaborator

Oooohhhh, I don't think the error status is reached until the very end. It stays in the loading state until the final failure or success.

@tannerlinsley
Copy link
Collaborator

And I am pretty sure that is desired from the UI/UX standpoint out of the box.

@cherniavskii
Copy link
Contributor Author

Yeah, this is fine

@tannerlinsley
Copy link
Collaborator

tannerlinsley commented Feb 27, 2020

Something still doesn't seem right tho. Try logging out the failureCount in the render function....

@tannerlinsley
Copy link
Collaborator

tannerlinsley commented Feb 27, 2020

Oh wait. I forgot to build. I think it's good. This is what I have now:

it('should retry specified number of times', async () => {
    const queryFn = jest.fn()
    queryFn.mockImplementation(() => {
      return Promise.reject('Error test')
    })

    function Page() {
      const { status, failureCount } = useQuery('test', queryFn, {
        retry: 1,
        retryDelay: 1,
      })

      return (
        <div>
          <h1>{status}</h1>
          <h2>Failed {failureCount} times</h2>
        </div>
      )
    }

    const rendered = render(<Page />)

    rendered.getByText('loading')

    // query should fail `retry + 1` times, since first time isn't a "retry"
    await waitForElement(() => rendered.getByText('Failed 2 times'))

    rendered.debug()

    expect(queryFn).toHaveBeenCalledTimes(2)
  })

@cherniavskii
Copy link
Contributor Author

Let's try

@cherniavskii
Copy link
Contributor Author

Wow, I had to set 50s timeout to make it pass :D

I don't have a better solution for now, at least it works

@tannerlinsley
Copy link
Collaborator

That just doesn't seem right though... like something else is wrong.

@tannerlinsley
Copy link
Collaborator

Have you tested this in the browser and played with it there?

@cherniavskii
Copy link
Contributor Author

@cherniavskii
Copy link
Contributor Author

Same with tests - they're passing really fast locally.
That single test takes 4.587s

@tannerlinsley
Copy link
Collaborator

That seems really strange. I want to get to the bottom of it.

@tannerlinsley
Copy link
Collaborator

I think I found the reason. queryCache is shared among test instances, which are all running in parallel I think...

@tannerlinsley
Copy link
Collaborator

Well, no, they're in isolated workers.... What is going on!!!

@iamchanii
Copy link

I'd experienced the issue what almost same it. the test passed on my machine but CI failed by timeout. so I had to google and found a solution.

if this project use Jest, would you like to try --maxWorkers=2 flag? it is working for my case without increase timeout seconds. but there is a slight drop in performance.

@cherniavskii
Copy link
Contributor Author

@iamchanii Unfortunately that didn't help - 9f46b36

@cherniavskii
Copy link
Contributor Author

queryCache is shared among test instances

I think this is true, looking into this...

@tannerlinsley
Copy link
Collaborator

Alright, I found something that might help us debug. Change the queryKey to something other than test (or something unique to the entire test suite) and watch it pass 🤯

@cherniavskii
Copy link
Contributor Author

Yeah, I've noticed that too
But I found something else. Which is really weird. Would you have time for a call on Zoom?

@tannerlinsley
Copy link
Collaborator

Yeah

@cherniavskii
Copy link
Contributor Author

I've sent you a link in Twitter DM

@tannerlinsley tannerlinsley merged commit 6eb3997 into TanStack:master Feb 28, 2020
@cherniavskii cherniavskii deleted the useQuery-retry-number branch February 28, 2020 17:38
Copy link

nx-cloud bot commented Apr 1, 2024

View your CI Pipeline Execution ↗ for commit 22ccd91.

Command Status Duration Result
nx affected --targets=test:sherif,test:knip,tes... ✅ Succeeded <1s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1s View ↗

☁️ Nx Cloud last updated this comment at 2025-04-27 00:15:09 UTC

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.

3 participants