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: toHaveAccessibilityValue() matcher #127

Merged
merged 4 commits into from
Nov 16, 2022

Conversation

mdjastrzebski
Copy link
Collaborator

What:

Matcher for matching accessibility value.

Why:

Current toHaveProp is performing strict matching of accessibilityValue prop, while matching based only on some a11y value entries is useful.

How:

Matching code based on *ByA11yValue matcher.

Checklist:

  • Documentation added to the docs
  • Typescript definitions updated
  • Tests
  • Ready to be merged

@pierrezimmermannbam, @AugustinLF, @MattAgn could you help with reviewing this?

@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Merging #127 (ca4ca1a) into main (f050c41) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #127   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        11    +1     
  Lines          193       208   +15     
  Branches        60        66    +6     
=========================================
+ Hits           193       208   +15     
Flag Coverage Δ
node-14 100.00% <100.00%> (ø)
node-16 100.00% <100.00%> (ø)
node-18 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/extend-expect.ts 100.00% <ø> (ø)
src/to-have-accessibility-value.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mdjastrzebski mdjastrzebski force-pushed the feat/to-have-accessibility-value branch from 3f2f3d2 to ca4ca1a Compare November 10, 2022 15:17
Copy link

@AugustinLF AugustinLF left a comment

Choose a reason for hiding this comment

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

Seems good to me. Might be nice to have a second review, I'm not as familiar with this lib as with RNTL.

Copy link

@pierrezimmermannbam pierrezimmermannbam left a comment

Choose a reason for hiding this comment

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

Looks good to me as well ! What concerns me a bit though is that I feel like there is a lot of duplicated code between testing-library/jest-native and RNTL, I wonder if it wound't make sense if this library was part of the RNTL repo (or some other solution that would allow to share code more easily) ?

@mdjastrzebski
Copy link
Collaborator Author

@pierrezimmermannbam There is indeed some code duplication between RNTL and JN. But there are also parts when the they have same concepts but different behaviour which is even worse. I share you point that about bringing them closer together in some form e.g. monorepo or including them in our package, etc. There is definitely benefit in RNTL users using JN matchers, as some assertions are better expressed through queries and some through Jest matchers.

@mdjastrzebski mdjastrzebski merged commit eeb35d1 into main Nov 16, 2022
@mdjastrzebski mdjastrzebski deleted the feat/to-have-accessibility-value branch November 16, 2022 09:13
@github-actions
Copy link

🎉 This PR is included in version 5.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants