Jest-dom: toBeDisabled no longer considers parent element

Created on 22 Jun 2020  路  9Comments  路  Source: testing-library/jest-dom
  • @testing-library/jest-dom version: 5.10.1
  • node version: 12.13.1

  • npm (or yarn) version: 1.22.4

  • dom-testing-library version: 6.16.0

  • react-testing-library version: 9.5.0

Relevant code or config:

const button = getByText('disabled button')

expect(button).toBeDisabled();

What you did:

The rendered HTML comes from Material UI and it's a simple <Button /> being passed properties { children: 'disabled button', disabled: true }.

<body style="">
    <div>
      <button
        class="MuiButtonBase-root MuiButton-root MuiButton-contained MuiButton-containedPrimary Mui-disabled Mui-disabled"
        disabled=""
        tabindex="-1"
        type="button"
      >
        <span
          class="MuiButton-label"
        >
          disabled button
        </span>
      </button>
    </div>
</body>

What happened:

In version [email protected] the assertion .toBeDisabled() worked fine, in [email protected] it fails with:

expect(element).toBeDisabled()

    Received element is not disabled:
      <span class="MuiButton-label" />

      36 |     debug();
      37 |     const button = getByText('disabled button');
    > 38 |     expect(button).toBeDisabled();
         |                    ^
      39 |   });
      40 | });
      41 |

It DOES work if I change the assertion to hit the parent element directly, which is indeed the element that's disabled:

const button = getByText('disabled button').parentElement;
expect(button).toBeDisabled();

Reproduction:

https://codesandbox.io/s/react-testing-library-demo-utmkj?file=/src/__tests__/button.js

Problem description:

It seems like earlier versions of jet-dom were considering parent elements to check whether an element was disabled, but no longer now.

picture simoneb

Most helpful comment

it's clear that it is also a breaking change, because whether it's a bug or not, user code is relying on this behavior.

That is unreasonable for library authors. If a dependant relies on a bug then this, while unfortunate, is a problem in the dependent code. SemVer makes it pretty clear that this was a patch.

You're essentially arguing to release any change in a SemVer major because for every package there is some code that can "break" if the package changes (even if you don't change documented behavior".

Then changelog includes that toBeDisabled was changed. It was considered a bug fix because previously toBeDisabled would check elements that couldn't be disabled in the first place.

By definition a bug fix is backwards incompatible for some code i.e. can be considered a breaking change. The question we need to answer is if that previous behavior was documented as such.

picture eps1lon on 22 Jun 2020
👍2

All 9 comments

The 5.10.1 has introduced a fix to not look at elements that are not allowed to be disabled. Take <a/> as an example, even inside a disabled fieldset, the <a/> is clickable by the user so it's not disabled.

I don't know what to think about a <span /> inside a disabled fieldset...

lourenci picture lourenci on 22 Jun 2020

The problem is that you queried the span not the button and a span can't be disabled because it doesn't have any behavior.

To query the button, use getByRole('button', { name: 'disabled button' });

eps1lon picture eps1lon on 22 Jun 2020
👍2

@eps1lon fair enough, it makes sense but I thought I'd report it anyway since this used to work and upgrading broke our CI.

Thanks for the suggested way to query the button, I agree it's more correct and robust. Note that you're introducing a breaking change in a patch release though.

simoneb picture simoneb on 22 Jun 2020
👍2 😕1

Note that you're introducing a breaking change in a patch release though.

Sorry. That was my mistake when merging testing-library/jest-dom#261. In my mind I viewed it as a bug fix, since that's ultimately what it is. But you are right, and I'll keep that more in mind in the future. Thanks!

gnapse picture gnapse on 22 Jun 2020
👍1

In my opinion, a breaking change that's intended exclusively as a bug fix usually doesn't need a major release, as relying on that behavior would generally not be considered part of the public API. A major release would make it harder for users to upgrade and fix this bug. However, I think it could help to update the release/documentation/readme/etc to make the behavioral change more obvious.

nickmccurdy picture nickmccurdy on 22 Jun 2020

This is more of a general discussion. I agree that this is a fix to address a behavior that should probably not have been there in the first place, but at the same time it's clear that it is also a breaking change, because whether it's a bug or not, user code is relying on this behavior.

I do agree that it doesn't deserve a major release, but when I review package upgrades I usually consider major bumps to require much attention, minor bumps a little less and patches to be usually non-breaking.

Now, this is a testing library, so it's not a big deal anyway, but clearly if it is a library used in production, I guess you can agree that getting a breaking change from a patch bump would be somewhat unexpected.

simoneb picture simoneb on 22 Jun 2020
👍2

it's clear that it is also a breaking change, because whether it's a bug or not, user code is relying on this behavior.

That is unreasonable for library authors. If a dependant relies on a bug then this, while unfortunate, is a problem in the dependent code. SemVer makes it pretty clear that this was a patch.

You're essentially arguing to release any change in a SemVer major because for every package there is some code that can "break" if the package changes (even if you don't change documented behavior".

Then changelog includes that toBeDisabled was changed. It was considered a bug fix because previously toBeDisabled would check elements that couldn't be disabled in the first place.

By definition a bug fix is backwards incompatible for some code i.e. can be considered a breaking change. The question we need to answer is if that previous behavior was documented as such.

eps1lon picture eps1lon on 22 Jun 2020
👍2

By definition a bug fix is backwards incompatible for some code i.e. can be considered a breaking change. The question we need to answer is if that previous behavior was documented as such.

Good point. I think this is the key argument here. It was certainly not documented that it would work on non-form elements. However, I can see how it can cause confusion since in their mind they were matching a button and not a span. This is a combination of unfortunate situations, because they were also not using the more semantic getByRole('button') and since the button had a span in it, that element was matched instead of the button.

All-in-all I get your point @simoneb, and I'll still try to take a better look at this in the future for tricky cases, and maybe make a better job at announcing the changes if possible. But overall it was ok to make it a bug fix in this case.

Thanks for bringing the issue up anyway. It certainly helps.

gnapse picture gnapse on 22 Jun 2020

This is not an issue whatsoever, it's an easy fix on our side, so thanks and keep up the great work.

simoneb picture simoneb on 22 Jun 2020
👍1
Was this page helpful?
0 / 5 - 0 ratings

Related issues

alexandernanberg picture alexandernanberg  路  4Comments
maltebaer picture maltebaer  路  6Comments
anyexinglu picture anyexinglu  路  5Comments
gnapse picture gnapse  路  3Comments
gnapse picture gnapse  路  3Comments