Material-ui: [TablePagination] Snapshot test issue

[is1ash]

Recent changes to compnent TablePagination break test.

<Typography color="inherit" variant="body2" className={classes.caption} id={labelId}> 
  {labelRowsPerPage}
</Typography>

link

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 https://github.com/mui-org/material-ui/issues/17070#issuecomment-523009146 to include this problem. It will help when revamping https://material-ui.com/guides/testing/.

@eps1lon Are you sure that https://github.com/mui-org/material-ui/issues/21293#issuecomment-638660839 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 https://github.com/mui-org/material-ui/issues/21293#issuecomment-638290461 🙂)

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().

[nickpalmer]

Sorry I missed this issue in my search. Thanks for pointing it out.

@eps1lon Snapshot testing is very useful against the law of unintended consequences. We use it to catch unexpected impacts on higher level components when low level components change how they render.

As a workaround I am using this:

    expect(wrapper.html().replace(/id="mui-[0-9]*"/g, '').replace(/aria-labelledby="(mui-[0-9]* *)*"/g, '')).toMatchSnapshot();

[nickpalmer]

@eps1lon In terms of resolving the problem permanently, why not allow the passing of an id generator function, like you do for class names? In our tests we use a test specific generateClassName to get stable class names across tests. Why not have the StylesProvider have a generateId function that we can also use to give stable IDs in tests?

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

śr., 8 lip 2020 o 10:59 Sebastian Silbermann [e-mail]
napisał(a):

Closed #21293 https://github.com/mui-org/material-ui/issues/21293 via

21703 https://github.com/mui-org/material-ui/pull/21703.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/mui-org/material-ui/issues/21293#event-3524192117,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAG753TRPZ5Y2X4ZQTS52LTR2QYOHANCNFSM4NRQW3JQ
.

--
Piotr Zduniak
https://zduniak.net