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 v6 w/ React Testing Library Docs #7169

Closed
wants to merge 12 commits into from
Closed

Testing v6 w/ React Testing Library Docs #7169

wants to merge 12 commits into from

Conversation

thatzacdavis
Copy link

@thatzacdavis thatzacdavis commented Feb 26, 2020

  • Intro
  • Basic Test
  • Links and Navigation
  • Routes and Redirects


```jsx
test('renders react router header', () => {
const { getByText } = render(<Router><App /></Router>);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're presently recommending people import screen from RTL instead of destructuring the render result. This works better with autocomplete and makes each test a little simpler.

👀 @kentcdodds

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, so this would be:

import {screen, render} from '@testing-library/react';

test('renders react router header', () => {
  render(<Router><App /></Router>);
  const header = screen.getByText('Welcome to React Router!');
  expect(header).toBeInTheDocument();
})

It would be even better to recommend the wrapper option so rerender works as expected:

import {screen, render} from '@testing-library/react';

test('renders react router header', () => {
  render(<App />, {wrapper: Router});
  const header = screen.getByText('Welcome to React Router!');
  expect(header).toBeInTheDocument();
})

And then to take it to the next level, there's a custom render: https://testing-library.com/docs/react-testing-library/setup#custom-render

Hope that context is helpful!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that in the repo README @alexkrolick but not in the docs site, the destructuring is still there, which is what I've been going off of for a while.

I can definitely update these examples, and I can also put in a PR for the RTL docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, we're still in the process of updating things to the new recommendations. PRs would be appreciated!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I can definitely help with that.

Thanks for all the help with this! I'll work on some updates for this tonight.

const { getByText } = render(<Router><App /></Router>);

const aboutLink = getByText('About');
act(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to use act around fireEvent because it's already wrapped in act.

You should rarely need to use act in regular tests. For more on this: https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning

window.history.replaceState({}, '', '/');
});

afterEach(cleanup);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, and cleanup is unnecessary and happens automatically now: https://testing-library.com/docs/react-testing-library/api#cleanup

Copy link
Contributor

@alexkrolick alexkrolick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there is a parallel effort over here #7154

If there are changes for v6, we'd like to update the RTL recipe here https://testing-library.com/docs/example-react-router

One side note is that in the RTL docs we try to avoid language like "simple", "easy", etc. - not everyone is at the same level or has the same background knowledge. If possible we'd like to avoid making that impression over here as well 😄

@thatzacdavis
Copy link
Author

@will-t-harris want to join forces on this?

This also moves the `Testing Routes and Redirects` section below `Testing Links and Navigation`
@will-t-harris
Copy link

It seems that the work you've done covers the little that I got through in #7154, unless I'm missing something. I'd be happy to contribute more, but I'm not sure where to start. My experience with RTL is not very deep.

@thatzacdavis
Copy link
Author

It seems that the work you've done covers the little that I got through in #7154, unless I'm missing something. I'd be happy to contribute more, but I'm not sure where to start. My experience with RTL is not very deep.

I haven't handled any redirects yet, which you could build off the tests I have if you want.

@will-t-harris
Copy link

I haven't handled any redirects yet, which you could build off the tests I have if you want.

I can try to make some headway on that this evening -- full disclosure, I've never used redirects with react-router. Happy to learn about them though!

@thatzacdavis
Copy link
Author

I can try to make some headway on that this evening -- full disclosure, I've never used redirects with react-router. Happy to learn about them though!

Same haha, but we got this 💪

@will-t-harris
Copy link

will-t-harris commented Mar 6, 2020

I have some edits and additions to the content you've added on this fork so far, but I can't push to the branch -- I think because I'm not a contributor on your fork.

Could you please add me so that I can contribute to this PR?

Edit: I suppose I could branch off of your dev branch and make a PR into this? But that seems much more convoluted

@thatzacdavis
Copy link
Author

I have some edits and additions to the content you've added on this fork so far, but I can't push to the branch -- I think because I'm not a contributor on your fork.

Could you please add me so that I can contribute to this PR?

Edit: I suppose I could branch off of your dev branch and make a PR into this? But that seems much more convoluted

Just invited you to my fork

@will-t-harris
Copy link

Added an example of testing a redirect -- not entirely confident that it's a good example, but it's at least a starting point to iterate on.

@thatzacdavis
Copy link
Author

It looks good to me -- maybe @mjackson can weigh in here?


TODO
Since we've wrapped our `App` component in the `Router` in our `index.js` file, we do have to wrap it around each of our individual component tests, otherwise `history` will be undefined. If the `Router` had been inside of our `App`, then we would not have to wrap it inside of our tests.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though I read the above section about "Adding RR via CRA" I find it a bit confusing to hold all these references in my head. I think it would help to explain the exact structure that is required and when you'll need to wrap with Router, in a general way without referencing the example files.


```jsx
test('<App /> renders successfully', () => {
render(<App />, { wrapper: Router });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also show the imports - is this a memory router or other kind? What JSDOM setup is required, if any?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and also show the RTL imports) import {screen, render} from '@testing-library/react'

const aboutLink = screen.getByText('About');
fireEvent.click(aboutLink);

expect(window.location.pathname).toBe('/about');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't be the case if you use an in-memory router (it does not update the window.location). I think there are different opinions about this. The downside of using a "full" router is that JSDOM doesn't give you a way to reset the navigation stack, so if you do something like "go back 3 times" you could end up on a previous test's URL (even if you reset the initial location as described below, the number of items on the stack doesn't reset). On the other hand, memory router doesn't represent what's going in the actual app as well, especially if you have some regular <a> tags mixed in or manual manipulation of the URL.

Copy link

@sadsa sadsa Sep 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using MemoryRouter in my tests and using a test component as a the route to redirect to.

const RedirectPage = () => {
  const [searchParams] = useSearchParams();
  const action = searchParams.get("action");

  if (!action) return <Navigate to="/" />;

  return <></>;
};

 test("Should redirect to the home screen if 'action' query param is missing", async () => {
    function HomePage() {
      return <h1>Home</h1>;
    }
    const { findByRole } = render(
        <Router initialEntries={["/redirect"]}>
          <Routes>
            <Route path="/" element={<HomePage />} />
            <Route path="/redirect" element={<RedirectPage />} />
          </Routes>
        </Router>
    );
    const homePageHeading = await findByRole(/heading/i);
    expect(homePageHeading).toBeVisible();
  });

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if you test the page contents, you can make an assertion. But you can't assert window.location.pathname with MemoryRouter, because it doesn't update that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why I've moved away from using the MemoryRouter and instead I render the same router that's rendered in my source code and interact with window.location directly instead: https://github.com/kentcdodds/bookshelf/blob/d9d70fceaea790dfe57ee5a46c13f9f9cc810675/src/test/app-test-utils.js#L15

Works way better. And I get more confidence out of my tests.


```

This test initializes the `MemoryRouter` with an initial path of `/the-old-thing` and verifies that the `RedirectHandler` correctly changes the path to `/the-new-thing`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this actually happen with a MemoryRouter? I don't think it updates the location. But you could read from the history object (or not use an in-memory router).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked the docs, you're correct that MemoryRouter won't update window.location.

I'm not sure how to set the equivalent of this

initialEntries={["/the-old-thing"]}

using BrowserRouter, in order to set up for the redirect.

Is it acceptable to directly access history.location for testing purposes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know why initialEntries exists (maybe to restore across sessions?). Pushing history and waiting for redirect to result in the new URL seems more realistic to me for web use cases. Again, there are limitations to both approaches. window.location.href = doesn't trigger navigation in JSDOM, for example.

@thatzacdavis
Copy link
Author

@will-t-harris @alexkrolick How does this redirect test look?

import RedirectHandler from "./RedirectHandler";

it("redirects /the-old-thing to /the-new-thing", () => {
window.history.pushState({}, "The Old Thing", "/the-new-thing");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to pushState() to /the-new-thing at the beginning of the test? It seems to me that if this is how the history is set up, then we aren't really testing whether the redirect is happening.

Am I misunderstanding how pushState is functioning here?

@thatzacdavis
Copy link
Author

thatzacdavis commented Mar 8, 2020 via email

@thatzacdavis
Copy link
Author

Since I wrote that test incorrectly it looked like that worked, but alas, putting in a redirect like that does not seem to work.

@will-t-harris
Copy link

will-t-harris commented Mar 11, 2020

So what is the next step forward with this? Should we go back to using MemoryRouter to force the test to start on a specific route?

If so, how are we supposed to read the current path since MemoryRouter won't update window.location?

Perhaps you have some advice @kentcdodds ?

@kentcdodds
Copy link
Member

Here's an example of what I do: https://github.com/kentcdodds/react-testing-library-course/blob/f5a5f685eb0ecaec444a4fb923050dca29859a21/src/__tests__/react-router-03.js

There's also full-on integration tests which disregard the router altogether (and therefore are using the browser router that the app renders):

https://github.com/kentcdodds/react-testing-library-course/blob/f5a5f685eb0ecaec444a4fb923050dca29859a21/src/__tests__/app-03.js

So my recommendation is that if you're rendering a router, then yeah, the MemoryRouter is probably best, and you can return the history that you create. Otherwise, render the whole app and navigate around.

@kentcdodds
Copy link
Member

(This isn't a one or the other either, I suggest people do both).

@thatzacdavis
Copy link
Author

@will-t-harris I'm swamped this week if you want to jump on anything.

@will-t-harris
Copy link

@thatzacdavis I will try to get to this over the weekend, this week has me all over the place

@stale
Copy link

stale bot commented Aug 19, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
You can add the fresh label to prevent me from taking any action.

@stale stale bot added the stale label Aug 19, 2020
@alexkrolick
Copy link
Contributor

I think this would still be useful - anyone want to take it over?

@stale stale bot removed the stale label Aug 19, 2020
@stale
Copy link

stale bot commented Nov 7, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
You can add the fresh label to prevent me from taking any action.

@stale stale bot added the stale label Nov 7, 2020
@stale stale bot closed this Nov 14, 2020
@alexkrolick
Copy link
Contributor

If anyone needs it, there is an example on the React Testing Library site:

https://testing-library.com/docs/example-react-router

@fzaninotto
Copy link

I had problems running several unit tests about routing in my app, and this PR gave me the one thing I missed:

beforeEach(() => {
  window.history.replaceState({}, '', '/');
});

I really think this should appear somewhere in the public documentation.

@alexkrolick
Copy link
Contributor

@fzaninotto can you make a PR to add it to the RTL docs?

@fzaninotto
Copy link

well, isn't it a problem that developers will meet with all testing frameworks? I think it should be part of a "testing with react-router" chapter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants