`renderHook` from /dom/pure has global side-effects

[eps1lon]

• react-hooks-testing-library version: 5.0.0
• react-test-renderer version: not installed
• react version: 17.0.1
• node version: 17.0.1
• npm (or yarn) version: yarn@1.22.0

Relevant code or config:

const { renderHook } = require("@testing-library/react-hooks/dom/pure");

let consoleError;
beforeEach(() => {
  consoleError = console.error;
});

afterEach(() => {
  if (console.error !== consoleError) {
    throw new Error("Did not cleanup console.error");
  }
});

test("pure", () => {
  function useHook() {
    return useState(false);
  }

  const { result } = renderHook(() => useHook(), {});
});

What you did:

Use @testing-library/dom/pure in karma-mocha though the problem happens in arbitrary testing frameworks I suspect

What happened:

console.error was not cleaned up

Reproduction:

https://github.com/eps1lon/testing-library-react-hooks-impure

Problem description:

We check for mistakes in afterEach if a test was not properly cleaned up. One of those checks looks at console.error.

Suggested solution:

renderHook probably has some global side-effects. These should be documented by explaining why they exists and how to disable them or work around them.

[ljosberinn]

related #541 & #542 & discord discussion - tldr is filter-console is used internally to suppress surfacing react internals console.erroring due to the internal error boundary catching if a hook throws

[joshuaellis]

Hi @eps1lon

As @ljosberinn has pointed out we're currently looking at this, so far we have two proposals (neither are ready to be merged yet) but if you'd like to look at them both and give us your take on them, that'd be really helpful in us implemented a well-received solution.

[eps1lon]

Disabling them makes the most sense for us so #541 seems like it would satisfy that need.

As a component library we generally want testing utilities to interfere with the setup as little as possible since we want to test a bit closer to React/DOM. We already have our own testing infrastructure for errors thrown or logged and don't need @testing-library/react-hooks to do that.

It would IMO make more sense to coordinate with @testing-library/react and maybe jest-dom to decide where error suppression/assertion should happen. /react-hooks doing something that /react isn't might surprise users.

[joshuaellis]

Thank you for that.

I don't think we're trying to interfere per say, the source of us implementing something was to resolve #308 where we weren't able to correctly catch errors in useEffect hooks. The reasoning for suppression is documented here - #539. But maybe we misunderstood the reach of the package.

[eps1lon]

as well as providing various useful utility functions for updating the inputs and retrieving the outputs of your amazing custom hook. This library aims to provide a testing experience as close as possible to natively using your hook from within a real component.

-- https://github.com/testing-library/react-hooks-testing-library#the-solution

I don't see a mention of catching errors nor do I think that catching and suppressing errors is in the spirit of "close to using the hook from within a real component". To be clear, I don't have an opinion about whether this behavior is useful. I just think that the documentation is lacking in this regard.

I can only find mention of errors in the advanced section (https://react-hooks-testing-library.com/usage/advanced-hooks#errors) which only talks about errors during render not effects, timers etc.

The reasoning for suppression is documented here - #539.

Do you plan on documenting this in the actual docs? A PR works for documenting implementation details but this behavior is observable by users.

[joshuaellis]

Do you plan on documenting this in the actual docs?

I think you're misunderstanding. I'm not suggesting we don't update the documentation. I wanted to show you why we did it, so you might have been able to provide a solution you think would work for both your problem and the problem we were trying to solve. However, based on this comment:

To be clear, I don't have an opinion about whether this behavior is useful.

I'll assume you don't have a suggestion. But thank you for your opinon on which current draft PR would be more beneficial to your use case.

If you do want to submit a PR for the documentation that would be more than useful, but at this time we're currently working on solving the issue.

[mpeyper]

You bring up an interesting point here about the /pure import and causing a side-effect, which was unintentional with the v5 changes and we should fix that somehow, although I foresee the code to achieve that getting quite messy (hopefully I'm wrong).

In my ideal world, we would stop catching errors all together and the errors would just be thrown back to the test code, but historically this has been difficult to do due to the react render lifecycle. If the component throws in the initial render, it's fine, but if a subsequent render as the result of an async effect causes the error it's much harder to surface that in a catchable way to the test code.

It would IMO make more sense to coordinate with @testing-library/react and maybe jest-dom to decide where error suppression/assertion should happen. /react-hooks doing something that /react isn't might surprise users.

The issue occurs because we render the error boundary and it isn't specific to using react-dom or jest or using the DOM. The output is coming from react (regardless of renderer) and it felt worse to me to push that to our users, but perhaps we have overstepped our responsibility here?

[eps1lon]

it felt worse to me to push that to our users, but perhaps we have overstepped our responsibility here?

It's not about where this happens for me. The problem I see is that /react works different than /react-hooks. So if I'm a user of /react and introduce /react-hooks (and vice versa) I also have to learn about new library behavior.

For example, rendering a hook with /react-hooks creates a completely different flow than rendering the same hook in a component with /react. This feels a lot like we're going back to testing implementation details. This is already what we're doing by testing the hook itself but I always justified it with "it's just rendering a component that let's me access the return value of a hook". The more features we only add to /react-hooks the more we're slipping back into testing implementation details.

[mpeyper]

It's not about where this happens for me. The problem I see is that /react works different than /react-hooks. So if I'm a user of /react and introduce /react-hooks (and vice versa) I also have to learn about new library behavior.

This is where I think our philosophies diverge a bit. While this library was inspired by, and much of the original implementation did indeed come from react-testing-library, at the end of the day they are trying to solve different problems which is why they have different implementation and behaviour. I believe that any time you introduce a new library into your project you should take the time to learn about it and it's behaviour.

For example, rendering a hook with /react-hooks creates a completely different flow than rendering the same hook in a component with /react. This feels a lot like we're going back to testing implementation details. This is already what we're doing by testing the hook itself but I always justified it with "it's just rendering a component that let's me access the return value of a hook".

If all this library did was wrap the hook call in a component and remove a handful of lines out of your test file then I don't think we would exist at all.

Fundamentally, hooks are not components and components are not hooks. They have different use cases and different lifecycles. The more we see users moving their complex logic into custom hooks and the more we see people wanting to ensure all the ways that hook will behave, the more use cases we are going to try to support.

The more features we only add to /react-hooks the more we're slipping back into testing implementation details.

I'm not sure what this basis for this commentary is. I don't believe that that a user wanting to test that triggering an effect that causes an error to be thrown is testing implementation detail. I do believe that the user having to deal with the console output from our use of an error boundary is an implementation detail and is what we were trying to trying to protect them from.

If React didn't log the error and we didn't patch console.error would you still have a concern? Is the problem just that we messed up and don't restore the the console for the pure import (a legitimate issue with the current version and one we are attempting to fix).

Is the concern that we have have result.error instead of allowing the error to bubble out to the test code? I've mentioned a few times that I'd love to remove it, but I haven't been able to have it raise out in all instances where I believe it would be important. It was implemented before act existed so it may be possible now and I am planning on trying to try it it again, but I stumble somewhere every time I've tried before, usually around async update to state that case the error on a subsequent render.

---

Please don't feel like I'm being dismissive of your concerns here. I'm genuinely trying to understand and think about what the implications of this discussion might be to the library in the future.

[eps1lon]

I believe that any time you introduce a new library into your project you should take the time to learn about it and it's behaviour.

This is just not reality but I can't tell you that. I can only show you that. Maybe me, as a testing-library maintainer, being confused by this behavior helps you understand why this mentality is problematic.

So we still have the documentation issue. This behavior is underdocumented which is especially problematic since it's not far fetched to assume that these libraries work similarly considering they are under the same org.

Anyway, I'll be using a thiner wrapper in the future that doesn't require to learn a new library. Getting co-workers on board with testing is hard enough. Expecting them to learn yet another library is not viable.

[mpeyper]

This is just not reality but I can't tell you that. I can only show you that. Maybe me, as a testing-library maintainer, being confused by this behavior helps you understand why this mentality is problematic.

I honestly don't know what else to say. We don't generate markup like most of the libraries here. We don't deal with user interaction like most of the libraries here. We have no dependency on dom-testing-library or a DOM like most of the libraries here. Maybe we don't belong here?

I'm genuinely curious what you want this api to look like because I do not know how to make our API 100% compatible with react-testing-library's API and support the use cases our users have been asking for. I've been looking for better ways for well over a year and nothing has worked out or been objectively better than what we have now.

So we still have the documentation issue. This behavior is underdocumented which is especially problematic since it's not far fetched to assume that these libraries work similarly considering they are under the same org.

Just for clarity, which behaviour are you referring to?

All the PRs up have various levels of documentation around the error boundary, console suppression and how to disable it. Edit: these are live now if you want to read them and suggest any further information you would want to see about them. References are here and here.

Anyway, I'll be using a thiner wrapper in the future that doesn't require to learn a new library. Getting co-workers on board with testing is hard enough. Expecting them to learn yet another library is not viable.

That's fine. I suspect that this library is not for you and that's fine too.

I'd be interested to see what your thin wrapper ends up looking like.