Test failure caused by [email protected]+

[Dreamsorcerer]

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:

        const container = await screen.findByRole("columnheader", {"name": "Select all"});
        const selectAll = within(container).getByRole("checkbox");
        await userEvent.click(selectAll);
        expect(selectAll).toBeChecked();
        // This is a bulk update button.
        await userEvent.click(await screen.findByRole("button", {"name": "Set to 7"}));
        // This is the confirmation dialog (as a result of mutationMode=pessimistic).
        expect(await screen.findByText("Update 6 simples")).toBeInTheDocument();
        await userEvent.click(screen.getByRole("button", {"name": "Confirm"}));
        await waitFor(() => screen.getAllByText("7"));

The test now fails with (https://github.com/aio-libs/aiohttp-admin/actions/runs/7705984738/job/21389490839?pr=854):

Unable to find an element with the text: 7
...

      50 |         expect(await screen.findByText("Update 6 simples")).toBeInTheDocument();
      51 |         await userEvent.click(screen.getByRole("button", {"name": "Confirm"}));
    > 52 |         await waitFor(() => screen.getAllByText("7"));
         |                      ^
      53 |
      54 |         const table = await screen.findByRole("table");
      55 |         const rows = within(table).getAllByRole("row");

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.

[Dreamsorcerer]

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 [email protected], 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.

[fzaninotto]

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 <Edit mutationOptions> prop.

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.

[Dreamsorcerer]

If you open a dialog, it's your responsibility to close it.

I'm confused on why you think this? Firstly, my code didn't open the dialog, I merely set mutationMode=pessimistic. Second, there are no examples in the documentation that suggest I need to do anything or even provide a way to get a reference to the dialog.

Again, clicking the confirm button in a typical browser, like Firefox, does still close the confirmation dialog.

[fzaninotto]

If it's react-admin that opened the dialog (with mutationMode=pessimistic), then it's react-admin's responsibility to close it ;) You didn't specify it in your report, so I assumed you opened the dialog.

I'm reopening this issue, but you must provide a repro for us to look further.

[Dreamsorcerer]

You didn't specify it in your report, so I assumed you opened the dialog.

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.

[slax57]

My guess is that without the refresh() call, the confirmation dialog doesn't get removed in jest-dom.

refresh() only affects the data managed by react-query, so to me this is rather unlikely.

The dialog seems to work correctly when I use the app in Firefox though, so this seems to only affect tests.

This seems to indicate the problem comes from your test implementation and not from RA's code.

Possible lead: Can you try changing the line 51 of your test by the following?

-await userEvent.click(screen.getByRole("button", {"name": "Confirm"}));
+await userEvent.click(await screen.findByRole("button", {"name": "Confirm"}));

[Dreamsorcerer]

My guess is that without the refresh() call, the confirmation dialog doesn't get removed in jest-dom.

refresh() only affects the data managed by react-query, so to me this is rather unlikely.

Hmm, it's the only notable change I could see related to it. Will do a bit more testing...

This seems to indicate the problem comes from your test implementation and not from RA's code.

Well, my test uses standard test functions, but it could highlight a subtle issue in jest-dom.

Possible lead: Can you try changing the line 51 of your test by the following?

-await userEvent.click(screen.getByRole("button", {"name": "Confirm"}));
+await userEvent.click(await screen.findByRole("button", {"name": "Confirm"}));

These are equivalent, as the getByRole() would have produced an error if the button wasn't already present.

[slax57]

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 https://github.com/marmelab/react-admin/pull/9659

[fzaninotto]

No news for some time, closing.