[BUG] Does not render items in tests from 2.17.1

mihkeleidast commented 2 years ago

Describe the bug

After updating to 2.17.x, react-virtuoso does not render list items (or table rows) in tests anymore.

Reproduction Not sure how to do tests in Codesandbox, so created a simple repo in GH: https://github.com/mihkeleidast/react-virtuoso-test-rendering

To Reproduce Steps to reproduce the behavior:

As you can see from the initial commit, when I have installed react-virtuoso@2.16, the items are rendered in the test snapshot: https://github.com/mihkeleidast/react-virtuoso-test-rendering/commit/d5005a364f3cb102938c5db7c212aba168c72424#diff-896843a0f2c025052a543ecd29db770dbbfdfa0da0ec787d15d5316f777a6c2f
After updating to latest react-virutoso, the items are not rendering anymore in the test snapshot, making the test fail as well: https://github.com/mihkeleidast/react-virtuoso-test-rendering/commit/18e94306c29f9f43d585ad5818edef094c7e6a10#diff-896843a0f2c025052a543ecd29db770dbbfdfa0da0ec787d15d5316f777a6c2f

Expected behavior

I'd expect the tests to work the same after updating react-virtuoso.

Desktop (please complete the following information):

OS: Ubuntu in WSL2.
Browser: N/A

Additional context There is no issue in the browser, as far as I can see. Only tests seem to be affected.

petyosi commented 2 years ago

I'm almost certain that this is the reason: https://github.com/petyosi/react-virtuoso/commit/a846a39af6ac57d55dee087c862b4aa3d069e887

But why does that pass then? https://github.com/petyosi/react-virtuoso/blob/master/test/ssr.test.tsx. Can you help me out a bit with the investigation?

mihkeleidast commented 2 years ago

Hmm, it does still work on 2.16.6 and 2.17.0, breaks on 2.17.1 - the only change in the changelog is https://github.com/petyosi/react-virtuoso/commit/fe55e3f3f5031a52bc624c6cab7afb9da22a9d8e, so I'd suspect that one more?

As for the SSR test, it does use ReactDOMServer.renderToString directly - my tests are run through testing-library and jest, which means a jsdom environment.

petyosi commented 2 years ago

Good catch. This is most likely a problem, although it beats me why.

A bit of a deviation: my take on the matter is that react-virtuoso is not easy or reasonable to be tested in jsdom environment. The lib relies on input element dimensions for its rendering, and as far as I know, this is not available. The best and most reliable way to test the component is to go through a real browser, with a tool like playwright. Of course, there are fast unit tests for the logic.

This being said, I am far from picking the battle of telling people how to test their apps. I also know that what you're doing is fairly popular and conventional. I am happy to acknowledge and leave this issue open, but I don't know if or when I will get into researching this deeper. The jsdom env is a weird world.

mihkeleidast commented 2 years ago

I looked at what data List renders with, it seems like before it rendered once and listState was:

    {
      items: [
        { index: 0, size: 0, offset: 0, data: [Object], originalIndex: 0 },
        { index: 1, size: 0, offset: 0, data: [Object], originalIndex: 1 },
        { index: 2, size: 0, offset: 0, data: [Object], originalIndex: 2 }
      ],
      topItems: [],
      topListHeight: 0,
      offsetTop: 0,
      offsetBottom: 0,
      top: 0,
      bottom: 0,
      totalCount: 3,
      firstItemIndex: 0
    }

After the change, List renders twice, the first render is with the same listState, the second one is with empty items:

    {
      items: [],
      topItems: [],
      offsetTop: 0,
      offsetBottom: 0,
      top: 0,
      bottom: 0,
      topListHeight: 0,
      totalCount: 3,
      firstItemIndex: 0
    }

So, I'm still struggling with the state system a bit, but it seems like changing the totalCount triggers an update somewhere else, I'll try to keep looking.

petyosi commented 2 years ago

Thanks for taking a look. My concern is that it was accidentally working before.

mihkeleidast commented 2 years ago

A bit of a deviation: my take on the matter is that react-virtuoso is not easy or reasonable to be tested in jsdom environment. The lib relies on input element dimensions for its rendering, and as far as I know, this is not available. The best and most reliable way to test the component is to go through a real browser, with a tool like playwright. Of course, there are fast unit tests for the logic.

This being said, I am far from picking the battle of telling people how to test their apps. I also know that what you're doing is fairly popular and conventional. I am happy to acknowledge and leave this issue open, but I don't know if or when I will get into researching this deeper. The jsdom env is a weird world.

Controversial topic for sure :) In our case, our goal with the snapshot tests is twofold:

validate the HTML the components render, i.e. that virtuoso wouldn't inject some unexpected divs in a list, etc.
that it actually does render items - if it suddenly stops rendering items in tests, that's a bit of red flag, as it might be broken in the browser as well. Essentially catch if updating virtuoso breaks anything. We don't test stuff more than that, at some point, we do need to trust that the 3rd party lib does it's job .

mihkeleidast commented 2 years ago

But as it pertains to this issue and the topic, to make our own wrapper component a bit more testable, we implemented a context provider that allows overriding the initialItemCount from higher in the component tree. We currently pass the context value directly to Virtuoso, if it is defined. This was implemented logically similarly how react-responsive has a ResponsiveContext to mock the window size in tests, in order to test different scenarios. That library is similarly "browser-only", i.e. it does not make much sense to test the other behavior much in jsdom.

What I'm thinking is, would that kind of override context for testing purposes make sense for react-virtuoso core? We could then lose our own custom implementation, and supporting this in core would probably work around this issue as well.

mihkeleidast commented 2 years ago

Some more findings:

This statefulStreamFromEmitter logic runs twice in both cases, before and after the change in question
I do not understand why the emitter in question runs twice - is there any good way to debug what triggers the run?

Edit: Oops, had to delete most of the points, ran it without the change in question, not with, so of course it worked. Would still like to understand why the emitter logic runs twice, though...

petyosi commented 2 years ago

I am usually throwing console log and console trace in the map/scans to see where the change originates from. I think that the changed I introduced altered the distinct until changed filter in that exact file (previously, it was working because of the object identity in EMPTY_LIST_STATE remaining the same), so now the produced list state overrides the one constructed by the initialItemCount.

I really like the idea you outline about controlling the DOM state through some sort of a test oriented utility (context for example). Will give this some more thought.

mihkeleidast commented 2 years ago

Yeah, I did wonder if it compared the initial state object identity. Seems a bit weird, shouldn't it compare it to the current value of listState in that scenario?

petyosi commented 2 years ago

It does, but in the previous state, the initial state and the first state were the same object identity.

reinertisa commented 2 years ago

We also have the same problem. Thank you for your help. @mihkeleidast @petyosi

mihkeleidast commented 2 years ago

@petyosi Have you had a chance to think about the test-oriented context utility? Just thinking how we should proceed with this. We are blocked on this in our library for adding React 18 support (as https://github.com/petyosi/react-virtuoso/pull/733 and possibly some other fixes are only in the latest version). We don't like accepting this issue as it is, because we do have some interactive elements used in the virtualized lists and tables that are covered by functional tests, and this not rendering in tests would mean less coverage for us.

I would be willing to help with implementing this, although not sure how exactly it would work in here (I don't think there's any similar context existing), so some guidance would be appreciated.

petyosi commented 2 years ago

@mihkeleidast unfortunately, I have not had the time to think this problem through, day work is quite busy. I am happy to consider a design suggestion on the context-based approach, to be honest.

mihkeleidast commented 2 years ago

@petyosi I threw a very initial experiment in https://github.com/petyosi/react-virtuoso/pull/744, maybe you have some thoughts - let's continue under that PR perhaps?

chloe-samdesk commented 2 years ago

I'm running into the same issue.

v2.17.1 breaks some of our jest tests.

The output of the tests failing is now showing that there's no children being rendered for virtuoso.

we're mocking virtuoso thusly in jest:


    const { Virtuoso } = jest.requireActual('react-virtuoso');

    const mockVirtuoso = (WrappedVirtuoso: ElementType) =>
        class extends mockComponent<{ totalCount: number }, unknown> {
            render() {
                return <WrappedVirtuoso initialItemCount={this.props?.totalCount} {...this.props} />;
            }
        };

    return { Virtuoso: mockVirtuoso(Virtuoso) };
});

petyosi commented 2 years ago

Solved with the changes introduced in #744 . Warning: read 744 as it needs work on your side.

reinertisa commented 2 years ago

@petyosi @mihkeleidast Thank you for the fix. It works for me.

petyosi / react-virtuoso

[BUG] Does not render items in tests from 2.17.1 #737