[Table] Snapshot test issue with TablePagination

[is1ash]

Recent changes to compnent TablePagination break test.

https://github.com/mui-org/material-ui/blob/3eb02f5498857bd01bb7924eed9b946f33e39052/packages/material-ui/src/TablePagination/TablePagination.js#L114-L116

The generated id is always new and snapshot doesn't/cann't match.

<p 
  class="MuiTypography-root MuiTablePagination-caption MuiTypography-body2 MuiTypography-colorInherit"
-  id="mui-7460"
+  id="mui-62542"
>

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Environment 🌎

@material-ui/core: 4.10.1
jest: 24.9.0

[eps1lon]

Auto generated ids have no guarantee to be stable. This is another reason why we don't recommend snapshot testing.

[oliviertassinari]

Up until now, we have been providing ways to get deterministic snapshot tests, requiring extra work.

As far as I know, this occurrence of the problem is unique. It's the first time we don't provide any workaround. It sounds like the position of Material-UI is shifting from we discourage snapshot to we prevent it. Should we document it? Should we provide an escape hatch? It will be interesting to see if useOpaqueIdentifier is snapshot friendly when released as stable.

[eps1lon]

It's the first time we don't provide any workaround.

<TablePagination SelectProps={{ id: 'stable-select-id', labelId: 'stable-select-label-id' }} />

[oliviertassinari]

I have updated my comment on #17070 (comment) to include this problem. It will help when revamping https://material-ui.com/guides/testing/.

@eps1lon Are you sure that #21293 (comment) works? it seems that it won't solve the issue with the Typography. So does it mean that we do no longer support snapshot tests?

EDIT:

https://github.com/mui-org/material-ui/blob/fdd859c7e5d0007c3508012e0e4f93e6d12ffa1f/packages/material-ui/src/TablePagination/TablePagination.js#L119

[eps1lon]

So does it mean that we do no longer support snapshot tests?

We never did and we shouldn't start. It restricts us for something that shouldn't be done in the first place.

[oliviertassinari]

@eps1lon Looking at the issue history around snapshot tests https://github.com/mui-org/material-ui/issues?q=is%3Aissue+snapshot+tests+, it's seems that there is a non neglieable demands for it. This makes me think that no matter which direction we take, I think that we should write it down in the testing guide documentation. Developers should be able to find the information (this is the main outcome I was looking for with my comment in #21293 (comment) 🙂)

Now, I though that we were supporting snapshot tests, e.g. https://github.com/mui-org/material-ui/pull/16523/files#diff-ee6298306b47efa1a027a3bc93186d60R152 (meaning it was possible without being easy) and I was curious about this tradeoff. Should we do the extra mile for developers that think it's helpful for their case or shouldn't we? My initial intuition is to say that if React allows it, we should too, as long as the overhead is small. Which seems to be the case.

I any case, I'm all in for warning developers about the limitations of it and how painful it can be down the road, that they might lose time, have a false feeling of security, etc.

[eps1lon]

I'm tempted to say that if React allows it, maybe we should too, as long as the overhead is small.

What does this mean? You mean you want to support snapshotting the component tree? Because that is even more restrictive than snapshotting the DOM.

It would mean we can't make any change to the render output without breaking tests.

Looking at the issue history around snapshot tests https://github.com/mui-org/material-ui/issues?q=is%3Aissue+snapshot+tests+

You already said it: "history". create-react-app no longer has a snapshot test in the default template but uses @testing-library/react. I don't think we have the bandwidth to support snapshot testing.

[pzduniak]

It's the first time we don't provide any workaround.

<TablePagination SelectProps={{ id: 'stable-select-id', labelId: 'stable-select-label-id' }} />

Weirdly enough that does not fix anything here, we're calling it from TypeScript and it just keeps using the internally generated IDs. Any suggestions?

[oliviertassinari]

To be noted that we have two other duplicate issues that have been opened since Monday's release of v4.10.1: #21314, #21306.

@pzduniak to be clear, there is no "clean" workarounds, for now. The current direction is to encourage developers to drop snapshot testing.

I'm personally leaning toward providing a better escape hatch than mocking Math.random, like using a process.env.NODE_ENV !== 'test' branching logic, a global for useId, or an extra prop.

[pzduniak]

Thank you for the accurate response.

If there's no workaround implemented, then I'd suggest adding a note to the README stating that the maintainers hold strong opinions about somewhat unrelated practices and reserve the right to randomly break compatibility in workflows used widely in the community. Then perhaps we would've been prepared for this change. I'm disappointed.

I'm saying all of this with no offense meant to the maintainers. Just telling y'all how I perceive this whole situation, as a leader of a team that uses the framework to develop early stage commercial software that hasn't become profitable yet. We would support the project if we could, unfortunately all of that is out of our budget right now. And every single time we waste hours / days of effort on fixing our project after a third party breaks it, it makes it less likely that we'll want to stick to using that third party's libraries. You're potentially hurting adoption by the project's future sponsors.

[oliviertassinari]

@pzduniak If you want to go the extra mile, you can mock Math.random().

Looking closer, it seems that React.useOpaqueIdentifier supports snapshot testing thanks to the incremental counter approach.

[pzduniak]

@oliviertassinari Thank you for your suggestions, but I'm currently just trying to roll back to a release that doesn't do this, which opened another can of worms and currently my whole setup is broken because the Node ecosystem is so fragile. If that doesn't work, we'll just purge every single place where TablePagination is used in our code and replace it with something that's usable in our setup.

Please simply consider my initial message as an opinion of a disgruntled user and think about it when making decisions in the future. MUI itself is pretty good and we enjoyed working with it so far, but these things are dealbreakers if you're responsible for any product's success.

EDIT: I finally got it running. Worst case scenario we'll stick to an old version and migrate away to some other library at some point.

[oliviertassinari]

@pzduniak I think that the main interesting element for us would be to explain your use case for running snapshot tests. What do you gain from it? Did you even catch a regression with it? What workflow do you have in place so that your team doesn't blindly accept the changes? Thanks :)

The common story we hear from developers is: "yeah, we thought it was a great idea, after using them for a year, we have decided to drop them, time wasted". This leads to one of the problems: by having Material-UI diverging from the default set of features that React supports, we prevent this leaning path. If we get more feedback in this direction, I will look into how we can improve the snapshot test compatibility.

[pzduniak]

Coincidentally we're currently working on a set of tickets that heavily rely on the functionality!

Our stack is Apollo GraphQL connected to a Go service that currently has a ton of fancy code that automatically optimizes the queries depending on what fields are requested. The app iteslf is an analytics tool that runs on a single partitioned database, most of the time only a single partition is queried.

We didn't have the optimizer in place in the beginning when we wrote most of our components, so nearly all of our current frontend code always queries for everything. We're currently in the middle of a project where we're trying to limit the field counts whenever possible (ie. every single component gets its own specific query), so we're rebuilding every single slow component's data layer, while trying to not modify the shots' output.

We're working around the issue of "blindly trusting the changes" by splitting storyshots into smaller files (every directory with stories gets its own subdir with renders), so that they are readable, and making sure that the tickets are structured in a way where you can make use of the storyshots to validate that you didn't cause any unintended side effects (so mostly data layer / internal hook placement and so on). Obviously if we rework the components from scratch, the storyshots are ignored.

This strategy was inspired by https://github.com/keybase/client, where Storyshots are used roughly for the same purposes (although the data layer there is way more complicated). At Keybase I'd say I mostly relied on them when modifying backend's functionality, as I was adapting all the existing users' code to continue working with the modified RPC calls.

[bbourn]

For our team, after reading this, we are moving to using straight asserts for DOM elements we need to ensure are being rendered instead of comparing to a 'known good' snapshot (of json). This actually seems more bullet-proof. For regression testing we will move to actual graphic representations of pages (NOT json) via puppeteer screenshots and use diff tooling.

[eps1lon]

Are you sure that #21293 (comment) works? it seems that it won't solve the issue with the Typography.

No I wasn't. I have now identified what the issue is. A fix is underway but won't land in v4 because snapshot testing is not a priority for us. Please do your future selves a favor and move away from it. If you need a smoke test to check that a component doesn't crash just call ReactDOM.render().

[eps1lon]

Why not have the StylesProvider have a generateId function that we can also use to give stable IDs in tests?

Because this requires work for a feature that is harmful to our development effort. Matching the DOM structure is not something we consider a public API. What is important is that you customize certain styles, or that the accessibility tree looks the same etc.

I don't know what you mean with "law of unintended consequences.". I only know that the particular shape of the DOM tree is not something we're concerned about at a lower level where snapshot testing works.

We use it to catch unexpected impacts on higher level components when low level components change how they render.

What impact did this change have on your higher level components?

This issue is the perfect example (among others in this repo) why snapshot testing is insufficient. Nothing broke in TablePagination yet your test broke. Instead of re-examining your testing approach we are supposed to spend time on an issue that doesn't affect the end user. This is what technical debt does to your codebase. It creates work without improving the end product.

[pzduniak]

This issue is the perfect example (among others in this repo) why snapshot testing is insufficient. Nothing broke in TablePagination yet your test broke.

Well, the deterministic DOM output from that component broke after a patch release :)

I don't think that anyone is asking for the guarantee of having the exact same DOM output across patch releases. We simply want a specific commit of our code to always output the DOM tree deterministically given specific input.

Instead of re-examining your testing approach we are supposed to spend time on an issue that doesn't affect the end user. This is what technical debt does to your codebase. It creates work without improving the end product.

Seriously? The issue doesn't affect the end user, the issue affects the workflow of teams that are used to a specific approach. Development time isn't free. I literally chose this framework because it worked pretty well with the Storybook/Storyshots combo. If you build out a codebase specifically with Storyshots in mind (which isn't really that much more work), keeping it running isn't really a huge investment. Maybe we use it for trivial stuff that could be tested in a different way, but that does not invalidate the approach. We accepted the tradeoff, that's all.

[oliviertassinari]

To put things in perspective, we have had 3 users request "test - stable snapshot" during our last survey https://material-ui.com/blog/2020-developer-survey-results/.

[eps1lon]

Development time isn't free.

I absolutely agree which is why we're not working on this issue. If you can come up with a solution that doesn't affect prod bundle size and doesn't make the code unreadable I'm happy to review a PR.

[pzduniak]

I'm OK with that as a solution if it actually works. Thanks.

[eric-burel]

Late to the party but just hit this bug. Mocking Math.random sounds good for me, I found an implementation here: https://stackoverflow.com/a/43532567/5513532

And for the lazy ones the code copy pasted from the accepted answer from user Stuart Watt:

const mockMath = Object.create(global.Math);
mockMath.random = () => 0.5;
global.Math = mockMath;

Sorry if I missed something in the discussion but do you have a technical explanation on why you rely on Math.random? To me, mocking this function is a perfectly relevant fix and totally normal in a testing context, where randomness can be a problem, so I definitely buy this solution. But out of pure curiosity it would be interesting to know why you need to introduce randomness in Material UI. Also is it sure that there can't be any conflict? Or are they simply statistically impossible?

Snapshot testing is still the best solution to me to generate unit tests based on my Storybook stories. I trust your feedback that it may not be a good solution in the long run but it's rather useful in the early development iterations and at least catches non rendering components/broken and non building stories...

Btw, there are ongoing work at Storybook to make it easier to use Stories directly in Jest, rather than relying only on snapshots, which would lessen a bit the need for snapshots: storybookjs/storybook#12959.

[oliviertassinari]

@eric-burel We rely on Math.random() as a fallback for a missing unique id: https://github.com/mui-org/material-ui/blob/0edc67f4cc8e34bcb64a89ee1dd495bfeff00b6d/packages/material-ui-utils/src/useId.js#L11

[eric-burel]

Ok I see, and I guess a pseudo-random suite doesn't work? I guess it has risk of being called in parallel and give the same value twice, or maybe it's just slower/adds complexity ? Like this one: https://gist.github.com/mathiasbynens/5670917 (don't know if it is a good random function but you get my point)

Edit: a benchmark between Math.random and this gist (which I found... randomly on the web) is in favour to the pseudo-random function.

But still not sure if it actually gives uniform distribution (lessen risk of collision) + pseudo-random is not robust to parallel calls.
(again purely out of curiosity, this is way beyond the scope of snapshotting)

[oliviertassinari]

@eric-burel Interesting, I think that we could consider a pseudo-generator. I have plotted the distribution in https://www.khanacademy.org/computer-programming/chrome-v8-random/4973161440821248 and https://jsfiddle.net/t90u1fa7/10/.

https://gist.github.com/blixt/f17b47c62508be59987b seems to work as well and is shorter. One note, I had to increase the number of digits we take to increase the first collision from ~200 with 5 to ~8000 with 8.

@eps1lon What do you think, does it worth replacing Math.random()?

[eps1lon]

@eps1lon What do you think, does it worth replacing Math.random()?

I have not done the math. What's the average number of TablePagination one has to have on a page to have a 1% chance of collision?

[oliviertassinari]

@eps1lon It seems that we can have useId() ~10 hooks on a page. We could write a script to compute, for a given random generator, this probability. At this point, we can wait for the outcome of reactjs/rfcs#32. React is currently experimenting with unstable_useOpaqueIdentifier().

@eric-burel if you want to try replacing the random with a pseudo generator, feel free to. It could help.

[yurtaev]

I'm using jest.mock to fix snapshots, with v5:

jest.mock('@mui/utils/useId', () => jest.fn().mockReturnValue('mui-test-id'))

[eric-burel]

The advantage of jest.mock over mocking the random function, is that you may want a pseudo-random mock for Math.random but a constant value just for Material UI (as a pseudo-random will still break often if the DOM slightly change)

[gabrielmendes98]

I'm using jest.mock to fix snapshots, with v5

jest.mock('@material-ui/utils/useId', () => jest.fn().mockReturnValue('mui-test-id'))

With v4

jest.mock('@material-ui/core/utils/unstable_useId', () =>
  jest.fn().mockReturnValue('mui-test-id'),
);

Thanks @yurtaev

this did not work for me. Had to use the following:
jest.mock('@material-ui/core/utils/unstable_useId', () => ({ __esModule: true, default: () => 'mui-test-id', }))

[bill-kasa]

I'm using jest.mock to fix snapshots, with v5:

jest.mock('@mui/utils/useId', () => jest.fn().mockReturnValue('mui-test-id'))

Thank you! This just ended my hour long search. Worked perfectly.

[ThanosDi]

I'm using jest.mock to fix snapshots, with v5:

jest.mock('@mui/utils/useId', () => jest.fn().mockReturnValue('mui-test-id'))

Any idea why this is not working with react-test-renderer ?

Libraries used:

 "@mui/core": "^5.0.0-alpha.54",
 "@mui/material": "5.10.9",
 "jest": "^29.0.3",
 "jest-environment-jsdom": "^29.0.3",
 "jest-jasmine2": "^29.1.2",
 "react-test-renderer": "^18.2.0",

Test file

import React from 'react';
import renderer from 'react-test-renderer';
import IndexPage from '../templates';
import indexQueryData from '../__mocks__/index-data';

describe('Index', () => {
    it('renders correctly', async () => {
        const tree = renderer
            .create(
                <IndexPage
                    data={indexQueryData.data}
                    pageContext={indexQueryData.pageContext}
                />
            )
            .toJSON();

        expect(tree).toMatchSnapshot();
    });
});

In the snapshot file I still have mui-9c4omp as a className in various components that change on every new style.
I'm mostly using sx={}

[krichter722]

For React 18 you also need

jest.mock("react", () => ({
  ...jest.requireActual("react"),
  useId: jest.fn().mockReturnValue("mui-test-id"),
}))

besides jest.mock('@mui/utils/useId', () => jest.fn().mockReturnValue('mui-test-id')) for some components (I experienced with this @mui/x-data-grid 6.10.

I understand the argument that snapshot testing is discouraged and you that don't support it and I agree with it, however some users might be stuck with what they have and knowingly continue bad practice...