Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Adds test id to cards #2976

Merged

Conversation

orangevolon
Copy link
Contributor

@orangevolon orangevolon commented Nov 4, 2024

Description

Adds testId extractor function to cards inside the card component. Unlike other components, cards can't have a single testId prop as the items are generic with no base types. For this reason we use a function to extract test id for every individual item.

Related links, issue #, if available:

Collective PR: #2985
Project: Improving test utils API

How has this been tested?

Tests added to cover the change.

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.27%. Comparing base (ba58915) to head (c3ef382).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2976      +/-   ##
==========================================
+ Coverage   96.25%   96.27%   +0.02%     
==========================================
  Files         768      769       +1     
  Lines       21750    21776      +26     
  Branches     7434     7042     -392     
==========================================
+ Hits        20935    20965      +30     
+ Misses        807      803       -4     
  Partials        8        8              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@orangevolon orangevolon changed the base branch from main to feat/test-utils-project-m2 November 4, 2024 10:35
@orangevolon orangevolon changed the title Alamiami/adds test id to cards feat/adds test id to cards Nov 6, 2024
@orangevolon orangevolon changed the title feat/adds test id to cards feat: adds test id to cards Nov 6, 2024
@orangevolon orangevolon changed the title feat: adds test id to cards feat: Adds test id to cards Nov 11, 2024
@orangevolon orangevolon force-pushed the alamiami/adds-test-id-to-cards branch 2 times, most recently from da09f0e to 7411bf8 Compare November 11, 2024 15:01
@orangevolon orangevolon marked this pull request as ready for review November 11, 2024 15:02
@orangevolon orangevolon requested a review from a team as a code owner November 11, 2024 15:02
@orangevolon orangevolon requested review from avinashbot and removed request for a team November 11, 2024 15:02
Copy link
Member

@avinashbot avinashbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, but left a non-blocking doc comment.

@@ -30,6 +31,19 @@ export class CardWrapper extends ComponentWrapper {
return this.findAllByClassName(styles.section).map(c => new CardSectionWrapper(c.getElement()));
}

/**
* Returns the wrapper of the first card section that matches the specified test ID.
* Looks for the `data-testid` attribute that is assigned via `sections.testId` prop.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, since I don't want to derail all the other PRs: why do we even have this line? From a builder perspective, all they care about is that they provided a testId and this util finds it based on what you provided. Why would I need to care about the fact that it's using data-testid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data-testid is now part of our public API. This is for keeping the compatibility with RTL and other tools that look for data-testid.

This is actually one of the reasons why we decided to use this attribute rather than internal ones like data-awsui-id or others.

You can check the conversation in this PR or comments in related API proposal.

@orangevolon orangevolon merged commit 5b056ba into feat/test-utils-project-m2 Nov 14, 2024
1 check passed
@orangevolon orangevolon deleted the alamiami/adds-test-id-to-cards branch November 14, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants