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

[5.5.0] Getting warning about "act" anyway #281

Closed
FredyC opened this issue Feb 6, 2019 · 128 comments · Fixed by #431

Comments

@FredyC
Copy link
Contributor

commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

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.

@FredyC

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

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

This comment has been minimized.

Copy link

commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

A codesandbox would be very helpful.

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

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

@panjiesw

This comment has been minimized.

Copy link

commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

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?

@FredyC

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

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

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

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.

@FredyC

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

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

This comment has been minimized.

Copy link

commented Feb 6, 2019

@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 comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

^ 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.

@FredyC

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

Ah, now that is starting to make sense, but it's ugly nonetheless considering that this was working just fine pre-16.8. I was kinda hoping that with the official release there will be an improvement to testing experience, not that it will be even worse 😢

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

Hmmm... I'm still not certain how to solve for cases like this:

Edit react-testing-library-examples

// this is similar to __local_tests__/async-with-mock.js
// except this one uses a form of dependency injection because
// jest.mock is not available in codesandbox
import React from 'react'
import axios from 'axios'
import {render, fireEvent, waitForElement, cleanup} from 'react-testing-library'
import 'jest-dom/extend-expect'

afterEach(cleanup)

function useFetch({url, axios = axios}) {
  const [state, setState] = React.useState({})
  const fetch = async () => {
    const response = await axios.get(url)
    setState(s => ({...s, data: response.data}))
  }
  const ref = React.useRef()
  React.useEffect(
    () => {
      if (ref.current) {
        ref.current = true
      } else {
        fetch()
      }
    },
    [url],
  )
  return {state, fetch}
}

function Fetch({url, axios}) {
  const {state, fetch} = useFetch({url, axios})
  const {data} = state
  return (
    <div>
      <button onClick={fetch}>Fetch</button>
      {data ? <span data-testid="greeting">{data.greeting}</span> : null}
    </div>
  )
}

test('Fetch makes an API call and displays the greeting', async () => {
  const fakeAxios = {
    get: jest.fn(() => Promise.resolve({data: {greeting: 'hello there'}})),
  }
  const url = 'https://example.com/get-hello-there'
  const {getByText, getByTestId} = render(<Fetch url={url} axios={fakeAxios} />)
  fireEvent.click(getByText(/fetch/i))

  const greetingNode = await waitForElement(() => getByTestId('greeting'))

  expect(fakeAxios.get).toHaveBeenCalledTimes(2) // React runs the render twice in dev mode
  expect(fakeAxios.get).toHaveBeenCalledWith(url)
  expect(greetingNode).toHaveTextContent('hello there')
})
@panjiesw

This comment has been minimized.

Copy link

commented Feb 6, 2019

I get the intentions though, by avoiding the warning we can be sure that the state update is intended. Same with mobx under strict mode where observable mutation must be used within action, runInAction and its family.

But then the strict mode is configurable in mobx. So, maybe make act requirement somehow configurable? I don't know if it'll make things easier or more complicated

@FredyC

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

And another use case where it will be even more annoying.

let oldTodo: any // nooooo, I am too sexy for this :(
act(() => {
    oldTodo = store.todos.pop()
})
expect(getDNode(oldTodo, "title").observers.size).toBe(0)

And in the TypeScript world this means falling back to any escape hatch because it will complain seriously about such use.

@panjiesw

I get the intentions though, by avoiding the warning we can be sure that the state update is intended

That might be true, but those warnings don't really give a useful hint where in the code is the problem happening. It would need to be improved for sure.

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

That might be true, but those warnings don't really give a useful hint where in the code is the problem happening.

are you not seeing a stack trace under your warning?

just got home, working on your fetch example @kentcdodds

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

I was kinda hoping that with the official release there will be an improvement to testing experience, not that it will be even worse

Consider that this will make your tests more reliable. This is apparent most when you fire multiple events in a row, effects cascade (or even just get triggered). I wouldn't be surprised if fixing all the warnings leads to discovering actual bugs in your code. That was my experience when doing the same for fb.

@FredyC

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

@threepointone

are you not seeing a stack trace under your warning?

Not really, you can see the output in code sandboxes presented here or in my linked Travis build. Jest is only showing the line where console.error got called: node_modules/react-dom/cjs/react-dom.development.js:506. Should I open react issue about improving so it's actually helpful? :)

A good thing is I've managed to fix mobx-react-lite tests and it working just fine. I guess in time it will become a habit.

@alexknipfer

This comment has been minimized.

Copy link

commented Feb 6, 2019

I'm still seeing the act warning with some of my tests as well. Unfortunately, I cannot share the source, however, I can give as much context as possible. I have a useEffect hook that makes an axios request and stores the result in state (using setState), which seems to be causing the issue.

Here is what one of my tests look like for a context provider:

test('Should select first bot id and pass to provider', async () => {
  axios.get = jest.fn().mockResolvedValueOnce({
    data: {
      data: myData
    }
  })

  const { getByText } = render(
    <MyProvider>
      <MyContext.Consumer>
        {value => <span>Received: {value.myValue}</span>}
      </MyContext.Consumer>
    </MyProvider>
  )

  const receivedElement = await waitForElement(
    () => getByText(/^Received:/).textContent
  )

  expect(receivedElement).toBe(`Received:  my test text`)
})

The tests succeeds, however I'm seeing the console warn, any suggestions / ideas as to why this is happening? Please let me know if I can provide any further clarification.

@dclaude

This comment has been minimized.

Copy link

commented Feb 6, 2019

Edit act
same problem as the one described in the last post of Kent

test("renders data", async () => {
  const spy = jest
    .spyOn(Fetch, "doFetch")
    .mockReturnValue(Promise.resolve(createResponse(items)));
  const { getAllByTestId } = render(<Items />);
  const elements = await waitForElement(() => getAllByTestId(dataTestIdItem));
  expect(elements.length).toBe(items.length);
  spy.mockRestore();
});
@marcin-piela

This comment has been minimized.

Copy link

commented Feb 7, 2019

It should working:

  jest.useFakeTimers();

  const spy = jest
    .spyOn(Fetch, "doFetch")
    .mockReturnValue(Promise.resolve(createResponse(items)));
  const { getAllByTestId } = render(<Items />);

  act(() => {
    jest.runAllTimers();
  })

  const elements = await waitForElement(() => getAllByTestId(dataTestIdItem));
  expect(elements.length).toBe(items.length);
  spy.mockRestore();
});

I've got a problem with fetch and catch errors.

@ghengeveld

This comment has been minimized.

Copy link

commented Feb 7, 2019

So I'm currently updating the tests for useAsync to work with 16.8, hitting this exact problem.

I tried using {act} from react-dom/test-utils, as well as the useFakeTimers trick mentioned above. So far I wasn't successful. Any pointers would be great.

test("returns render props", async () => {
  jest.useFakeTimers()
  const promiseFn = () => new Promise(resolve => setTimeout(resolve, 0, "done"))
  const component = <Async promiseFn={promiseFn}>{({ data }) => data || null}</Async>
  const { getByText } = render(component)
  act(() => {
    jest.runAllTimers()
  })
  await waitForElement(() => getByText("done"))
})

Wrapping stuff with act is really annoying, as it doesn't work well with async/await.

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

just a heads up, I've been wanting to answer this in detail, but just swamped with a bunch of other things with the team. we're aware of the problems, promise. I'll ping here when I have something to share.

@ghengeveld

This comment has been minimized.

Copy link

commented Feb 7, 2019

Good to know you're aware. I trust you'll come up with a good solution. Keep up the good work!

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

@bpb54321 thanks for the repro! I sent a PR bpb54321/dialog-app#1 that fixes your situation.

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

tldr - you should use async act around asynchronous operations, and careful that your yarn lockfile hasn't locked you into an older version of react.

@bpb54321

This comment has been minimized.

Copy link

commented Jul 31, 2019

@threepointone Thank you so much for your help!

@JSFernandes

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

Since it looks like it will take a while until React DOM gets updated, what do you think of adding this to the README? It seems too relevant to be tucked away in an issue.

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Feel free to open a PR to add it somewhere, but I have it on good authority that we're a week or two away from the 16.9.0 release. facebook/react#16254

@alexcheuk

This comment has been minimized.

Copy link

commented Aug 6, 2019

Can confirm that this issue is resolved using "react-dom": "16.9.0-rc.0" and "react": "16.9.0-rc.0". My example below no longer cause warnings about act() or updating state on unmounted component

My components:

const MyComponent = props => {
  const [num, setNum] = React.useState(0)

  React.useEffect(() => {
    SomeAPI.fetch().then(() => {
      setNum(10)
    })
  }, [])

  return <div>{num}</div>
}

RTL Test:

it('should do something and expect something', async () => {
    mockAdapter
      .onGet('/someapi/fetch/')
      .replyOnce(() => [200, {}])

    const { debug, container } = render(<MyComponent />)

    await flushAllPromises()

    expect(true).toBe(true)
  })
@fcoronelmakingsense

This comment has been minimized.

Copy link

commented Aug 8, 2019

Can confirm that this issue is resolved using "react-dom": "16.9.0-rc.0" and "react": "16.9.0-rc.0". My example below no longer cause warnings about act() or updating state on unmounted component

My components:

const MyComponent = props => {
  const [num, setNum] = React.useState(0)

  React.useEffect(() => {
    SomeAPI.fetch().then(() => {
      setNum(10)
    })
  }, [])

  return <div>{num}</div>
}

RTL Test:

it('should do something and expect something', async () => {
    mockAdapter
      .onGet('/someapi/fetch/')
      .replyOnce(() => [200, {}])

    const { debug, container } = render(<MyComponent />)

    await flushAllPromises()

    expect(true).toBe(true)
  })

Hi! i have a question. How to install the version "react-dom": "16.9.0-rc.0"

kentcdodds added a commit that referenced this issue Aug 8, 2019
kentcdodds added a commit that referenced this issue Aug 8, 2019
kentcdodds added a commit that referenced this issue Aug 8, 2019
@kentcdodds

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

🎉 This issue has been resolved in version 8.0.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kentcdodds kentcdodds added the released label Aug 8, 2019

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

react@16.9.0 has been released 👏 @threepointone!!

So as long as you ensure you're using the latest version of react and @testing-library/react and you're using React Testing Library's utilities for firing events and async things, then you shouldn't have to worry about calling act except in a few rare cases.

@Yagogc

This comment has been minimized.

Copy link

commented Aug 12, 2019

Hi @kentcdodds , I'm still having the issue for some reason:
Maybe, and probably, it's my fault, but here is my code:
Note: Removed some bits for more clarity.

import { useQuery } from '@apollo/react-hooks'

const Calendar = () => {
  const { loading, error, data } = useQuery(GET_CALENDAR_QUERY)

  if (loading) {
    return (...)
  }
  if (error) {
    return (...)
  }
  return (...)
}
import { MockedProvider } from '@apollo/react-testing'
import wait from 'waait'

  it('renders a loading state initially', () => {
    const { getByTestId } = renderProviders(
      <MockedProvider mocks={[]} addTypename={false}>
        <Calendar />
      </MockedProvider>
    )
    expect(getByTestId('loading')).toBeDefined()
  })

  it('renders the calendar', async () => {
    const { getByText } = renderProviders(
      <MockedProvider mocks={mocks} addTypename={false}>
        <Calendar />
      </MockedProvider>
    )
    await wait(0)
    expect(getByText('P1 W1')).toBeDefined()
  })

Even all the test are green, I'm still getting the console error:

    Warning: An update to Calendar inside a test was not wrapped in act(...).
    
    When testing, code that causes React state updates should be wrapped into act(...):
    
    act(() => {
      /* fire events that update state */
    });

I updated all the dependencies: React 16.9, RTL 9.1.0, etc. and still have the error.

Any clue on why is happening? Is related to Apollo? Or just my fault 😄 ?

Thanks!

@ChristianBoehlke

This comment has been minimized.

Copy link

commented Aug 12, 2019

@Yagogc, the code that causes the state update is the await wait(0) which is not wrapped in act(). You can wrap it yourself (await act(() => wait(0))) or you just use the wait() method that comes with @testing-library/react which already does the work for you.

@FredyC

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@Yagogc Generally I would recommend you the following instead of wait(0).

// wait imported from RTL as @ChristianBoehlke mentioned
await wait(() => {
  expect(getByText('P1 W1')).toBeDefined()
})

That way RTL will keep trying until the expectation passes (or timeouts). It's more natural than waiting for some explicit timers imo.

@Yagogc

This comment has been minimized.

Copy link

commented Aug 12, 2019

@Yagogc, the code that causes the state update is the await wait(0) which is not wrapped in act(). You can wrap it yourself (await act(() => wait(0))) or you just use the wait() method that comes with @testing-library/react which already does the work for you.

Hey @ChristianBoehlke thank you so much, that works, at least for the second test. Now the first test is still passing but throwing a different error:

import { wait } from '@testing-library/react'

  it('renders a loading state initially', () => {
    const { getByTestId } = renderProviders(
      <MockedProvider mocks={[]} addTypename={false}>
        <Calendar />
      </MockedProvider>
    )
    wait()
    expect(getByTestId('loading')).toBeDefined()
  })
Warning: You seem to have overlapping act() calls, this is not supported. Be sure to await previous act() calls before making a new one.

If I make it async/await the error goes away but the test fails. And if I don't use the wait it will error about not using act...

Any ideas?
Thanks again.

@bopfer

This comment has been minimized.

Copy link

commented Aug 12, 2019

It's probably best to use the findBy* queries. Then the waiting is built in.

@FredyC

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@Yagogc And why don't you do what it's telling you to do? :) You need to await that wait 😆 JavaScript does not have a concept of some magical "pause" which you can just call.

Or use findBy if you like it more, indeed.

@alexknipfer

This comment has been minimized.

Copy link

commented Aug 12, 2019

Wouldn't something like this do the trick?

await wait(() => {
    expect(getByTestId('loading')).toBeDefined()
})
@Yagogc

This comment has been minimized.

Copy link

commented Aug 12, 2019

Sorry @FredyC , I didn't saw your comment earlier. I have updated the second test with the change you mentioned, but for the first test I had to use the findBy* query that @bopfer mentioned. Thanks both by the way.

Right it is working like this:

  it('renders a loading state initially', async () => {
    const { findByTestId } = renderProviders(
      <MockedProvider mocks={[]} addTypename={false}>
        <Calendar />
      </MockedProvider>
    )

    expect(findByTestId('loading')).toBeDefined()
  })

  it('renders the calendar', async () => {
    const { getByText } = renderProviders(
      <MockedProvider mocks={mocks} addTypename={false}>
        <Calendar />
      </MockedProvider>
    )

    await wait(() => {
      expect(getByText('P1 W1')).toBeDefined()
    })
  })

Now I have a question: why I can in the finBy* query on the first test but not on the second test?

If instead I just do: expect(findByTestId('P1 W1')).toBeDefined() on the second test I get again the : Warning: You seem to have overlapping act() calls, this is not supported. Be sure to await previous act() calls before making a new one.

@FredyC

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Your first test is false positive because findBy returns Promise. Without awaiting you are just doing ...

expect(promise).toBeDefined()

If you await the findBy in the second test, it will work as well.

@bopfer

This comment has been minimized.

Copy link

commented Aug 12, 2019

This:

await wait(() => {
  expect(getByText('P1 W1')).toBeDefined()
})

Can also be completed replaced with this:

await findByText('P1 W1')

The getBy* and findBy* queries will all throw on failure and the test will fail. The expect and toBeDefined are unnecessary, but some people like them for readability. Personally, I like the brevity of not using them.

@FredyC

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@bopfer I tend to do that sometimes as well, but it's somewhat weird to see a test without an actual expect. It makes the impression of an incomplete test.

Anyway, I think someone shall lock this long term issue to make people open specific new issues.

Note for passers-by:

As long as you are using RTL helpers and functions, you most likely won't need to worry about act

@testing-library testing-library locked as resolved and limited conversation to collaborators Aug 12, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
You can’t perform that action at this time.