fix: toHaveProp func to work with @testing-library/react-native latest version (7.0.2) #36

yonatan-altaraz · 2020-09-03T12:21:09Z

What:

when using the latest @testing-library/react-native version, the function toHaveProp(attr: string, value?: any) doesn't work properly when getting the element by text, it checks for the prop existence but does not compare to the expected value, when given, and returns 'pass' even if the expected value is wrong.

Why:

to be able to work with this function using the new version of @testing-library/react-native

How:

just changed a couple of row in to-have-props.js

Checklist:

Documentation added to the
docs (N/A)
Typescript definitions updated (N/A)
Tests
Ready to be merged

to recreate the issue, you can checkout this branch

codecov · 2020-09-03T12:22:44Z

Codecov Report

Merging #36 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master       #36   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines           95        94    -1     
  Branches        27        27           
=========================================
- Hits            95        94    -1

Impacted Files	Coverage Δ
src/to-have-prop.js	`100.00% <ø> (ø)`
src/utils.js	`100.00% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb02b6a...d3bebde. Read the comment docs.

mdjastrzebski

Not sure about the fact that composite components with specific displayName are allowed matched by this matcher.

mdjastrzebski · 2020-09-04T20:06:23Z

src/to-have-prop.js

@@ -18,7 +18,8 @@ export function toHaveProp(element, name, expectedValue) {
  const prop = element.props[name];

  const isDefined = expectedValue !== undefined;
-  const isAllowed = VALID_ELEMENTS.includes(element.type);
+  const elementType = typeof element.type == 'string' ? element.type : element.type?.displayName;
+  const isAllowed = VALID_ELEMENTS.includes(elementType);


I'm wondering whether we should have a list of allowed components to check their props. IMO we should be able to check props of any host component. @thymikee, @brunohkbx, wdyt?

Same. A list of valid elements is a remnant of the previous lib implementation

The current impl. seems a bit weird here, as reading the code bellow:

pass: isDefined && isAllowed ? hasProp && equals(prop, expectedValue) : hasProp,

Is the mere prop existence is checked in case either expected value is not defined (which sound fine) and also when element is not on VALID_ELEMENTS (which sounds kind of strange).

As I see it, the desired behavior is as follows:

For all host elements:

if provided expectedValue check for prop existence and that it's value matches expectedValue

if not provided expectedValue: just check if prop exists.

For composite elements:

as all RNTL queries already return only host elements, then the dev user had to use some escape hatch like .parent, .children to get hold of that kind of element,

IMO I would allow user to use this check on such elements, because: 1) it's hard to write such test by mistake, so they are rather written deliberately, 2) it reduces the "surprise factor" for the user, 3) it simplifies our codebase.

@mdjastrzebski yeah I completely agree.

mdjastrzebski · 2020-09-04T20:10:20Z

src/__tests__/to-have-prop.js

+  expect(() =>
+    expect(getByText('text')).toHaveProp('allowFontScaling', 'wrongValue'),
+  ).toThrowError();


As far as I don't prefer {get|query}ByTestId queries, it seems that for consistency and readability it would be better to use it here.

it's just for the example, any reason it should behave this way if using getByText here?

getByText('text') vs queryByTestId('text') is just a stylistic issue here, as for current setup, both elements return exactly the same element.

Optimally, we should be using the same queries, as we encourage our users (ByText, ByHintText, ByRole, etc) everywhere in our tests when possible. But since surrounding code uses queryByTestId, the second best solution seems to be consistent with surrounding code to reduce cognitive load for reading user.

Be aware that this is a bit nitpicking :-)

i understand, in the current setup it's correct, the returned element of both accessors is the same,
but this fix is for an issue that pops only when using the 7.0.2 version of testing-library/react-native, in which the returned elements are not the same, so only the getByTest func will fail the test (without the fix). that was the logic behind using getByTest.
but no problem to change it to queryByTestId, tell me what you think is best

mdjastrzebski · 2020-09-04T20:13:24Z

src/to-have-prop.js

@@ -18,7 +18,8 @@ export function toHaveProp(element, name, expectedValue) {
  const prop = element.props[name];

  const isDefined = expectedValue !== undefined;
-  const isAllowed = VALID_ELEMENTS.includes(element.type);
+  const elementType = typeof element.type == 'string' ? element.type : element.type?.displayName;


This looks a bit strange, as it will allow any composite component that sets it's displayName to a value from VALID_ELEMENTS list, to have it's props check. AFAIK we want to discourage interaction with composite components in favor of interacting only with host components.
@thymikee, @brunohkbx, wdyt?

Yup, I got the same impression, we shouldn't do that.

can you explain the logic behind not checking the value of a prop for composite components (or any component)?

So the reason why React Native Testing Library ignores composite components, and is only concerned with host components, is that it tries to resemble what user (or manual tester) would see when interacting with the app. This is similar to React Testing Library, when user interacts with rendered HTML and not composite components.

That said, RNTL already filters out composite components in all queries (getBy, queryBy, findBy), so we probably can ignore checking for host/composite component in toHaveProp matchers, and perform prop check on both types. This should not compromise our goal of making it easy to write good test, and hard to write bad tests.

Additionally, this will reduce the "suprise factor" for users trying to validate props on compisite components that they got from using parent or children props for navigation.

@thymikee, what's your take on that?

I'm good with not validating that here. It's the library that's responsible for filtering.

did i understand you correctly that the change should be removing the whole isAllowed and VALID_ELEMENTS
validation?

…/khealth/jest-native into toHaveProp_not_comparing_val_FIX

yonatan-altaraz · 2020-09-16T13:29:00Z

@mdjastrzebski, I'v changed the code according to what i understand from your comments, i hope i got it right.
please have another look.

mdjastrzebski

@yonatan-altaraz I think that the code looks good now. Great thanks for your contribution!

Please updated the docs (README.md) to remove the mention that toHaveProp checks only native elements (= host elements), as after this change this is no longer the case.

mdjastrzebski · 2020-09-21T14:18:05Z

README.md

@@ -215,8 +215,7 @@ expect(parent).not.toContainElement(grandparent);
 toHaveProp(prop: string, value?: any);
 ```

-Check that an element has a given prop. Only works for native elements, so this is similar to
-checking for attributes in the DOM.
+Check that an element has a given prop. this is similar to checking for attributes in the DOM.


Suggested change

Check that an element has a given prop. this is similar to checking for attributes in the DOM.

Check that an element has a given prop.

bcarroll22 · 2020-09-28T15:55:09Z

🎉 This PR is included in version 3.4.3 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

yonatan-altaraz added 2 commits September 2, 2020 19:14

fix

dd855f0

test

9c2d261

yonatan-altaraz changed the title ~~fix~~ fix toHaveProp func to work with @testing-library/react-native latest version (7.0.2) Sep 3, 2020

yonatan-altaraz mentioned this pull request Sep 3, 2020

toHaveProp does't work properly with @testing-library/react-native: 7.0.2 (latest) #37

Closed

thymikee requested a review from mdjastrzebski September 3, 2020 13:22

mdjastrzebski reviewed Sep 4, 2020

View reviewed changes

yonatan-altaraz and others added 5 commits September 16, 2020 15:58

remove isAllowed and VALID_ELEMENTS from toHaveProp

c31152a

fix test

b161a11

Merge branch 'master' into toHaveProp_not_comparing_val_FIX

94d64f9

delete VALID_ELEMENTS import

93298c3

Merge branch 'toHaveProp_not_comparing_val_FIX' of https://github.com…

9c7de00

…/khealth/jest-native into toHaveProp_not_comparing_val_FIX

yonatan-altaraz changed the title ~~fix toHaveProp func to work with @testing-library/react-native latest version (7.0.2)~~ fix: toHaveProp func to work with @testing-library/react-native latest version (7.0.2) Sep 16, 2020

yonatan-altaraz requested review from thymikee, mdjastrzebski and brunohkbx September 16, 2020 13:29

remove redundant getByText

99af638

mdjastrzebski reviewed Sep 20, 2020

View reviewed changes

change readme

9fdff20

mdjastrzebski reviewed Sep 21, 2020

View reviewed changes

change readme again

d3bebde

mdjastrzebski approved these changes Sep 22, 2020

View reviewed changes

brunohkbx approved these changes Sep 23, 2020

View reviewed changes

brunohkbx mentioned this pull request Sep 23, 2020

.toBeDisabled() buggy with RN 0.62 #23

Closed

thymikee merged commit 29756b4 into testing-library:master Sep 28, 2020

bcarroll22 added the released label Sep 28, 2020

brunohkbx mentioned this pull request Oct 12, 2020

toHaveProp not working on Button #27

Closed

dependabot bot mentioned this pull request Mar 17, 2021

Bump @testing-library/jest-native from 3.1.0 to 4.0.1 newerton/gostack-template-conceitos-react-native#18

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: toHaveProp func to work with @testing-library/react-native latest version (7.0.2) #36

fix: toHaveProp func to work with @testing-library/react-native latest version (7.0.2) #36

yonatan-altaraz commented Sep 3, 2020 •

edited by mdjastrzebski

codecov bot commented Sep 3, 2020 •

edited

mdjastrzebski left a comment

mdjastrzebski Sep 4, 2020

thymikee Sep 7, 2020

mdjastrzebski Sep 7, 2020

brunohkbx Sep 7, 2020

mdjastrzebski Sep 4, 2020 •

edited

yonatan-altaraz Sep 6, 2020

mdjastrzebski Sep 7, 2020

yonatan-altaraz Sep 10, 2020

mdjastrzebski Sep 4, 2020

thymikee Sep 7, 2020

yonatan-altaraz Sep 8, 2020

mdjastrzebski Sep 8, 2020

thymikee Sep 8, 2020

yonatan-altaraz Sep 10, 2020

yonatan-altaraz commented Sep 16, 2020

mdjastrzebski left a comment

mdjastrzebski Sep 21, 2020

bcarroll22 commented Sep 28, 2020

	Check that an element has a given prop. this is similar to checking for attributes in the DOM.
	Check that an element has a given prop.

fix: toHaveProp func to work with @testing-library/react-native latest version (7.0.2) #36

fix: toHaveProp func to work with @testing-library/react-native latest version (7.0.2) #36

Conversation

yonatan-altaraz commented Sep 3, 2020 • edited by mdjastrzebski

codecov bot commented Sep 3, 2020 • edited

Codecov Report

mdjastrzebski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjastrzebski Sep 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonatan-altaraz commented Sep 16, 2020

mdjastrzebski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcarroll22 commented Sep 28, 2020

yonatan-altaraz commented Sep 3, 2020 •

edited by mdjastrzebski

codecov bot commented Sep 3, 2020 •

edited

mdjastrzebski Sep 4, 2020 •

edited