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

expose TestUtils.act() for batching actions in tests #14744

Merged
merged 33 commits into from Feb 5, 2019

Conversation

@threepointone
Copy link
Contributor

@threepointone threepointone commented Feb 1, 2019

act() for testing react components


// React DOM
import { act } from 'react-dom/test-utils';

// React Native
import { act } from 'react-test-renderer';

React, like other libraries, doesn't guarantee synchronous ordering and execution of it's own work. eg - calling this.setState() inside a class doesn't actually set the component state immediately. In fact, it might not even update the state of the component in the same 'tick'. This isn't a problem when it comes to 'users'; they interact with React surfaces asynchronously, giving react and components plenty of time to 'resolve' to particular states.

However, tests are unique in that people write code, usually sequential, to interact with React components, and some assumptions they make won't hold true. Consider - with React's new useEffect hook, a component can run a side effect as soon as it 'starts up'. As a contrived example -

function App(props) {
  useEffect(() => {
    props.callback();
  });
  return null;
}

Let's say you write a test for it like so -

let called = false;
render(
  <App
    callback={() => {
      called = true;
    }}
  />,
  document.body
);
expect(called).toBe(true); // this fails, it's false instead!

The test would fail, which seems counterintuitive at first. But the docs explain it - "The function passed to useEffect will run after the render is committed to the screen." So while the effect has been queued into it's scheduler, it's up to React to decide when to run it. React only guarantees that it'll be run before the browser has reflected changes made to the dom to the user (ie - before the browser has 'painted' the screen)

You may be tempted to refactor this like so -

// don't do this!
function App(props) {
  useLayoutEffect(() => {
    props.callback();
  });
  return null;
}

This would "work" in that your test would pass, but that's because you've explicitly using a render blocking effect where it possibly wasn't required. This is bad for a number of reasons, but in this context, it's bad because we're changing product behavior just to fix a test.

What can we do better?

Well, React could expose a helper, let's call it act, that guarantees the sequential execution of it's update queue. Let's rewrite the test -

let called = false;
act(() => {
  // this 'scope' is safe to interact with the React component,
  // rendering and clicking as you please
  render(
    <App
      callback={() => {
        called = true;
      }}
    />,
    document.body
  );
});
// at this point, we can guarantee that effects have been executed,
// so we can make assertions
expect(called).toBe(true); // this passes now!
act(() => {
  // further interactions, like clicking buttons, scrolling, etc
});
// more assertions

Note - React still doesn't synchronously execute the effect (so you still can't put your expect statements inside act), but it does guarantee to execute all enqueued effects right after act is called.

This is a nice mental model for separation of concerns when testing components - "React, here's a batch of code I'd like you to run at one go", followed by more code to test what React has actually 'done'.

@threepointone
Copy link
Contributor Author

@threepointone threepointone commented Feb 1, 2019

@threepointone
Copy link
Contributor Author

@threepointone threepointone commented Feb 1, 2019

moving it into TestUtils
Edit - done.

@gaearon
Copy link
Member

@gaearon gaearon commented Feb 1, 2019

Possible follow up: warn if we detect setState on a Hook in jsdom environment?

@threepointone threepointone changed the title expose unstable_interact for batching actions in tests expose TestUtils.interact() for batching actions in tests Feb 1, 2019
@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Feb 1, 2019

If we're going to warn for setState on Hook outside of this thing, we should do it before release.

@threepointone
Copy link
Contributor Author

@threepointone threepointone commented Feb 1, 2019

Noted. I'll send a followup in a bit.

Copy link
Member

@sebmarkbage sebmarkbage left a comment

We shouldn't expose private APIs to do this in the production build since we'll have to live with those for longer. The goal is to get rid of all the other exports so it's zero private exports. We only get there if we stop going the other direction. Let's just use the hack in the test utils.

We should bikeshed the name a bit more. interact is the wrong word because not all of these are interactions, and even for things we've called interactions in the past we should rename (e.g. to "discrete events" rather than interactive events).

function batchedInteraction(callback: () => void) {
batchedUpdates(callback);
flushPassiveEffects();
}

This comment has been minimized.

@sebmarkbage

sebmarkbage Feb 1, 2019
Member

This function should move to ReactTestUtils and instead using the ReactDOM.render(null...) trick to flush the passive effects. That way we don't have to add any unnecessary invasive APIs to the production ReactDOM. We can add private APIs once we have the new bundle that is exclusively for testing.

This comment has been minimized.

@threepointone

threepointone Feb 1, 2019
Author Contributor

agreed, I felt icky doing this. will change,

@kentcdodds
Copy link
Contributor

@kentcdodds kentcdodds commented Feb 1, 2019

I think this looks good. We'll probably just re-export the interact function in react-testing-library.

warn if we detect setState on a Hook in jsdom environment?

I'm not sure I understand this.

@threepointone
Copy link
Contributor Author

@threepointone threepointone commented Feb 1, 2019

We should bikeshed the name a bit more. interact is the wrong word

strong agree. it felt wrong right there. some options - run, execute, batch, batchAndRun. I would have also liked do

@gaearon
Copy link
Member

@gaearon gaearon commented Feb 1, 2019

I'm not sure I understand this.

We want to add a warning when you setState on a Hook-using component in jsdom outside of this "interact" scope. This is because you're usually testing the wrong behavior that doesn't occur in practice due to batching.

In fact this is already a problem in classes. setState() from test won't work like a real setState() in a class click handler. But it's too late to fix in classes since everybody does that. With Hooks we have a chance to explain this is bad, and point to the recommended solution.

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Feb 1, 2019

We shouldn't warn in all jsdom environments. Only test ones, such as jest. Sometimes jsdom can be used in some other esoteric use cases - e.g. to server render webcomponents.

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Feb 1, 2019

@threepointone Instead of detecting if we're inside "interact()", we can detect if we're batching updates or in concurrent mode. I.e. if we're in sync mode. It's unfortunate because we won't warn if you use batchedUpdates() which won't properly flush the effects but at least we catch the common case. That way we don't need to expose and new APIs from the ReactDOM bundle.

@kentcdodds
Copy link
Contributor

@kentcdodds kentcdodds commented Feb 1, 2019

This makes sense. So correct me if I'm wrong but this basically means that if you interact with a component in any way that calls a state updater, you'll get a warning if it wasn't done within an interact callback (or whatever that ends up being called). That sounds good to me 👍 Makes the rules easier to follow. I'd love to play with this when it's ready!

@gaearon
Copy link
Member

@gaearon gaearon commented Feb 1, 2019

We shouldn't warn in all jsdom environments. Only test ones, such as jest

This is tricky because we don't have a way to detect. We can detect Jest by global.it or global.expect (or both). But not Ava which uses non-global helpers.

We could check NODE_ENV. But not everybody sets it. Also, we can't easily do this from inside our bundles because our build step would replace it. So it would need to be threaded through somehow.

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Feb 1, 2019

I suppose any jsdom server renderer should be using batchedUpdates anyway.

@@ -380,6 +380,11 @@ const ReactTestUtils = {

Simulate: null,
SimulateNative: {},

interact(callback: () => void) {

This comment has been minimized.

@sebmarkbage

sebmarkbage Feb 1, 2019
Member

Drop the inter. Just call it act. It's cleaner.

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Feb 1, 2019

There is precedence in the arrange, act, assert style testing to call the phase where you are actually performing things "act" which is what you're meant to be doing in this callback. Followed by expect() calls later.

It's also short so it's not too bothersome to write all the time in all your tests.

I think it's a feature that it is non-descriptive about what actually this models. It's kind of a frame boundary but it's not always because they can also flush early. Event sequences move around a lot in various heuristics/polyfills/spec changes. So there isn't a clear semantic other than there is a bunch of work.


interact(callback: () => void) {
ReactDOM.unstable_batchedUpdates(callback);
ReactDOM.render(null, document.createElement('div'));

This comment has been minimized.

@sebmarkbage

sebmarkbage Feb 1, 2019
Member

Can we reuse a single node? Maybe put an inline React element instead so we don't bail out.

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Feb 1, 2019

I believe that in the nested case, these will flush at the outer act(...) boundary which makes sense. That way I can put these on the top level always, but I can also put them inside test utilities like "dispatch" helpers.

@threepointone threepointone changed the title expose TestUtils.interact() for batching actions in tests expose TestUtils.act() for batching actions in tests Feb 1, 2019
@gaearon
Copy link
Member

@gaearon gaearon commented Feb 1, 2019

Canonical jsdom detection jsdom/jsdom#1537 (comment)

@threepointone
Copy link
Contributor Author

@threepointone threepointone commented Feb 2, 2019

Question - Assuming the warning exists, how would I get this test to pass?

it('lets a ticker update', () => {
  function App(){
    let [toggle, setToggle] = useState(0)
    useEffect(() => {
      let timeout = setTimeout(() => {
        setToggle(1)
      }, 200);
      return () => clearTimeout(timeout)
    })
    return toggle
  }
  const el = document.createElement('div')
  act(() => {
    ReactDOM.render(<App />, el);
  })
  jest.advanceTimersByTime(250); // this warns!!!        
  expect(el.innerHTML).toBe("1")
})

for context - I wrote ensureBatchingAndScheduleWork in ReactFiberScheduler, that dispatchAction in ReactFiberHooks would call. Running this across our tests, I found a number of failures (because we use the pattern we want to prevent - getting a pointer to a hooks setState and calling it). filtering on only jsdom occurrences, that number got smaller, but led me to writing the above test. current work here - threepointone@ac7416d

@threepointone
Copy link
Contributor Author

@threepointone threepointone commented Feb 2, 2019

one workaround is to isolate the render call and the timer advance, so this passes the test -

act(() => {
  ReactDOM.render(<App />, container);
});
act(() => {
  jest.advanceTimersByTime(250); // doesn't warn
})

expect(container.innerHTML).toBe('1');

bit annoying though.

another alternative, also annoying -

act(() => {
  act(()=> {
    ReactDOM.render(<App />, container);  
  })      
  jest.advanceTimersByTime(250); // this warns!!!      
});

(putting them both in the same act call doesn't work, since the effect wouldn't have fired yet)

@kentcdodds
Copy link
Contributor

@kentcdodds kentcdodds commented Feb 2, 2019

Good point @threepointone. I can see how this complicates the testing story quite a bit. Even experienced React engineers will struggle with writing tests for effects like this 🤔

This was referenced Sep 20, 2019
NMinhNguyen referenced this pull request in enzymejs/react-shallow-renderer Jan 29, 2020
* expose unstable_interact for batching actions in tests

* move to TestUtils

* move it all into testutils

* s/interact/act

* warn when calling hook-like setState outside batching mode

* pass tests

* merge-temp

* move jsdom test to callsite

* mark failing tests

* pass most tests (except one)

* augh IE

* pass fuzz tests

* better warning, expose the right batchedUpdates on TestRenderer for www

* move it into hooks, test for dom

* expose a flag on the host config, move stuff around

* rename, pass flow

* pass flow... again

* tweak .act() type

* enable for all jest environments/renderers; pass (most) tests.

* pass all tests

* expose just the warning from the scheduler

* don't return values

* a bunch of changes.

can't return values from .act
don't try to await .act calls
pass tests

* fixes and nits

* "fire events that udpates state"

* nit

* 🙄

* my bad

* hi andrew

(prettier fix)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet