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

Testing components that use useQueryState? #259

Open
rswerve opened this issue Oct 14, 2021 · 12 comments
Open

Testing components that use useQueryState? #259

rswerve opened this issue Oct 14, 2021 · 12 comments

Comments

@rswerve
Copy link

rswerve commented Oct 14, 2021

I'm curious if anyone has written unit tests for a component using this library? I'm using it in a Next.js app (works beautifully, by the way) but when I try to test the component, it bombs out with "Cannot read property 'replace' of null" from this line:

https://github.com/47ng/next-usequerystate/blob/064ecfdaf093bbde2cc72db008685d4a588efed5/src/useQueryState.ts#L108

@franky47
Copy link
Member

I've been wondering the same thing about how to unit-test the library itself to be honest.

If anyone has suggestions, I'd be happy to add them to the README.

@karuhanga
Copy link

karuhanga commented Dec 12, 2021

Unfortunately ended up rewriting this(here) to make use of router.query instead of window.location.search so that I could use https://www.npmjs.com/package/next-router-mock.

My initial hope was I'd just update the and do a PR, but there was some weirdness- likely with the useMemo optimizations. Perharps if someone threw more light on why we chose not to use router.query in the first place(only found the comment, but not much more context than that), I could take a stub on the PR again.

[great work btw :)]

@franky47
Copy link
Member

franky47 commented Dec 12, 2021

Thanks for your work and feedback @karuhanga.

The reason why I didn't use router.query was for optimisation: I did not want useQueryState('foo') to cause rerenders when any other part of the router changed (path, unrelated query keys, hash etc..).

If this goes against the adage "make it work, make it right, make it fast, in that order", then I'd be happy to reconsider this decision.

Your comment raised an interesting idea: why stub the underlying implementation of this library (the need for a router) when we could instead provide a mock of useQueryState, where routing behaviour can be controlled and spied on from test code ?
Would this be an interesting approach ?

@karuhanga
Copy link

The reason why I didn't use router.query was for optimisation: I did not want useQueryState('foo') to cause rerenders when any other part of the router changed (path, unrelated query keys, hash etc..).

I see. I think it might be possible to prevent these rerenders with a strict memoization, maybe based on router.query.key?

Your comment raised an interesting idea

Yes. That was actually something I considered(e.g using useState as a mock impl in tests). It might also better since the router doesn't have a hash component yet anyway.
I don't see any big reason that wouldn't work, I was already using the mock anyway so I went that direction. I suspect most people are already using that mocking package as well, so maybe something else to think about is if we want to expand the scope of this package. I liked that it was small and neat.

Generally though, I think depending on two different sources of truth for what the queries are is not great.

@leon486
Copy link

leon486 commented Aug 12, 2023

Can you provide information about how you ended up testing this? I'm having the same problem and rewriting the package itself is something I would prefer to avoid.

I'm curious if anyone has written unit tests for a component using this library? I'm using it in a Next.js app (works beautifully, by the way) but when I try to test the component, it bombs out with "Cannot read property 'replace' of null" from this line:

https://github.com/47ng/next-usequerystate/blob/064ecfdaf093bbde2cc72db008685d4a588efed5/src/useQueryState.ts#L108

@franky47
Copy link
Member

franky47 commented Sep 9, 2023

As I'm working on shipping an update that supports the app router, I'd like to collect some feedback and ideas on how to make it easier to test these hooks.

In the upcoming API, the setState query updater function will return a Promise to the updated URLSearchParams. This might make it easier to unit-test the hook itself, but won't be much help in a synchronous component.

For testing components that use the hook, a black box approach would be recommended:

  1. Trigger an action in the component (eg: click a button). That action calls a query state updater function.
  2. Assert that the state update had an effect on the component (eg: changing text or updating the DOM)
  3. Assert that the URL has changed accordingly

Points 1 and 2 are akin to testing regular React.useState hooks (which is exactly what there is under the hood now), so let's focus on testing point 3.

Since URL updates are now asynchronous to support batching and throttling, it may become more difficult to test. However, as the library uses an event emitter internally, we could expose a special key that emits URLSearchParams when updates are applied to the URL. Test code could subscribe to this key using mock callbacks, and assert that they have been called with certain expected values.

On the other hand, the initial issue had to do with testing the hook outside of a Next.js router context. It should 1 now be possible to rely only on the test environment providing a Web History API mock, which could also be hooked for assertions.

Edit: initial attempts to mount a component using useQueryState in a RTL test fixture failed due to the dependency on useSearchParams and useRouter failing to find a Next.js router context. This would require mocking the router, using next-router-mock, for which app router support is being worked on.

Notes: some useful links to how to mock next/navigation:

Footnotes

  1. The hooks still rely on the Next.js router in the app directory for providing server-side access to the querystring via useSearchParams. Testing the server-side behaviour of the hooks vs the client side (depending on whether the test environment exposes window) seems like a mine field to me..

@sgoodrow-zymergen
Copy link

Any workarounds in the meantime? In particular, I'd like to be able to use next-router-mock to initialize a url (and search query param) and have nuqs deserialize its state from it.

@ivasilov
Copy link

Just another issue to add:

 ⨯ Error: invariant expected app router to be mounted
    at useRouter (/Users/ivasilov/Documents/supabase/node_modules/next/dist/client/components/navigation.js:157:15)
    at useQueryState (file:///Users/ivasilov/Documents/supabase/node_modules/nuqs/dist/index.js:305:18)

This is on a pages router in a page with no SSR functions defined (getServerSideProps etc).

I've localized the issue to NODE_ENV=test env var. The variable is used in Playwright tests (to force the Nextjs app to use env.test file), which should be ok according to Nextjs docs. It's happening probably because of the import of next/navigation.js.

@PavelYatskevich
Copy link

I have similar issue to previous comment
image

We currently migrating from jest to vitest and getting this error atm, maybe some on faced same issue, thanks

@pimmee
Copy link

pimmee commented Sep 24, 2024

I also ran into issues trying to test a component using useQueryState with initial values. It works fine if mockImplementation is used only for single test is run, but ran into problems when trying to run two tests sequentially with different initial values.

I added logs and found that the internal queueValue prevents the search params mock for the second test to be initialized correctly.

setupTests.ts

export const mockUseRouterPush = jest.fn();
export const mockUseRouterReplace = jest.fn();
export const mockUsePathname = jest.fn().mockImplementation(() => '/');
export const mockUseSearchParams = jest.fn().mockImplementation(() => new URLSearchParams());
export const mockUseRouter = jest
  .fn()
  .mockImplementation(() => ({ push: mockUseRouterPush, replace: mockUseRouterReplace }));
jest.mock('next/navigation', () => ({
  useRouter: mockUseRouter,
  usePathname: mockUsePathname,
  useSearchParams: mockUseSearchParams,
  useSelectedLayoutSegment: jest.fn(),
  useSelectedLayoutSegments: jest.fn(),
}));

mytest.test.tsx

import { mockUseSearchParams } from '@app/utils/test/setup/setupTests';

const testCases = ["a", "b"]
test.each(testCases)('can do %s, async letter => {
       const mockSearchParams = new URLSearchParams({ letter });
       mockUseSearchParams.mockImplementation(() => mockSearchParams);
       ...
 }

The internal queueValue is "a" for the second test run while initialSearchParams is "b" which results in the value being set to a instead of b for the second test run.

node_modules/nuqs/dist/index.cjs
image

I looked for a way to clear the updateQueue but failed to find a way to do so.

Edit: I found that running the following at the end of the test flushed the queued value.

test.each(testCases)('can do %s, async letter => {
    const mockSearchParams = new URLSearchParams({ letter });
    mockUseSearchParams.mockImplementation(() => mockSearchParams);
    ...
    expect(screen.getByText(template.category)).toBeInTheDocument();
    await act(() => new Promise(resolve => setTimeout(resolve, 0))); // this solved it
});

@franky47
Copy link
Member

Some updates on this front: the upcoming Adapters feature in #644 (which opens nuqs to other React frameworks like Remix, React Router, Vite, Astro etc), also comes with a testing adapter, which should make unit-testing setup & assertions much easier, by not relying on Next.js internals at all.

Here's a sneak peak of what a test (using Vitest & Testing Library) might look like:

import { render, screen } from '@testing-library/react'
import userEvent from '@testing-library/user-event'
import { NuqsTestingAdapter, type UrlUpdateEvent } from 'nuqs/adapters/testing'
import { describe, expect, it, vi } from 'vitest'
import { CounterButton } from './counter-button'

it('should increment the count when clicked', async () => {
  const user = userEvent.setup()
  const onUrlUpdate = vi.fn<[UrlUpdateEvent]>()
  render(<CounterButton />, {
    // Setup the test by passing initial search params / querystring:
    wrapper: ({ children }) => (
      <NuqsTestingAdapter searchParams="?count=42" onUrlUpdate={onUrlUpdate}>
        {children}
      </NuqsTestingAdapter>
    )
  })
  // Act
  const button = screen.getByRole('button')
  await user.click(button)
  // Assert changes in the state and in the (mocked) URL
  expect(button).toHaveTextContent('count is 43')
  expect(onUrlUpdate).toHaveBeenCalledOnce()
  expect(onUrlUpdate.mock.calls[0][0].queryString).toBe('?count=43')
  expect(onUrlUpdate.mock.calls[0][0].searchParams.get('count')).toBe('43')
  expect(onUrlUpdate.mock.calls[0][0].options.history).toBe('push')
})

The feature is still in an RFC stage, if you want to join the discussion to give feedback: #620

@PavelYatskevich
Copy link

Hi all, I got the solution from here: vercel/next.js#48937 (comment)

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

8 participants