-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: Require await act(...)
#1214
base: main
Are you sure you want to change the base?
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit dd27bd9:
|
5337ab0
to
686a5ba
Compare
Codecov Report
@@ Coverage Diff @@
## alpha #1214 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 192 155 -37
Branches 38 30 -8
=========================================
- Hits 192 155 -37
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
8669481
to
b156f87
Compare
@eps1lon any chance for an explainer on this one please? AFAIR, |
I always knew this day would come 😅 Everything we do in tests really should be async. |
There's a reason why we should do this now and a React specific reason that why we may need to do it in the future. For now consider this test: function App() {
const [ctr, setCtr] = React.useState(0);
async function someAsyncFunction() {
// queue a promises to be sure they all flush
await null;
setCtr(1);
}
React.useEffect(() => {
someAsyncFunction();
}, []);
return ctr;
}
act(() => {
render(<App />, container);
});
expect(container.innerHTML).toEqual("1"); That test will log missing act warnings unless you unmount the tree before you exit your test i.e. we'd have to tell people to unmount in each test instead of relying on a single I'm testing this internally first to see how it behaves across Native and Web and to check we cover most scenarios with a codemod (because that's required to land this now). Once that's done I write out a full explainer. |
I'm not sure that example is a good justification honestly. Normally if you're setting state there is some visual indication of what state you just set and I would recommend people to make an assertion based on the UI that changed from that state update which would typically result in requiring a query which is wrapped in act. Can you show me a example where that wouldn't be a solution? |
I'll add that not always will you have some visual indication that some state changed, but the alternative to a visual indication is typically a function call which can be mocked and the assertion can be inside a waitFor which is also wrapped in act. |
We already don't let people assert between what was committed from render and what was committed from |
True, but when we're talking about something happening asynchronously (even if triggered by useEffect) that is currently possible and understandable. I'm not convinced this is justified. |
Just to see I understood, what we're talking about for the example @eps1lon posted is something like this? function App() {
const [ctr, setCtr] = React.useState(0);
async function someAsyncFunction() {
// queue a promises to be sure they all flush
await null;
setCtr(1);
}
React.useEffect(() => {
someAsyncFunction();
}, []);
return ctr;
}
test("Should show 1", () => {
act(() => {
render(<App />, container);
});
await waitFor(() => {
expect(container.innerHTML).toEqual("1");
})
}) Because this seems to me like a reasonable usage of |
I don't feel like I am really eligible to provide very valuable feedback. But I found this PR trying to wrap my head better around Debugging and understanding these errors is by far the biggest problem I see with testing react apps. They're always really difficult to debug, understand and solve. Sometimes the act-warnings also only appear when you are running multiple tests at once in parallel which makes it all next to impossible. TLDR: Edit: Clarifications |
@eps1lon do we want to move on with this by pushing it to an alpha branch? |
Agreed, that would also give us time to update DTL or our supported Node versions before a major release (see testing-library/dom-testing-library#1255). |
I have a bit of time start of October where I want to revisit this. So testing how good codemod coverage is. Testing react-dom and react-native tests. Checking if we want this in React core and then we advance to alpha. I don't see this blocking Node.js 18 bump. But I would also not force the Node.js 18 bump unless it is necessary or enables some new feature. |
It's necessary for security. Node 16 is intentionally ending support early because its version of OpenSSL is no longer supported, so we should update our Node versions as soon as we can. Fortunately we're already making progress on DTL's major release dropping Node 16 (currently in alpha), so I'd also like to start migrating RTL and our other libraries to use DTL 10 with Node 18+. If this PR isn't ready by then, I'd still support releasing in another major. By then I might want to make some changes to support #1209 anyway. |
I'm going to be recording testing videos for EpicWeb.dev starting tomorrow and I would love to use the alpha for these videos to make sure I'm teaching what will be available when EpicWeb.dev is launched. So yeah, an alpha release would be great for me personally :) |
@kentcdodds Sounds like a good idea! I suggest we continue working on the other breaking changes for RTL (porting testing-library/dom-testing-library#1255 and then updating the dependency on DTL) in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, making render async and wrapping it with act() by default is also the solution I came up with in my current project (on React 17) to get stable and consistent first assertions in tests
@eps1lon, what are the chances this will actually land in React Testing Library do you think? I'll be recording videos tomorrow that I would love to not have to re-record. I can use smoke and mirrors to record these videos with async On my end, like I said I've always felt like this was inevitable we'd end up doing this anyway, so I'm in favor. |
b156f87
to
b5cf504
Compare
types/index.d.ts
Outdated
: typeof reactAct | ||
export function act<T>(scope: () => T): Promise<T> | ||
|
||
export * from '@testing-library/dom' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment here and to src/pure.js
explaining why export order is important.
iirc we had a discussion around this with Rollup people and this was actually specified behavior: first export wins not last like you would expect if it would work like object spreading i.e. Read next comment to understand which export is used.export * from ...; export { fireEvent }
is not equivalent to module.exports = { ...dtlExports, fireEvent }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found it: #790 (comment)
TL;DR: By spec, the local fireEvent
should be preferred. Babel specifically transpiles the export *
so that the local fireEvent
is in exports
not the one from @testing-library/dom
.
So this is a TS quirk we have to work around. I try to create a repro to confirm I didn't hallucinate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverting for now. Local checkout was pointing to the fireEvent from /dom but this may have been the language server having an outdated install. Repros do match spec behavior.
try { | ||
const result = await cb() | ||
// Drain microtask queue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if we added a test for this.
f676301
to
4fd9f05
Compare
4fd9f05
to
b038f70
Compare
They also need work in DTL since advanceTimersWrapper is now async.
This reverts commit 7315eb8.
b038f70
to
dd27bd9
Compare
This is a WIP. Changes are described as they are discovered.
Codemod: https://github.com/eps1lon/codemod-missing-await-act
fixes to missing act warnings
Sometimes you wanted to assert on a given commit of the component without caring about scheduled tasks. This always worked since these scheduled tasks were on the macrotask queue (e.g.
setTimeout
) because we automatically unmounted the component after the test ended.However, when these tasks where on the microtasks queue (e.g. a promise that just resolved), you would get a missing act warning between the test ending and our automatic cleanup running.
The only solution would've been to add an explicit unmount to each test.
Now that we require
await act(...)
these issues will no longer persist since we'll also flush the state updates on the microtask queue. If you want to continue asserting on the intermediate states, you can useglobalThis.IS_REACT_ACT_ENVIRONMENT = false
. Keep in mind though that while this flag is set, you have to manually wait for every state update to be applied e.g. by usingfindBy
orwaitFor
.Timer changes after module initialization
Drops support for changing timers after module initialization.
This affects tests that changed the timer implementation (i.e. fake vs real timers) after modules were imported. For example, in the following test, updates are no longer flushed.
Instead, you have to decide on timers before you import modules. In Jest, you can choose timers at the config level, or keep using
jest.useFaketimers()
but call it before anyimport
orrequire
.This only ever worked because we cheated a bit and flipped silently to an act environment during
waitFor
. However, since we now flush a bit more when usingIS_REACT_ACT_ENVIRONMENT
, we need to offer APIs to assert on intermediate states e.g. when your component initially displays a loading spinner that goes away in the next microtask. You can do this by simply settingglobalThis.IS_REACT_ACT_ENVIRONMENT = false
and continue to use existing APIs. React will fallback to standard (real or fake) timers and not the act queue.Full explainer will follow. Change will be accompanied by a codemod: https://github.com/eps1lon/codemod-missing-await-act