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

Expecting result.error after "rerender" is not working #74

Closed
danielkcz opened this issue May 16, 2019 · 3 comments
Closed

Expecting result.error after "rerender" is not working #74

danielkcz opened this issue May 16, 2019 · 3 comments
Labels
bug Something isn't working

Comments

@danielkcz
Copy link
Contributor

danielkcz commented May 16, 2019

  • react-hooks-testing-library version: 0.5.0
  • react-testing-library version: 7.0.0
  • react version: 16.8.6
  • node version: 10.12.0
  • yarn version: 1.13.0

Relevant code or config:

const { result, rerender } = renderHook(
    ({ second }) => {
        const obj = second ? { foo: "brr", baz: "new" } : { foo: "baz" }
        return useAsObservableSource(obj)
    },
    { initialProps: { second: false } }
)
rerender({ second: true })
expect(result.error).toMatchInlineSnapshot(
    `[Error: the shape of objects passed to useAsObservableSource should be stable]`
)

What you did:

I had to edit the following line and removed the state check to make the test passing.

https://github.com/mpeyper/react-hooks-testing-library/blob/934423c3ffc4b6b546a88fce4b79b1179cc27007/src/index.js#L30

What happened:

Without that modification, the result.error after the rerender is undefined

Reproduction:

Failing test: https://travis-ci.org/mobxjs/mobx-react-lite/builds/533269870#L484
Source code: https://github.com/mobxjs/mobx-react-lite/blob/no-observable-source-failing/test/useAsObservableSource.test.tsx#L344

Problem description:

I am not entirely sure what's happening there. The error is caught within componentDidCatch, but the resultContainer.setError is not called at all. It's really strange.

Suggested solution:

I think that for the purpose of testing the children should be always returned from ErrorBoundary. React will remount it anyway, so there is no problem of endless loop. The state.hasError is more useful in real apps where you want to inform a user about the problem and offer some way out of it.

Edit: Later I realized my change has broken other tests, so it's the not correct way either.

@danielkcz danielkcz added the bug Something isn't working label May 16, 2019
@mpeyper
Copy link
Member

mpeyper commented May 16, 2019

I'll take a look when I get to work. I've previously stated (#50) that I'm just going to remove the error boundary as it seems to be causing more issues than it was helping me solve (not catching promises for Suspense).

I'll see if going back to a try/catch makes this work as expected.

danielkcz added a commit to danielkcz/react-hooks-testing-library that referenced this issue May 16, 2019
@danielkcz
Copy link
Contributor Author

danielkcz commented May 16, 2019

Yes, you are right, the try/catch fixes this particular scenario perfectly.

I've pushed the changes I've made ☝️ to the fork if you want to base the work on that.

@mpeyper
Copy link
Member

mpeyper commented Jun 2, 2019

I've brought in your changes with a slight adjustment to fix the Suspense tests. I've also added you as a contributor to the README, but if you don't want that let me know and I'll revert that commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants