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

The first test of example-react-router fails #897

Open
FahadAminShovon opened this issue Jul 24, 2021 · 7 comments · May be fixed by #898
Open

The first test of example-react-router fails #897

FahadAminShovon opened this issue Jul 24, 2021 · 7 comments · May be fixed by #898

Comments

@FahadAminShovon
Copy link

FahadAminShovon commented Jul 24, 2021

the first example of example-react-router fails
to be more precise

test('full app rendering/navigating', () => {
  const history = createMemoryHistory()
  render(
    <Router history={history}>
      <App />
    </Router>,
  )
  // verify page content for expected route
  // often you'd use a data-testid or role query, but this is also possible
  expect(screen.getByText(/you are home/i)).toBeInTheDocument()

  const leftClick = {button: 0}
  userEvent.click(screen.getByText(/about/i), leftClick)

  // check that the content changed to the new page
  expect(screen.getByText(/you are on the about page/i)).toBeInTheDocument()
})

To Reproduce Steps to reproduce the behavior:

  1. copy paste the app.test.js from here
  2. run tests

Expected behavior All three test cases should pass

Screenshots
Screenshot 2021-07-24 at 5 50 00 PM

However I could resolve the error by rendering the App component like this

test('full app rendering/navigating', () => {
	// const history = createMemoryHistory();
	// history.push('/about');
	render(<App />, { wrapper: MemoryRouter });
	// verify page content for expected route
	// often you'd use a data-testid or role query, but this is also possible
	// verify page content for expected route
	// often you'd use a data-testid or role query, but this is also possible
	expect(screen.getByText(/you are home/i)).toBeInTheDocument();

	const leftClick = { button: 0 };
	userEvent.click(screen.getByText(/about/i), leftClick);

	// check that the content changed to the new page
	expect(screen.getByText(/you are on the about page/i)).toBeInTheDocument();
});

Screenshot 2021-07-24 at 5 53 18 PM

@eps1lon
Copy link
Member

eps1lon commented Jul 24, 2021

This looks simpler anyway and MemoryRouter is the recommended approach for testing anyway. Want to send a PR that documents MemoryRouter instead?

@FahadAminShovon
Copy link
Author

sure, will send a pr

@alexkrolick
Copy link
Collaborator

alexkrolick commented Jul 26, 2021

I recall this was a point of contention with ReactRouter regarding the recommended approach, because you can't interop with window.location and other APIs that real apps use in conjunction with RR when using MemoryRouter.

@eps1lon
Copy link
Member

eps1lon commented Jul 27, 2021

because you can't interop with window.location and other APIs that real apps use in conjunction with RR when using MemoryRouter.

Shouldn't you use useLocation anyway? Especially because accessing window.location isn't trivial when working with concurrent rendering. Sounds to me like this is the reason they recommend using a different router component to test these incompatibilities.

Regardless, I would rather follow their advise and when they recommend a different approach, we can sync again. Otherwise we should document explicitly why we use a different approach.

@alexkrolick
Copy link
Collaborator

Previous discussion: remix-run/react-router#7169 (comment)

@ryan-sandy
Copy link

Hey, which version of react-router are you using? If you're using the stable version of react-router (v5) and history (v5), the tests will fail. You need to downgrade history to v4 for the route change to work.

Issue 869 documents the incompatibly. My PR to add a note about the dependency incompatibly was was rejected.

@MatanBobi
Copy link
Member

Hi, is this one still relevant for RR v6?

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 a pull request may close this issue.

5 participants