-
Notifications
You must be signed in to change notification settings - Fork 251
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
Error thrown when using react hooks #128
Comments
I'm waiting for testing-library/react-testing-library#281 to be closed to decided what to do here. Also, following #119 we might add support to Vue which would prevent us from importing from For now, I prefer not to address this issue, but it's something I want to get solved in the near future. Thanks for bringing it up! |
Yes, I agree adding a direct dependency on react-testing-library is not a good solution. Would it be possible to expose an API to configure it? Similar to how enzyme exposes a way of defining the react version adapter. So I could set this up once and the tests wouldn't have to care. |
I'd rather not have any configuration if possible. I prefer to wait for the two issues I mention to be resolved before doing anything here |
How about dividing user-event into multiple flavours? e.g. |
@Gpx, testing-library/react-testing-library#281 has been closed. |
I think now is a good time to reconsider this. React testing library and Vue testing library both expose their own customized versions of @Gpx would you consider an API like this? // In test setup file
import user from 'user-event'
import { fireEvent } from 'react-testing-library'
user.customFireEvent(fireEvent)
// or `configureFireEvent`
// or `customizeFireEvent` // Test file
import user from 'user-event'
import { render } from 'react-testing-library'
test('foo', () => {
const { getByText } = render(...)
// React updates are wrapped in `act` automatically!
user.type(getByText('asdf'), 'invalid text')
expect(getByText('Submit')).toBeDisabled()
}) cc @kentcdodds @afontcu I'm curious what you think as well |
That looks reasonable to me. |
I believe there are three (main) paths here. Some food for thought:
|
@afontcu For option 3, would that mean that RTL/VTL/etc would each re-export their own copy of |
I'm not sure how I feel about option 3. I'll add that if you're seeing act warnings, it's not because user-event doesn't handle act. Actually, the fact that For example: https://github.com/kentcdodds/user-event-act (no act warnings happen in those tests). So as long as you're using RTL's async utilities then you should be solid. Often I find that these warnings show up when someone has a test that doesn't wait for side-effects to finish (like having one test that verifies that a loading spinner shows up, but doesn't wait for the loading to actually complete and does that in another test. These tests should be combined... I think I'll blog about this today). So the urgency for me on this issue is very low because I don't think it should be even necessary. |
That's what I noticed too. I'm not sure we should do anything in this issue unless someone can provide an example of a test that would be fixed by wrapping |
I think there's a use case that still necessitates events to be wrapped in If using a third-party library that triggers async logic that you have no method of As a concrete example, I'm referring to the I've tested it and this works with no errors:
And this
If there's a RTL async utility that I should be using and am just missing here, I'm all ears 🙂 |
I have same issue of My general observation is that if I use |
If someone could make a codesandbox that shows two tests (one that wraps |
This should be passing without needing |
Unless I'm missing the point, I'm still finding this an issue. Minimal BasicForm.js:
Minimal BasicForm.test.js:
npm run test BasicForm.test.js:
If I wrap Is this the same issue, or is my logic flawed? Edit: Ignore me, I hadn't quite updated to the latest version of one of the dependencies. Arrgh. |
Unless I'm (again) missing the point, I'm still finding this an issue for Even more minimal BasicForm.js:
Even more minimal BasicForm.test.js:
npm run test BasicForm.test.js:
Wrapping Edit: Actually, I can make these even simpler by removing the input field. That results in only one error. Is it simply that the whole form is redrawing? |
Can you simplify it further so it's not using any dependencies? It's likely that Note also, |
If I strip it right back, all is fine: BasicForm.js:
BasicForm.test.js:
The problem is only present when I use BasicForm.js:
npm run test BasicForm:
|
I am also running into this, in my case, it's because a click triggers some async fetch, which calls a state hook. I'm using the following packages: "@testing-library/jest-dom": "^5.10.1",
"@testing-library/react": "^10.3.0",
"@testing-library/user-event": "^12.1.3", I'll try to find time to simplify my example and potentially open a new issue with those reproduction steps. |
This issue is still happening. |
I'm hitting this when using react-hook-forms and userEvent.click too, since this issue is closed and is not exactly the same (not specific about react-hook-forms) should we create a new one? |
@matburnham I'm creating a new issue with your example, hope you don't mind. |
This problem is caused by a React rerender after the end of the test because of an async To make it works I have added a import * as React from 'react'
import {useForm} from 'react-hook-form'
const App = () => {
const {handleSubmit, formState} = useForm()
const status = formState.isSubmitting
? 'submitting'
: formState.isSubmitted
? 'submitted'
: 'idle'
const onSubmit = (data) => {}
return (
<form onSubmit={handleSubmit(onSubmit)}>
<input type="submit" />
<span role="status">{status}</span>
</form>
)
}
export default App import React from 'react'
import {render, screen, waitFor} from '@testing-library/react'
import userEvent from '@testing-library/user-event'
import App from './App'
import '@testing-library/jest-dom/extend-expect'
it("doesn't fall over on userEvent.click", async () => {
render(<App />)
userEvent.click(screen.getByRole('button'))
waitFor(() => expect(screen.getByRole('status').toEqual('submitted')))
}) |
But what happens if there's no expected UI change? I'm getting a similar error when opening a dropdown with onChange validation, there's no visible change when the validation finishes, and the code is inside react-hook-form. I'm trying to create a repo with the issue. |
If there's no UI change, then there's probably some side-effect that you could observe. If there's no observable side-effect or UI change, then what is it even doing? 😅 |
If the field is valid, then it does nothing that's true. But, if there's an error, then it adds a classname to show the field as invalid. The problem comes when the field is valid. Here's an example repository: https://github.com/DamianPereira/user-event-react-hook-form-bug The field is marked as required, and after changing the value, the state in useForm changes to be valid. I can not do an expect that waits for "the error on the field to change from valid to valid". I could only wait for it if the field gets an invalid value, then I could waitFor the field to be invalid. Edit: Added a more real usecase (the form shows inline validations after values are changed, but if values are valid, there are no visible changes, but setState is still called to empty errors object if needed, or whatever react-hook-form does on positive validations) |
Ah, so what you really want is the ability to assert that nothing happens. This is admittedly tricky to think about, but it's no different from any other kind of test you write. Remember that if you're trying to write your test as instructions to a manual tester they would include the steps:
So your test will need to do this as well. Now, presumably your HTTP requests are mocked out, so the time to run the validation logic should be very fast, but you will want to wait. I did something like this recently in one of my workshop repos: https://github.com/kentcdodds/advanced-react-hooks/blob/560980b10fd10e66fcf5e4f5b4b90bf46cb5fc7b/src/__tests__/02.extra-3.js#L64-L71 It basically involves waiting for a bit before continuing my assertions: // wait for 100ms to make sure no errors appear later
await new Promise(r => setTimeout(r, 100))
// continue with assertions that no errors appear You'll want to make sure that this test can fail as well. Good luck! |
Yes exactly, I want to test that there are no errors for inline validations (while also testing for errors in a different test). Thinking about automated tests as instructions to a manual tester makes a lot of sense. Even then, waiting, even for a second does not fix the warning, this code still shows the warning: it("does not cause act warning", async () => {
render(<DropdownForm />);
const select = screen.getByRole("combobox");
userEvent.selectOptions(select, "2"); // warning right here, shown in console before the 1s delay
await new Promise(r => setTimeout(r, 1000))
expect(select.value).toEqual("2");
// An expectation that the error is not shown can be added here,
// but it would have no effect on the warning since it already fired.
}); The only way to make the warning go away is wrapping the userEvent.selectOptions in an async act like this (non awaited act does not fix the warning): it("does not cause act warning", async () => {
render(<DropdownForm />);
const select = screen.getByRole("combobox");
await act(async () => {
await userEvent.selectOptions(select, "2");
});
expect(select.value).toEqual("2");
}); Is this the way I'm supposed to make the warning go away in the scenario in the codepen? Or is there a way to make the "wait a bit" solution you wrote work for this scenario? |
See #457 (comment) You need to wrap the hook/state change in Inject a validation callback that resolves a |
The problem is that the whole validation code usually sits inside the react-hook-form library, and even when it isn't (like in the codepen example), it is still inside the component code and not available to the test. i.e I can not provide a validation callback from the test, unless I drill down the prop to the correct field. I'm not sure what you mean by "before the callback passed to act returns.", do you mean that I'd need that promise to make the non-async act example work? This already works fine if I add the act, but make it an async one: it("does not cause act warning", async () => {
render(<DropdownForm />);
const select = screen.getByRole("combobox");
await act(async () => {
await userEvent.selectOptions(select, "2");
});
expect(select.value).toEqual("2");
}); Even if I could add a promise that resolved after validation finishes, I don't think it would fix the warning (since the warning happens immediately after |
act( /* Here you pass a function to act */ async () => {
/* Perform your click/type/selectOption ...*/
await thePromiseThatResolvesWhenValidationIsDone // wait for the change
/* Here you return from what is done inside act.
If any change happens later (but before the test ends) you will receive a warning. */
}) See https://developer.mozilla.org/en-US/docs/Web/JavaScript/EventLoop |
I think I'm starting to understand why wrapping in act works (thanks for the explanation). What's not clear is if the recommended solution to the warning here is to actually wrap it in act myself? I was under the idea that it was a workaround/warning silencer, and that the best practice was to wait for side effects, or somehow wait for code to execute in a more natural way, and that calling act manually was a last resource. Or that userEvent could wrap it's methods in act calls as the original issue says:
|
But if the event handler returns and executes the change per timeout/promise you need to wait for the change. And then you need to wrap that change in // This probably works, too. But a change might occur between the act calls inside userEvent.
userEvent.whatever(element)
act(() => { /*wait for some change */ })
Your example does not really test anything in your system under test. So it is hard to tell if you should just wrap it in |
This is of course a reduced example, my actual tests look something like: Thing can be created:
But at point 1, I get the act warning, even if after that happens I'm making assertions. I have lots of inputs (I have big forms which need to be filled before submitting, so each one of the happy path tests right now generates over 20 act warnings.). I could wrap in an async and await act every time I edit a field, but it feels cumbersome, and makes each simple 1 line interaction with an input be a 3 line await/async complex thing. This makes my tests hard to write/read. I made the tests in the reproduction repository resemble the real world a bit more: https://github.com/DamianPereira/user-event-react-hook-form-bug I created 2 tests, one which is showing the warning, and the one wrapped in act that does not, is the second test the recommended way of writing tests for this usecase? |
testing To submit the form you should it('submits and does not cause act warning with userEvent without act', async () => {
const submitCallback = jest.fn()
render(<DropdownForm onSubmit={submitCallback} />)
const select = screen.getByRole('combobox')
userEvent.selectOptions(select, '2')
userEvent.click(screen.getByRole('button'))
await waitFor(() => expect(submitCallback).toHaveBeenCalled())
}) import React from 'react'
import {useForm} from 'react-hook-form'
const DropdownForm = ({onSubmit}) => {
const {register, errors, handleSubmit} = useForm({
mode: 'onChange',
})
return (
<form onSubmit={handleSubmit(onSubmit)}>
<select
name="test"
ref={register({
validate: (value) => {
return value !== '3'
},
})}
>
<option value={1}>1</option>
<option value={2}>2</option>
<option value={3}>3</option>
</select>
{errors.test && "Option can't be 3!"}
<button type="submit">Submit</button>
</form>
)
}
export default DropdownForm |
it('submits and does not cause act warning with userEvent without act', () => {
const submitCallback = jest.fn()
render(<DropdownForm onSubmit={submitCallback} />)
// move the async part into act to prevent refactored code that changes the operation order to mess with this test
act(async () => {
userEvent.selectOptions(screen.getByRole('combobox'), '2')
userEvent.click(screen.getByRole('button'))
await waitFor(() => expect(submitCallback).toHaveBeenCalled())
})
}) DRY. If this is a reoccurring pattern, you can shorten the tests by moving that code into a setup function - e.g.: function setupAndSubmit(action) {
const submitCallback = jest.fn()
render(<DropdownForm onSubmit={submitCallback} />)
act(async () => {
action()
userEvent.click(screen.getByRole('button'))
await waitFor(() => expect(submitCallback).toHaveBeenCalled())
})
return { submitCallback }
}
it('submit selected option', () => {
setupAndSubmit(() => userEvent.selectOptions(screen.getByRole('combobox'), '2'))
// perform some assertions...
}) |
@marcosvega91 Interesting, waiting for a certain amount of time does nothing ( Doing this causes no warning: it("changes value and emits no warning", async () => {
const submitCallback = jest.fn();
render(<DropdownForm onSubmit={submitCallback} />);
const select = screen.getByRole("combobox");
userEvent.selectOptions(select, "2");
await waitFor(() => expect(select.value).toEqual("2"));
} Even if a plain |
@ph-fritsche The repetition comes from having lots of different inputs, not from making changes to the same one. But I guess I could write a generic helper method for "modify this input, but wrap it in act". Kind of like a wrapper around userEvent which wraps everything in async act, but that seems hacky. |
Anyone getting the warning this issue was originally raised about is experiencing something completely different from the original issue so I'm going to lock it. Please read this: https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning If you're still certain that you're getting the warning in error and it's user-event's issue, then open a new issue with a https://kcd.im/repro. Thanks! |
Description
Given a react component using hooks
When a you attempt to use a user event e.g. type
Then you receive the error below
Reason
As far as I understand this is because we are using the
fireEvent
from@testing-library/dom
directly and not from@testing-library/react
which means that it is not being inact
.Solution
A possible solution would be to allow for the user to provide a
fireEvent
implementation or allow for this to be configured in test setupThe text was updated successfully, but these errors were encountered: