Proposal: prefer targeting interactive elements, document dataTest exports

[amcgee]

This is a synthesis of the discussions in #218 and #219 and LIBS-118 (for transfer)

We should formalize a policy about which elements within a component get data-test attributes and how we communicate this behavior to the consumer.

I propose the following simple guidelines:

1. Each user-interactive (focusable, i.e input/button/etc.) or prop/state-driven (dynamic, anthing that is user-facing and could change between renders) element in a component should be given a functionally-named dataTest attribute. This should be prefixed with the passed-in dataTest prop, and should be named after the function of the element rather than the type of element (unless they can be used interchangably). Example: ${dataTest}-search or ${dataTest}-output not ${dataTest}-input and ${dataTest}-div.
2. If a component has exactly one functional element (i.e. Button is just a styled button) or one clear "primary" element it is OK to omit the postfix from this element (i.e. <button dataTest={props.dataTest} />). This should be used carefully, however, in cases where a future version of the library might increase the complexity of the component.
3. Non-functional elements (container divs, etc.) shouldn't generally have dataTest props. The one exception, only if deemed necessary, is ${dataTest}-container on the ROOT element of a container. This is allowed, as it could occasionally be helpful for targeting and should be stable.
4. Incremental adoption of (1) above is allowed, meaning that it's OK to lazily add dataTest properties to additional functional elements in a component as they are needed or requested by consumers. They should, however, be named and added in a way that assumes that at some point in the future ALL focusable and dynamic elements will have dataTest properties, so the chosen names should be suitably unique.
5. REMOVING a dataTest assignment or CHANGING THE TYPE OF TARGETED ELEMENT is technically a breaking change (for tests, not for runtime) so should be avoided unless absolutely necessary. We should decide whether to trigger major version bumps for dataTest changes or to only guarantee dataTest stability across patch versions, which I think could be acceptable (let's discuss this separately, thread below)
6. Whenever there is a CHILD element which also accepts a dataTest prop (i.e. another ui component) it should inherit from the parent's passed dataTest prop - i.e. if the parent component renders a Button, that Button should be rendered with dataTest ${parentDataTest}-${buttonFunction}, so that the specified dataTest propagates all the way down to the final plain DOM elements
7. Each component MUST document each of the dataTest elements it "exports", similar to the documented table of proptypes. (cc @KaiVandivier)

[amcgee]

Breaking dataTest changes

I'd like to propose that we do not consider removal of a dataTest attribute (or changing the type of element it targets) as a breaking change which would trigger a new major version. The library's runtime API should be unaffected by these changes, and so bumping the major version would only cause version churn and thereby likely version stagnation in runtime code - tests would need to be updated if these dataTest attributes change, but those test issues should always be caught at the dependency update PR stage.

Thoughts welcome, please discuss in this thread =>

[ismay]

The goal here as I see it is to allow you to use it to target picked options (as opposed to the other list of options). Also, maybe there's a case to be made for say, wanting to check the height of that list. Or do something else that relies on the container. I'd say that as a distinct part of the ui it's reasonable to have it as part of the data-test props.

I do believe that we have several tests that check such things. Maybe we should take a look at what we're doing in the current test suite, and see whether the definition fits with all those tests.

[amcgee]

Are there scenarios where we've correctly applied data-test props in the current codebase where a data-test attribute would be removed, but there wouldn't be a breaking api change?

There are, though perhaps rare - if the Transfer component had a small <label> which just stated what the current applied filter was, which was assigned a dataTest attribute, and later @cooper-joe recommended we remove this label (no changes at all to the API of Transfer) we would also be removing that dataTest attribute without a functional breaking change.

Similarly and more subtly, if a component uses an Input and later changes to a textarea for the same functional purpose, this could be considered a breaking change for tests (they might be looking for an attribute of an input that doesn't exist on textarea)

[amcgee]

@ismay I'm not deadset on forbidding non-functional dataTest assignment, but let's create a separate thread in this discussion for that topic (keep this focused on breaking changes)

[amcgee]

Being strict becomes quite nasty when we support dynamic runtime module resolution - if ui major version is bumped without any actual runtime breaking changes, all apps will fallback to their bundled versions even though they could technically use the newer one just fine (test infrastructure being the exception).

Even without runtime modules, the PR or yarn dependency upgrade process across breaking changes is necessarily more burdensome on all application developers (rather than just test authors using dataTest) because you need to check the changelog and audit all use of the library to make sure there aren't actual API breakages you're using. Similarly here, with a package.json dependency of ^6 running yarn upgrade will automatically pick up any minor changes, but if a dataTest change had cut v7 of the library any subsequent minor/bugfix changes will NOT be automatically picked up until the developer manually upgrades to the latest major.

[varl]

To reduce the complexity of the rules a bit, how about:

• Only changes that changes the public facing API is a breaking change. Changes to internal implementation details (structure, elements, attributes) do not facilitate a breaking change.

Since we only commit to the component API being stable, removing dataTest would be a breaking change, but changing an element that has the data-test is not considered a breaking change. Some goes for other internal changes.

[amcgee]

we might want to ALWAYS have a root ${dataTest} element in every component, so that nesting works well... maybe, not sure about this one.