Consider removing testing-library/no-node-access lint rule

[huwshimi]

To build a component library around Vanilla we really care about the class names, visual elements, node order etc. not just the accessible labels and roles

To do this we often need to access the DOM directly so a lot of tests require disabling testing-library/no-node-access so possibly we should just disable that lint rule (though I could be overlooking some way of checking these things with RTL).

[bartaz]

I guess the question also is if we need to check these things in the test?
Do we want to test just component functionality (which RTL mostly focuses on), or do we want to also test specific DOM structure and class names?

[huwshimi]

I guess the question also is if we need to check these things in the test? Do we want to test just component functionality (which RTL mostly focuses on), or do we want to also test specific DOM structure and class names?

My feeling is that we do. Because Vanilla defines specific classes and elements (with specific orders) we need to catch any regressions that would result in a broken UI.

[bartaz]

My feeling is that we do. Because Vanilla defines specific classes and elements (with specific orders) we need to catch any regressions that would result in a broken UI.

True. While it seems agains the principles of RTL, we not only want to test how user interacts with component, but if the component aligns with Vanilla (HTML structure and class names).

I started reviewing some of the migration PRs and there are couple of aspects related to that.

1. Accessing root element of the component

In some cases we want to access root element of the component - to test a snapshot, to check if it has class names, etc.

In current PRs it seems we try to use proper RTL selectors via role. It works in some cases (button), but doesn't in others. Selecting by the "group" role, because root element is a div is not very helpful.

@petermakowski suggested using container.firstChild to access the root of component, which should be fine I guess. Maybe we can just use container when we don't care about specific node (like for snapshot)?

2. Snapshots

On one hand snapshot tests seem to fit into testing compatibility with Vanilla - they exactly check the structure of HTML and class names.
But the specifics of what is tested are "hidden" in the snapshot. The test itself doesn't assert any specific structure or class name, it just says "match the snapshot" - and snapshot will always be what is rendered by React, not what we expect it to be from Vanilla.

So while technically the test "works" - it fails when React structure changes - I'm not sure if it serves the purpose. Because we will quite likely just regenerate the snapshot to make it pass rather than check and notice why it doesn't pass.

So, do we need snapshot testing? Or should we more explicitly test for some elements/classnames in the code?

3. Dropping no-node-access rule

I guess we are clear here that we do need to access the nodes and classnames in SOME of the tests. Question is, is it enough to remove this rule from linting?

If we do remove it, it may start to be tempting to write all the tests using nodes and class names instead of more semantic selectors. Hopefully we only need to disable it in a fraction of tests, and majority should use proper RTL approach?

Maybe we can even somehow more explicitly separate "functionality" tests (check if components works as expected for user, this should be possible with clean RTL approach), and "structure" tests (check if component is consistent with Vanilla, which may require selecting nodes directly)?

But for the start my suggestion would be to keep the no-node-access rule. And if we notice that we need to disable it more often that we want to, we can consider removing it, or dropping severity to warning.

[huwshimi]

My feeling is that we do. Because Vanilla defines specific classes and elements (with specific orders) we need to catch any regressions that would result in a broken UI.

True. While it seems agains the principles of RTL, we not only want to test how user interacts with component, but if the component aligns with Vanilla (HTML structure and class names).

I started reviewing some of the migration PRs and there are couple of aspects related to that.

1. Accessing root element of the component

In some cases we want to access root element of the component - to test a snapshot, to check if it has class names, etc.

In current PRs it seems we try to use proper RTL selectors via role. It works in some cases (button), but doesn't in others. Selecting by the "group" role, because root element is a div is not very helpful.

@petermakowski suggested using container.firstChild to access the root of component, which should be fine I guess. Maybe we can just use container when we don't care about specific node (like for snapshot)?

That would be fine, we'd probably also want to disable the testing-library/no-container rule in that case.

2. Snapshots

On one hand snapshot tests seem to fit into testing compatibility with Vanilla - they exactly check the structure of HTML and class names. But the specifics of what is tested are "hidden" in the snapshot. The test itself doesn't assert any specific structure or class name, it just says "match the snapshot" - and snapshot will always be what is rendered by React, not what we expect it to be from Vanilla.

So while technically the test "works" - it fails when React structure changes - I'm not sure if it serves the purpose. Because we will quite likely just regenerate the snapshot to make it pass rather than check and notice why it doesn't pass.

So, do we need snapshot testing? Or should we more explicitly test for some elements/classnames in the code?

We've been moving away from snapshots in MAAS, particularly as with RTL they become a full snapshot of the tree. My feeling is that specific tests for elements/classnames are generally more useful. Having said that the snapshots do catch regressions when a change results in an unexpected diff in the snapshot (e.g. doing a refactor that should result in the same HTML).

I think generally for our apps we probably don't need snapshots, but part of me is leaning towards keeping these in react-components as I think we generally want to be a bit more pedantic as any breakage in react-components could break all our sites, but I don't feel too strongly either way.

3. Dropping no-node-access rule

I guess we are clear here that we do need to access the nodes and classnames in SOME of the tests. Question is, is it enough to remove this rule from linting?

If we do remove it, it may start to be tempting to write all the tests using nodes and class names instead of more semantic selectors. Hopefully we only need to disable it in a fraction of tests, and majority should use proper RTL approach?

Maybe we can even somehow more explicitly separate "functionality" tests (check if components works as expected for user, this should be possible with clean RTL approach), and "structure" tests (check if component is consistent with Vanilla, which may require selecting nodes directly)?

But for the start my suggestion would be to keep the no-node-access rule. And if we notice that we need to disable it more often that we want to, we can consider removing it, or dropping severity to warning.

I guess the other side of this is that we don't want to discourage testing these components properly. It's not actually wrong to check the DOM structure or classes but having to override the lint rule makes it feel like you're doing something naughty.

[bartaz]

Triage: seems that for now we are able to handle it as is.

For the new architecture of Vanilla we should revisit the decision on how we want to write tests and what level of access to give in tests.