feat: ...byRole type queries accepts second arg by kiranjd · Pull Request #875 · callstack/react-native-testing-library

kiranjd · 2021-12-02T14:51:56Z

Closes#827

accepts a 'name' property to refine query

Summary

Test plan

thymikee

Looks like it's a good direction. Please take a look at 31ef0b9 to see how to resolve API changes in a backward compatible way

…react-native-testing-library into feat/by-role-second-arg

AugustinLF · 2022-03-28T15:17:16Z

@kiranjd is it something you're still interested in getting in? This is a feature that is clearly lacking for our current API, and I'd love to help you getting that in a mergeable state.

kiranjd · 2022-03-28T15:35:57Z

@AugustinLF Missed following up on this. I remember I had a few questions. I'll post the questions and follow it up to finish within this week. Thank you 🙏

…react-native-testing-library into feat/by-role-second-arg

kiranjd · 2022-03-30T10:55:07Z

 createQueryByError,
 } from './errors';

+type QueryOptions = {


The name in QueryOptions type for getBy is optional. But, it's a must for the function, filterWithName. Is this duplication that I get rid of or is this okay?
(I haven't worked with types all that much 😬 )

- adds tests for deprecations - change second param on findBy queries

…ing-library into feat/by-role-second-arg

AugustinLF · 2022-03-31T15:44:22Z

+ 'interval',
+ 'stackTraceError',
+];
+const warnDeprectedWaitForOptionsUsage = (queryOptions?: WaitForOptions) => {


Suggested change
constwarnDeprectedWaitForOptionsUsage=(queryOptions?: WaitForOptions)=>{
constwarnDeprecatedWaitForOptionsUsage=(queryOptions?: WaitForOptions)=>{

Also pls move it to helpers/errors.ts, as this looks like reusable function that can be used with other findByXxx queries if they decide to go with query options.

AugustinLF · 2022-04-25T09:14:50Z

@kiranjd Do you need any help, is there anything I can do to help you getting that in? :)

mdjastrzebski · 2022-04-28T21:16:55Z

 type FindReturn = Promise<GetReturn>;
 type FindAllReturn = Promise<GetAllReturn>;

+export type QueryOptions = {


This type should be renamed to ByRoleOptions bacause this type of option is only relevant to byRole queries.

mdjastrzebski · 2022-04-28T21:20:28Z

+ isNodeValid(node) && matcherFn(node.props[name], matcher);
+
+ return (
+ matchesRole && !!getQueriesForElement(node).queryByText(options.name)


it looks like we should match for either queryByText(options.name) or queryByLabelText(options.name)

mdjastrzebski · 2022-04-28T21:23:08Z

 matcherFn: (prop: P, value: M) => boolean
 ) => (instance: ReactTestInstance) => {
- const getBy = (matcher: M) => {
+ const filterWithName = (


Suggested name filterByAccessibilityName

mdjastrzebski · 2022-05-04T10:28:14Z

Let's pause this PR for couple of days till we get a11y query reorganisation from #325 done. This should allow for easy separate query options type per each query, so that the name options will be allowed only for byRole queries and not all a11y queries.

kiranjd · 2022-05-05T17:49:32Z

Yes, those changes caused lot of conflicts

On Wed, 4 May 2022 at 3:58 PM, Maciej Jastrzebski ***@***.***> wrote: Let's pause this PR for couple of days till we get a11y query reorganisation from #325 <#325> done. This should allow for easy separate query options type per each query, so that the name options will be allowed only for byRole queries and not all a11y queries. — Reply to this email directly, view it on GitHub <#875 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGFANA3PQMXZBXCJ744WUCDVIJGMTANCNFSM5JHKFNKQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Kiran JD

kiranjd · 2022-07-25T13:34:20Z

@mdjastrzebski is this a good time to take this PR further? If so, should I raise a separate PR to continue the work?

AugustinLF · 2022-07-28T09:27:24Z

@kiranjd I feel like yes we should be able to start working on that once again. I'd however wait for the go from @mdjastrzebski who might have something else planned before that. Nonetheless, feel free to let me know if you need any help on that, I'm very keen in getting this one in.

mdjastrzebski · 2022-08-01T10:37:43Z

@kiranjd go ahead 👍🏻 Now is the right time to work on this PR, as we've got a11y properly refactored.

AugustinLF · 2022-08-25T13:02:43Z

@kiranjd is there anything we can do to help you to get that in? It'd bring so much value, I'm really looking to get that one in <3

kiranjd · 2022-08-25T19:32:16Z

@AugustinLF Thanks for the support. I will work on this and hopefully get this in a mergeable state by this weekend 🤞🏻

AugustinLF · 2022-09-14T11:03:47Z

@kiranjd if you're short on time to finish this PR I can definitely help fixing the last few things to get it merged, let me know if you want help :)

kiranjd · 2022-09-15T16:36:07Z

@AugustinLF Have tried to redo the changes from the last state when it was working. There seems to be a lot of refactoring undergone in the meanwhile.
I would really appreciate your help in taking this further. Thank you for understanding :)

mdjastrzebski · 2022-09-16T12:43:08Z

@kiranjd yeah RNTL codebase did move a lot since the original PR submission :-)

@AugustinLF I would be great if you could help to move this forward, as byRole query with name option has a huge potential to be one of the top recommended queries as in the case of RTL: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#not-using-byrole-most-of-the-time

mdjastrzebski · 2022-09-22T12:36:48Z

This PR has been rebased and updated by @AugustinLF as #1127. Thank you @kiranjd 👏🏻 for the initial implementation!

kiranjd mentioned this pull request Dec 2, 2021
getByRole should accept a second argument to refine query as in react-testing-library #827
Closed

fix: remove unwanted files
ea68aa1

kiranjd force-pushed the feat/by-role-second-arg branch from ca7b4c7 to ea68aa1Compare December 2, 2021 15:01

thymikee reviewed Dec 6, 2021
View reviewed changes

Comment threadsrc/helpers/a11yAPI.js Outdated

thymikee reviewed Dec 6, 2021
View reviewed changes

Comment threadsrc/helpers/makeA11yQuery.js Outdated

thymikee reviewed Dec 6, 2021
View reviewed changes

Comment threadsrc/__tests__/a11yAPI.test.js Outdated

thymikee reviewed Dec 6, 2021
View reviewed changes

Comment threadsrc/__tests__/a11yAPI.test.js Outdated

thymikee requested changes Dec 6, 2021
View reviewed changes

kiranjd added 8 commits December 7, 2021 01:14

deduplicate code for matching name
28a342c

adds tests for ...AllbyRole second arg
0713c95

update param to accept arity of 3 with waitFor as last arg
1360487

fix: remove unwanted files
f1e2c68

deduplicate code for matching name
30644cb

adds tests for ...AllbyRole second arg
cf0726f

update param to accept arity of 3 with waitFor as last arg
9c490c7

Merge branch 'feat/by-role-second-arg' ofhttps://github.com/kiranjd/…
ae3d062
…react-native-testing-library into feat/by-role-second-arg

thymikee reviewed Jan 10, 2022
View reviewed changes

Comment threadpackage.json Outdated

Update package.json
48c4e3b

AugustinLF added the compat: testing-library label Mar 28, 2022

kiranjd added 2 commits March 30, 2022 16:19

adds compulsory type for filterWithName function
949d559

Merge branch 'feat/by-role-second-arg' ofhttps://github.com/kiranjd/…
0405619
…react-native-testing-library into feat/by-role-second-arg

kiranjd commented Mar 30, 2022
View reviewed changes

support backward compatibility for findBy*
29c2d6f
- adds tests for deprecations - change second param on findBy queries

kiranjd marked this pull request as ready for review March 30, 2022 11:54

kiranjd added 2 commits March 30, 2022 19:02

Merge branch 'main' ofhttps://github.com/callstack/react-native-test…
1f725c7
…ing-library into feat/by-role-second-arg

migrate flow types to ts
a5f168c

kiranjd force-pushed the feat/by-role-second-arg branch from 291ebcc to a5f168cCompare March 30, 2022 13:35

making ts compiler happy
9ff3136

undo changes to other APIs
b6abc2f

AugustinLF requested changes Apr 4, 2022
View reviewed changes

mdjastrzebski reviewed Apr 28, 2022
View reviewed changes

AugustinLF mentioned this pull request Sep 18, 2022
Add support for ByRole with name #1127
Merged

mdjastrzebski closed this Sep 22, 2022

	constwarnDeprectedWaitForOptionsUsage=(queryOptions?: WaitForOptions)=>{
	constwarnDeprecatedWaitForOptionsUsage=(queryOptions?: WaitForOptions)=>{

Uh oh!

Conversation

kiranjd commented Dec 2, 2021• edited by thymikee Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thymikee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AugustinLF commented Mar 28, 2022

Uh oh!

kiranjd commented Mar 28, 2022

Uh oh!

kiranjdMar 30, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AugustinLFMar 31, 2022

Choose a reason for hiding this comment

Uh oh!

mdjastrzebskiApr 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AugustinLF commented Apr 25, 2022

Uh oh!

mdjastrzebskiApr 28, 2022

Choose a reason for hiding this comment

Uh oh!

mdjastrzebskiApr 28, 2022

Choose a reason for hiding this comment

Uh oh!

mdjastrzebskiApr 28, 2022

Choose a reason for hiding this comment

Uh oh!

mdjastrzebski commented May 4, 2022

Uh oh!

kiranjd commented May 5, 2022 via email

Uh oh!

kiranjd commented Jul 25, 2022

Uh oh!

AugustinLF commented Jul 28, 2022

Uh oh!

mdjastrzebski commented Aug 1, 2022

Uh oh!

AugustinLF commented Aug 25, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiranjd commented Aug 25, 2022

Uh oh!

AugustinLF commented Sep 14, 2022

Uh oh!

kiranjd commented Sep 15, 2022

Uh oh!

mdjastrzebski commented Sep 16, 2022

Uh oh!

mdjastrzebski commented Sep 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kiranjd commented Dec 2, 2021•
edited by thymikee
Loading

kiranjdMar 30, 2022•
edited
Loading

AugustinLF commented Aug 25, 2022•
edited
Loading