vadimdemedes / ink-testing-library

Utilities for testing Ink apps
MIT License
109 stars 17 forks source link

`act` equivalent or flush effects on render #3

Open mAAdhaTTah opened 5 years ago

mAAdhaTTah commented 5 years ago

Testing a component with hooks is a bit of an issue currently. I have this as an example component that renders & ends the application:

export const Cancelled = () => {
  const { exit } = useContext(AppContext);

  useEffect(() => {
    exit();
  }, [exit]);

  return <Box>Cancelled!</Box>;
};

And I'm testing it like this:

it('should render and exit', () => {
    const exit = sinon.spy();
    const { lastFrame, unmount } = render(
        <AppContext.Provider value={{ exit }}>
          <Cancelled />
        </AppContext.Provider>
    );

    expect(lastFrame()).to.matchSnapshot();
    expect(exit.calledOnce).to.equal(true);

    unmount();
});

The problem is the effect doesn't run by the time the render completes. I have to wrap the check around exit.calledOnce in a setTimeout. An act equivalent would allow use to write something like this:

it('should render and exit', () => {
    let lastFrame, unmount;
    const exit = sinon.spy();
    act(() => {
        ({ lastFrame, unmount } = render(
            <AppContext.Provider value={{ exit }}>
              <Cancelled />
            </AppContext.Provider>
        ));
    });

    expect(lastFrame()).to.matchSnapshot();
    expect(exit.calledOnce).to.equal(true);

    unmount();
});

and we'd know the effects would have been flushed by the time we check exit.calledOnce.

Alternatively, this version of render could flush effects immediately, rather than using the scheduler, which uses setTimeout.

vadimdemedes commented 5 years ago

Good suggestion! I've tried implementing act() already, but for some reason couldn't get it to work as expected. Any help is appreciated here!

mamachanko commented 5 years ago

Hi. I am having a similar issue. I have created a <Counter /> that increments with a delay when you press space. https://github.com/mamachanko/ink-playground/tree/master/counter-ts

When I move the subscription to stdin.on into useEffect (see here) it still works, but the tests no longer pass.

Am I right in understanding that my and this issue here are related? My intuition is to wrap the stdin.write with act(() => ...).

@vadimdemedes do you want to share what you have tried so far? Feels like I am out of my depth, but maybe others have valuable input.

mAAdhaTTah commented 5 years ago

I did a little bit of digging into the React codebase. act under the hood relies on ReactDOM.unstable_batchedUpdate. Tracing that back led to this:

https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberScheduler.js#L2486-L2497

I'm not familiar with writing custom reconcilers, but my first thought is this might be exposed for us already, as we're using react-reconciler already. That said, I'd echo @mamachanko and would like to hear from you @vadimdemedes as to what you attempted so far.

vadimdemedes commented 5 years ago

I tried replicating that act() ponyfill https://github.com/kentcdodds/react-testing-library/blob/master/src/act-compat.js, but had no luck. I'm still kind of confused by how it's implemented in React, will need to dive into it again.

mamachanko commented 5 years ago

I don't think I have made any progress here, but just want to share the context I collected. All of which is probably known to the advanced React practitioner. But here it goes anyway. Maybe it's helpful to others who want to look into this.

There's a really concise explanation for why this warning exists, what act() does and why you'd want that by @kentcdodds in one of his videos(link points to minute 16:54).

This (currently open) issue on React is about the act() warning and the conversation goes on to great detail about whats and ifs. @threepointone is working on a remedy PR and this PR seems to go out any day now.

What I haven't wrapped my head around yet is how (or whether at all) these things relate to ink. We are using a custom renderer and DOM and therefor reconciliation is custom too (is that correct?). But if React can give us the warning and figure out that there are possibly unflushed effects, then where are we leaving the React-paved road so that we can't just use act() for ink? This ties back into what @mAAdhaTTah said. act() is part of react-dom/test-utils. So since we use a custom DOM we need a custom act()? Not sure if I even make remotely sense here.

I forked and refactored the ink-testing-library's tests to use functional components and hooks to be able to easily reproduce the warning: https://github.com/mamachanko/ink-testing-library/blob/using-fc-and-hooks-in-test/test.js The problem is that the tests just fail and show no warning. I would have expected it when stdin.write('Hello World') happens. Maybe I missed something very obvious. Anyway, I've got this simple counter demo to consistenly reproduce the warning https://github.com/mamachanko/ink-playground/tree/master/counter-ts.

vadimdemedes commented 5 years ago

Thanks for such a detailed explanation, very helpful!

vadimdemedes commented 5 years ago

We are using a custom renderer and DOM and therefor reconciliation is custom too (is that correct?).

We do use a custom renderer and implement custom configuration for reconciliation, that's correct.

So since we use a custom DOM we need a custom act()?

Yeah, seems like we do.

threepointone commented 5 years ago

I'll try to have a look at this later this week, it should be possible for custom renderers. We just released an alpha that implements it for react-dom and react-test-renderer.

vadimdemedes commented 5 years ago

Thanks @threepointone, let us know about your progress! Totally understandable if you won't have time to look into it too, no worries.

mAAdhaTTah commented 4 years ago

Has there been any movement or explorations around this issue?

I'd be interested in picking this up for implementation in ITL, as I've run into a related issue in something I'm working on. I have a HOC that does something in useEffect. However, it doesn't appear that effects are flushed before the initial render completes. This means that if I call render and then attempt to interact with the result, the effects will not have been run yet, so I can't actually test those behaviors.

I think this is related to this issue because they're both related to batching & flushing pending effects. Like act, the initial render also needs to batch & flush any pending effects before returning.

threepointone commented 4 years ago

Sorry I never got around to this, but it’s still doable. Copy this file https://github.com/facebook/react/blob/master/packages/react-dom/src/test-utils/ReactTestUtilsAct.js, and replace the react-dom versions of isThisRendererActing, flushPassiveEffects and batchedUpdates with ink’s versions. Happy to assist on a PR if someone puts it up.

To be clear, this should be only used in tests, and not product code.

JaKXz commented 3 years ago

^has anyone had any luck with implementing this or a workaround?

benwainwright commented 1 year ago

Anyone got any solution for this?