[5.5.0] Getting warning about "act" anyway

[danielkcz]

Thanks for the fast work Kent, but seems something is still off. I've updated to 5.5.0 and tests are still generating same act warnings. Am I missing something here? Do we need to actually use act inside the tests?

https://travis-ci.org/mobxjs/mobx-react-lite/builds/489613981

It's curious that I've actually removed a bunch of flushEffects calls which were necessary before and tests are passing just fine. Clearly act helped with that. And yet those warnings shouldn't be there. 🤔

[threepointone]

I can't see your tests, but I'm guessing you're triggering some changes 'manually' (ie - without fireEvent). those calls should be wrapped with act() too.

[danielkcz]

Funny enough there is not a single fireEvent. It's a MobX library which is about reactions :) Ultimately it all boils down to this piece of code which is just useState incrementing integer for each update.

I guess I will wait when you can have a look at those test files because I am not sure how to handle this reality. For a reference, a test file like this is causing the warning.

https://github.com/mobxjs/mobx-react-lite/blob/master/test/ObserverComponent.test.tsx

[panjiesw]

Just got bitten by this, upgraded to 5.5.0, remove flushEffect and the warning is still there. I use fireEvent from react-testing-library. Double check the js file, and it's indeed using wrapped fireEvent of dom-testing-library inside actCompat callback.

EDIT to include the test:

import { render, fireEvent } from 'react-testing-library';

// Other imports and tests

it('sets and reacts to data', () => {
  const w = render(
    <FieldWrapper>
      <FieldInput id="foo" />
    </FieldWrapper>,
  );

  fireEvent.change(w.getByTestId('foo'), {
    target: { value: 'bar' },
  });

  expect(w.getByTestId('foo')).toHaveAttribute('value', 'bar');
});

[kentcdodds]

A codesandbox would be very helpful.

[threepointone]

yeah I can't help more without a working example, sorry, basically shooting blind.

[panjiesw]

Yeah, that change event basically also trigger bunch of effects (async processing) behind the scene, which when resolved updates other state.

I tried to simulate it in this sandbox https://codesandbox.io/s/rjnwr62v5o

The component is in src/input.js and the test is in src/__tests__/input.js. As you can see there, the test passed, but the warning also appeared in the console.

[kentcdodds]

Gotcha, so what @threepointone said is true:

I'm guessing you're triggering some changes 'manually'

So it's working as designed.

I'm honestly not sure how to account for situations like this though. What do you think Sunil?

[danielkcz]

I am kinda unclear what "manual update" means. Is invoking a setter from the useState considered a manual update? Isn't that going to be in nearly every component? That would make writing tests somewhat more annoying.

[threepointone]

Heading home so I’ll get to this in a bit, but tldr - jest.useFakeTimers/jest.runAllTimers should help. Back in an hour.

[kentcdodds]

Regarding the call stack.

Any time you call a state updater function like this.setState, or dispatch or setState, if that's not happening within the call stack of a batch update in react then you'll get this warning because it basically means you're test is doing something that wouldn't happen in the browser (this is where things get fuzzy for me, I'm not sure why that is).

Anyway, the act utility puts your callback within a batch update. But because you're calling a state updater in an async callback (via Promise.resolve().then), it's running outside a batch update and therefore you get the warning.

We need to find a good way to either work around this and/or document this.

Note: This has nothing to do with react-testing-library or even hooks and everything to do with new warnings that were introduced in React@16.8.0 to make your tests better resemble reality. As I said, I'm not sure what the difference is. Maybe Sunil can help explain it.

[danielkcz]

I've prepared another code sandbox reproducing the issue.

The warning comes from the first test. The second test is failing and the last one is only one working properly, but super ugly and I really hope there will be a better way.

[panjiesw]

@FredyC I got it working for the first test without warning:

it("works, but shows warning", () => {
  const state = mobx.observable({ value: 5 });
  const { container } = render(<TestComponent state={state} />);
  expect(container.textContent).toBe("5");
  // This updates state, so should be wrapped in act
  act(() => {
    state.value = 10;
  })
  expect(container.textContent).toBe("10");
});

AFAIK, this is consistent by the react-dom/test-utils docs, it's just that the example in the docs is firing event, but basically the same as updating state.

[threepointone]

^ this is correct. I’ll edit the docs to be clearer to say that you need to wrap anything that causes updates to be wrapped.