[docs][Rating] Testing the component with @testing-library/user-event results in NaN

[IgnusG]

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/p/sandbox/eloquent-breeze-7dtljr (run test task and scroll up for console log)

Steps:

1. Install @testing-library/user-event and setup a Rating component with an onChange handler
2. In a jest test use

const user = userEvent.setup();

user.click(screen.getByLabelText('2 Stars'))

4. Observe the onChange handler is fired with newRating set to NaN

Current behavior 😯

Due to the way user events simulates user interaction the handleMouseMove method in MUI's Rating is fired (since user event simulates the mouse movement before issuing the click event). This method however relies on .getBoundingClientRect which is not available in jest - or rather it is but all the values are set to 0 which means the percent calculation tries to divide by 0 ((event.clientX - left) / (width * max)) which results in a NaN being set as the hover state.

In onChange since hover is set even though the parseFloat(event.target.value) parses correctly it is overridden by the NaN from hover.

Expected behavior 🤔

Either this edge case is documented - I didn't find anything about this handleMouseMove behavior and how it takes precedence over the click when running the onChange callback in the Rating docs or the calculation is adjusted to take into account that width could potentially be 0 and the hover should not be set (or be set to -1) in this case - such that onChange still reports the event.target.value instead.

Context 🔦

No response

Your environment 🌎

npx @mui/envinfo

System:
    OS: macOS 13.5
  Binaries:
    Node: 18.14.2 - ~/.asdf/installs/nodejs/18.14.2/bin/node
    Yarn: 1.22.19 - ~/.asdf/shims/yarn
    npm: 9.5.0 - ~/.asdf/plugins/nodejs/shims/npm
  Browsers:
    Chrome: 116.0.5845.140
    Edge: Not Found
    Safari: 16.6
  npmPackages:
    @emotion/react: 11.11.1 => 11.11.1
    @emotion/styled: 11.11.0 => 11.11.0
    @mui/base:  5.0.0-beta.0
    @mui/core-downloads-tracker:  5.12.2
    @mui/icons-material: 5.10.3 => 5.10.3
    @mui/lab: 5.0.0-alpha.73 => 5.0.0-alpha.73
    @mui/material: 5.10.3 => 5.10.3
    @mui/private-theming:  5.12.0
    @mui/styled-engine:  5.12.0
    @mui/system: 5.10.3 => 5.10.3
    @mui/types:  7.2.4
    @mui/utils: 5.10.3 => 5.10.3
    @mui/x-date-pickers: ^6.6.0 => 6.6.0
    @types/react: 18.2.13 => 18.2.13
    react: 18.2.0 => 18.2.0
    react-dom: 18.2.0 => 18.2.0
    typescript: 5.1.6 => 5.1.6

[DiegoAndai]

Hey @IgnusG, thanks for the report!

This method however relies on .getBoundingClientRect which is not available in jest - or rather it is but all the values are set to 0

This seems like a testing limitation. In Material UI's Rating tests, there's getBoundingClientRect stubbing: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/Rating/Rating.test.js#L44. Do you think that's a good enough workaround for your use case?

[github-actions[bot]]

Since the issue is missing key information and has been inactive for 7 days, it has been automatically closed. If you wish to see the issue reopened, please provide the missing information.

[IgnusG]

Hey @DiegoAndai 👋

Yeah I also resorted to stubbing to resolve this issue. Maybe MUI will handle this edge case more gracefully in the future but until then I think it's a good enough solution :+1:

I would only suggest to add this gotcha to the Rating component's documentation. I couldn't find anything in there about this behavior and it took me a few hours of digging in the source code to find out this was the cause.

[ausram]

In the meantime, for anyone else intimidated by the stubbing workaround, another way to avoid this issue when testing the component is to set the skipHover option to the userEvent setup:

const user = userEvent.setup({ skipHover: true })
user.click(screen.getByLabelText('2 Stars'))

[DiegoAndai]

I would only suggest to add this gotcha to the Rating component's documentation. I couldn't find anything in there about this behavior and it took me a few hours of digging in the source code to find out this was the cause.

I agree that documenting this would be useful. I will add the ready to take and good first issue to see if we get any contributions. The work would be to add a small section in the Rating component docs about stubbing/skipping hover when testing.

If anyone is interested on working on it, let us know 😊

[rajabhi21]

@DiegoAndai can you please assign it to me? I'd like to work on it.

[DiegoAndai]

@rajabhi21 sure! Go ahead. Let me know if you need any help. Thanks in advance.

[p-pych]

Hi @DiegoAndai as i've not seen any progress on this for last two weeks. I decided to try my luck. Please find attached PR. I probably forgot about the label :( . https://github.com/mui/material-ui/pull/44129

[oliviertassinari]

This is how the component is tested:

https://github.com/mui/material-ui/blob/064fa67058fa16b76216c04da37c360ad304743b/packages/mui-material/src/Rating/Rating.test.js#L114-L122

So far we avoided @testing-library/user-event in the test as it's slower and hides events away, sometimes having wrong behavior too relative to the platform. In the reproduction, this works:

 // @vitest-environment jsdom

 import { Rating } from "@mui/material"
 import { render, fireEvent, screen } from "@testing-library/react"
 import userEvent from "@testing-library/user-event"

 import { describe, test } from 'vitest';

 describe('Rating', () => {
     test('Rating', async () => {
         const user = userEvent.setup();
         render(<Rating onChange={(_, newRating) => console.log('newRating', newRating)} />);
    
-        await user.click(screen.getByLabelText('2 Stars'));
+        fireEvent.click(screen.getByLabelText('2 Stars'));
     })
 })

https://codesandbox.io/p/devbox/wispy-fire-7lsnzs?file=%2Fsrc%2FApp.test.tsx%3A22%2C1

Now, yeah, I see @testing-library/user-event has been growing a bit in popularity: https://npm-stat.com/charts.html?package=%40testing-library%2Fuser-event&package=%40testing-library%2Fdom&from=2020-10-15&to=2024-10-15. Something from 43% to 59% in 4 years.

We have 1. docs as a lever, I think that we also have:

2. Add a test with user.click to make sure it works and be something others can copy
3. Consider special handling in the logic if getBoundingClientRect is not available, effectively for tests, we have a couple of those already in the codebase. Lately, we discussed this in https://github.com/mui/mui-x/issues/12983#issuecomment-2094300451

On updating the docs 1., my instinct as a developer would be to go straight to check the source to see how it's tested, I wouldn't trust the docs so much because it doesn't run in the CI, it could get outdated. If we do 3. We don't need 1.

+1 on my end to do 3. then 2, and skip 1 because 3 might be a minimal overhead compared to the DX pain (not a strong preference, only #suggestion).

[NooBat]

Can I ask whether are we gonna go with 3. -> 2.? I'd love to work on this approach

[DiegoAndai]

@NooBat, sorry for the delay. I agree with going with 3. and 2. If you (or anyone) wish to implement it, go ahead. Feel free to let me know if you need any help. Thanks in advance.

[0210shivam]

Hey @DiegoAndai Just saw that this issue is assigned to you but still has the good first issue label. Are you actively working on it? Just asking since the label might be a bit confusing for others looking for something to pick up. No rush — just wanted to check in!

[DiegoAndai]

Hey @0210shivam, sorry for the late reply.

You can see the discussion in https://github.com/mui/material-ui/pull/45054#discussion_r1936811292, I think we should only add a warning about this in the docs. Feel free to open a PR for that if you would like to contribute. I would be happy to review it.

[0210shivam]

Hey @DiegoAndai , thanks for your time and for pointing me to the discussion. I'm happy to take this on — already on it!

[0210shivam]

Hey @DiegoAndai

I've gone through the discussion in #45054 and understand that we should document a warning about hover behavior when testing the Rating component in environments like Jest + JSDOM.

Here’s the warning I plan to add:

Warning: When testing the Rating component in environments such as Jest + JSDOM, user interactions like hover may not behave as expected. This is because the component relies on getBoundingClientRect() to calculate hover state, which returns 0 values by default in JSDOM. This can lead to unintended issues, such as NaN being passed to the onChange handler.

Could you advise on the best place in the documentation to add this?

Happy to open a PR once I have confirmation on the best placement. Thanks again!

[DiegoAndai]

Hey @0210shivam! Let's add a Testing section under Accessibility explaining this, and if you can think of any workarounds, let's add that as well.

[0210shivam]

Hey @0210shivam! Let's add a Testing section under Accessibility explaining this, and if you can think of any workarounds, let's add that as well.

Got it! I’ll make those updates. Thanks for your help!

[0210shivam]

Hi @DiegoAndai ! I've added a PR addressing this: #46268 . Let me know what you think when you get a chance — would appreciate a review!

[github-actions[bot]]

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.