[Feature Request]: Add data-testid to Accordion

[bjornalm]

Summary

We need unique test ids for all components and major areas within the components. This is a first small step in that direction.

Justification

To enable support of testing automation in applications that consume the PAL library, we are working to provide a dedicated data-testid data attribute for each component. This is also needed in order to start creating a testability contract and we would like the carbon library to offer the same support. See discussion here https://github.com/carbon-design-system/carbon-addons-iot-react/issues/2446

Desired UX and success metrics

1. The Accordion will get a default property and value for the data-testid property which is added to the outmost html element, in this case the ul element.
2. The AccordionItem will also get a data-testid to the list element, the button and the content area.
3. If the data-testid is supplied to the component this id will be used, if not a default value will be used. Internal components will use available testId values from parent to attempt to provide a unique value.
4. The components and subcomponents can be targeted via querySelector or querySelectorAll.

Required functionality

No response

Specific timeline issues / requests

No response

Available extra resources

No response

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[joshblack]

Hi there @bjornalm! 👋 For background, could you share some examples of tests that you would like to write using these test ids and any corresponding technology used to run the tests? For example, React Testing Library, Selenium, etc. Similarly, if there are any libraries out there that follow this behavior I'd love to take a look at them / reference if you can share any links 👀

In general, data-testid will be passed to components and it will be placed on the outermost element rendered by that component. As we start getting into the internals of a component, our preference would be to use data passed in as props to the component in order to find what is needed.

For example, instead of using the data-testid to identify the accordion item one would use the text rendered in the accordion item to find the button to press.

Would love to hear how this model works with what tools/technology you're using and see where things are working, not working, etc. Thanks for taking the time to write up this issue and make the PR!

[bjornalm]

Hi @joshblack and thanks for your quick response.

I think this RFC that we got for the carbon-addons-iot-react (PAL) library explains the background for my request best. I'm on the IBM PAL team and Internally we use React Testing Library & jest for unit testing and Cypress for e2e testing and we mostly use the strategy of Testing Library (i.e. find the element the way the user would, like you suggest for the accordion item). But we have added test-ids to our component as a complement.

I'm not sure what testing tools our client is using, but I understand that there is huge amount of tests and that they are rather brittle and sensitive to changes in the DOM which poses a real problem. So they want to define a testing interface as a set of locators for each component and relevant elements in them. We believe that we can achieve this with our test-ids, but since the carbon-addons-iot-react library is based on carbon-components-react we also need the test-ids to be available in your components.

I think most if not all testing frameworks/libraries support querying on data- attributes so this would be a framework independent solution. What kind of tests our clients will be writing with this is up to them, we just want to provide the testability contract for each component that we export, including the components from this library and to get started we created this RFC using the Accordion as an example.

[davidicus]

We are attempting to convert them to this newer methodology and they are receptive to it but we are providing these ids as a compliment to help them transition over.

[joshblack]

@bjornalm I definitely think it's fair to expect certain structural contracts (like the element that renders some text is interactive) but the preference in testing would be to find these elements on the page using content passed to a component versus navigating its HTML structure. I think this is what you're hitting at @davidicus where it may be something that could be considered moving forward?

To better understand the requirements, it would help out a ton to see usage scenarios where this is necessary and how alternative testing schemes can't be considered (like the React Testing Library approach you mentioned)

As a potential interim strategy, there could be a helper object/function that could return selectors for different parts of a component. For example, document.querySelector(selectors.accordion.heading) could provide a contract for querying for an accordion heading that would result in fewer changes in the project. If the eventual goal is to use the ideal testing approach above, I think we'd be more interested in an approach like that versus one that would require changes to components in the project.

[davidicus]

@joshblack I think there is still validity in this approach that @bjornalm has outlined. The reason being that Carbon does not consider internals as part of the semantic versioning contract. Because of this, consumer testing could be broken. For instance, in our PAL repo we have had test that would queryByRole and those roles had been changed within a minor version upgrade. We had documented a few examples from our last upgrade here

By having these data-testid landmarks applied to a component and test written that would raise awareness when one of them is removed we can be more cognizant of when things may break downstream. The hope being that consumers would be less affected by internal changes after refactors and their test will be more resilient.

This was just one way we thought of to address the issues that we see being brought up in our org. We are definitely open to other ideas but I do think a data attribute is a pretty low touch solution.

[joshblack]

Sorry for the delay here! Totally my bad.

@davidicus totally get what you mean with the role changing or even how changing how one interacts with a component using a keyboard could ultimately break e2e tests. The ideal for this, from my perspective, would be to use only information that you've passed to a component in order to find it. However, when it comes to operating the component I totally can see how this assumption could break as there is an implicit contract there.

We are definitely open to other ideas but I do think a data attribute is a pretty low touch solution.

The reluctance on my end comes from the number of changes this would need inside of the codebase. For example, if we take a look at accordion we have test ids for:

• The outermost ul of the accordion
• The li of each accordion item
• The button to toggle the visibility of the item
• The content of the accordion
• The skeleton for the accordion
• Each skeleton item for an accordion

When it comes to this component or others, there is a concern that teams may request potentially more test ids depending on their testing strategy. Across 200 odd components, this can feel like an intimidating change.

In the case of accordion, the hope would be that one could:

• Use title to find the toggle button for the accordion
• Use data-testid and pass it to Accordion, AccordionItem, or AccordionSkeleton, if needed, or use the content one owns to query for items inside of an AccordionItem

The reason being that Carbon does not consider internals as part of the semantic versioning contract. Because of this, consumer testing could be broken

From your perspective, is it common for design systems/libraries to consider internals as part of semver? I was trying to find examples of design systems that offered this approach (along with test ids) and would appreciate any links/resources that I could reference for implementation. If this is something that is fairly common and expected when working with design systems then it seems incredibly fair to expect us to consider/implement it.

[davidicus]

Hey @joshblack. No worries. We know you are dealing with a tons of stuff.

The reluctance on my end comes from the number of changes this would need inside of the codebase. For example, if we take a look at accordion we have test ids for:

• The outermost ul of the accordion
• The li of each accordion item
• The button to toggle the visibility of the item
• The content of the accordion
• The skeleton for the accordion
• Each skeleton item for an accordion

This is totally understandable and I think Carbon could decide what the appropriate level of test ids could be. At the very least having the outermost wrapper would be a stable starting point that people could then query from. Some way to meet in the middle.

My comment about your semver contract regarding internals was not a critique. I mentioned it merely to highlight the reason why these testids are valuable to help negate the impact this has on some consumers. I agree with Carbon's philosophy on this but I am trying to serve the needs of our consumers and somehow bridge the gap.

[joshblack]

@davidicus

At the very least having the outermost wrapper would be a stable starting point that people could then query from. Some way to meet in the middle.

Totally fair, I think in v11 we will have all components pass additional props onto the outermost element for this use case. In v10, components should have this capability and if they are currently not doing this we can 100% add it in 👍

Really appreciate you trying to bridge the gap between different expectations and methodologies. I know how hard it can be and I am so thankful for your time/effort here. Would there be a good case study that could be shared to highlight the challenges inherent in the different approaches? Having some code to reference would help out a ton to better understand what challenges exist and where test ids are helping to solve them.

[tay1orjones]

Update - in v11 all components should now place classnames on the outermost element as well as additional props where possible. I think this should alleviate the most primary concern here.

If there's more to tackle, please let us know. I'll underscore that if there's additional needs, having some code/situations to reference would be a great help to ground the conversation.