New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test failure caused by ra-material-ui@4.16.8+ #9655
Comments
Also, this would've been a little easier to detect and avoid breaking things if react-admin actually depended on pinned versions of the ra-* packages. Given that all packages are released at the same time, I don't see any reason it would be desirable to install newer versions of a ra-* package than the react-admin version. i.e. My dependency is still react-admin@4.16.7, but anybody doing a new install will get the broken ra-material-ui package. If react-admin depended on "4.16.7" instead of "^4.16.7", then this would be more reliable. |
Thanks for your report. This seems to be a problem in your code that was hidden by react-admin before, but that you need to fix on your side. If you open a dialog, it's your responsibility to close it. You may need to customize the update side effect for that, which you can do with the I don't believe that ra-ui-materialui is broken-unless you can provide a reproduction. Until then, as I believe the problem is on your side, I'm closing this issue. |
I'm confused on why you think this? Firstly, my code didn't open the dialog, I merely set Again, clicking the confirm button in a typical browser, like Firefox, does still close the confirmation dialog. |
If it's react-admin that opened the dialog (with I'm reopening this issue, but you must provide a repro for us to look further. |
Sorry, I linked the code change in the bulk update button that seems like the only suspect for breaking it, and added comments in the code snippet to highlight that it was a bulk update button. Forgot to explicitly mention that it was in pessimistic mode though. |
This seems to indicate the problem comes from your test implementation and not from RA's code. Possible lead: -await userEvent.click(screen.getByRole("button", {"name": "Confirm"}));
+await userEvent.click(await screen.findByRole("button", {"name": "Confirm"})); |
Hmm, it's the only notable change I could see related to it. Will do a bit more testing...
Well, my test uses standard test functions, but it could highlight a subtle issue in jest-dom.
These are equivalent, as the getByRole() would have produced an error if the button wasn't already present. |
Good point. I must admit I don't see anything else that could be wrong in your test. Let's wait for a reproduction on #9659 |
This is going to be tricky for me to create a reproducer here, so I'm hoping the issue will be obvious to one of you.
When using resolutions, if I set ra-material-ui to 4.16.7 then my tests pass. At 4.16.8+ they start failing.
Looking through the small number of changes in that release, this seems like the only obvious change that could have caused it:
https://github.com/marmelab/react-admin/pull/9600/files#diff-5f9cd2ee8b802d89b35ab1d0f4df69abfd29f37dc0208f4a836c4d6dbb629266
The test code looks like:
The test now fails with (https://github.com/aio-libs/aiohttp-admin/actions/runs/7705984738/job/21389490839?pr=854):
Using screen.debug(), I can see that the confirmation dialog is still visible in the HTML. It hasn't closed since clicking the button.
My guess is that without the refresh() call, the confirmation dialog doesn't get removed in jest-dom.
The dialog seems to work correctly when I use the app in Firefox though, so this seems to only affect tests.
The text was updated successfully, but these errors were encountered: