Avoid coupling LinkControl test to css class

[getdave]

As we saw here coupling this test to a CSS selector makes it more prone to error.

In general, it's my opinion that we should try to test user perceivable UI rather than developer-centric implementation details.

This PR updates the test to check for text content to identify the Edit button rather than rely on the CSS class implementation detail. It's a minor change but I feel it's worthwhile.

This should make the test more resilient to change, as changing text is a user perceivable change and will, therefore, be more easily identifiable as a cause of test regression.

PS: also sneaking in removal of legacy comments I sholuld have removed in #20182

How has this been tested?

Checking all existing unit tests pass both on Travis and on local

npm run test-unit:watch packages/block-editor/src/components/link-control/

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[retrofox]

Searching by the element content seems to be a little bit fragile as well, but I guess we don't any other way to get it, such as id, a data- attribute, or whatever.

[getdave]

I understand how this might appear. However, my take on this is that this is precisely when I'd want my test to break.

If the text changes to Flibble Wibble then I want the test to say "Whoooooaaa hold on there Mr!, you changed a crucial piece of UI text". This would force me as a developer to acknowledge my change and update the test to reflect it (ie: it couldn't happen by accident).

Contrast this with any of the other approaches mentioned. Changing an id - user can't see it, makes no difference to them but the test fails. Test is coupled to an implementation detail.

[retrofox]

LGTM. So let's wait for tests passing ;-)

[WunderBart]

This should make the test more resilient to change, as changing text is a user perceivable change and will, therefore, be more easily identifiable as a cause of test regression.

This is a good example of how the data-testid attribute would be useful here. We could be adding it to make tests even more resilient to change. The good part of that convention is that the attribute can be removed for non-test builds with the react-remove-properties :)

What do you think about this approach?

https://kentcdodds.com/blog/making-your-ui-tests-resilient-to-change

[retrofox]

This should make the test more resilient to change, as changing text is a user perceivable change and will, therefore, be more easily identifiable as a cause of test regression.

This is a good example of how the data-testid attribute would be useful here. We could be adding it to make tests even more resilient to change. The good part of that convention is that the attribute can be removed for non-test builds with the react-remove-properties :)

What do you think about this approach?

https://kentcdodds.com/blog/making-your-ui-tests-resilient-to-change

I exactly meant to this! Thanks, Bart for the information. 👏

[getdave]

This should make the test more resilient to change, as changing text is a user perceivable change and will, therefore, be more easily identifiable as a cause of test regression.

This is a good example of how the data-testid attribute would be useful here. We could be adding it to make tests even more resilient to change. The good part of that convention is that the attribute can be removed for non-test builds with the react-remove-properties :)

What do you think about this approach?

https://kentcdodds.com/blog/making-your-ui-tests-resilient-to-change

Really appreciate the feedback from both @retrofox and @WunderBart 👍

@WunderBart See #20190 (comment)

I'm aware of the test-id approach. I'm a fan in certain circumstances (ie: if there is no other way to grab an element). However, in 99% of cases there is:

• Text
• Label
• Aria role

All of these are user perceivable UI which we can use to select the correct element. By making our tests conform as closely to how a user interacts with our component we achieve less brittle tests which better capture the intent of the component's functionality.

You've referenced the exact article I would have done. Here's a good quote:

Given that "the more your tests resemble the way your software is used, the more confidence they can give you", it would be wise of us to consider the fact that our users don't care what our class names are.

Have you used RTL? If you have you know they prefer a certain order of matches.

Granted my approach here is less optimal then getByText from RTL, but i feel it's much better than a className selection.

[aduth]

This seems like a fine change. The main concern with this sort of approach is that the developer writing the test should be wary to not make the selection too broad that it risks choosing an unintended element. For example, there are probably many buttons on the editor screen whose HTML includes "Edit". I think this was accounted for sufficiently here (since the querySelector is applied to container), and is more of a general note of caution for this approach in selecting elements for test.

As we saw here coupling this test to a CSS selector makes it more prone to error.

There's some truth to this in that it might not have failed if it was targeting a label. I also think it's worth highlighting the issue that #19738 should have been rebased before it was merged, since it was running tests on a stale codebase. If it had been rebased, the build would have failed, and the issue would have been avoided.