chore(TextInputGroup): update testing suite to new standards by wise-king-sullyman · Pull Request #7169 · patternfly/patternfly-react

wise-king-sullyman · 2022-04-01T19:04:28Z

What: Closes #6929

Additional issues:

patternfly-build · 2022-04-01T19:14:13Z

Preview: https://patternfly-react-pr-7169.surge.sh

A11y report: https://patternfly-react-pr-7169-a11y.surge.sh

packages/react-core/src/components/TextInputGroup/__tests__/TextInputGroup.test.tsx

jpuzz0 · 2022-04-07T22:08:23Z

packages/react-core/src/components/TextInputGroup/__tests__/TextInputGroupMain.test.tsx

+    expect(input).toHaveClass('pf-c-text-input-group__text-input');
+  });
+
+  it('renders given input icon props', () => {


I think this is more-so "renders the icon" (or something like this) rather than this which reads to me essentially, "the text input group still renders when passing an icon prop". Maybe also mention that it is expected to be hidden.

jpuzz0 · 2022-04-07T22:11:00Z

packages/react-core/src/components/TextInputGroup/__tests__/TextInputGroupMain.test.tsx

+    expect(input).toHaveAccessibleName('Test label');
+  });
+
+  it('passes the value prop to the input', () => {


Can we not instead visibly see the value passed to the input or is that hidden from users?

You can't grab it or the placeholder with byRole/byText, but after some further research I did find that RTL provides a matcher specifically for input values/placeholders! Great callout!

jpuzz0 · 2022-04-07T22:12:30Z

packages/react-core/src/components/TextInputGroup/__tests__/TextInputGroupMain.test.tsx

+    expect(input).toHaveAttribute('value', 'value text');
+  });
+
+  it('passes the placeholder prop to the input', () => {


same as my other comment. If we can verify visibility of the placeholder, that is more meaningful than verifying an attribute exists and makes it so we don't have to test the attribute value IMO.

packages/react-core/src/components/TextInputGroup/__tests__/TextInputGroupMain.test.tsx

jpuzz0 · 2022-04-07T22:18:51Z

packages/react-core/src/components/TextInputGroup/__tests__/TextInputGroupMain.test.tsx

+
+    const input = screen.getByRole('textbox');
+    userEvent.click(input);
+    userEvent.click(document.body);


This is a good idea. I haven't thought to focus out by simply clicking the document body before.

jpuzz0 · 2022-04-07T22:19:08Z

packages/react-core/src/components/TextInputGroup/__tests__/TextInputGroupMain.test.tsx

+
+    const input = screen.getByRole('textbox');
+
+    expect(input).toHaveAccessibleName('Test label');


Very cool. Haven't used this before.

thatblindgeye

Other than Jeff's comments, the only thing I could think of when looking this over was whether the test that explicitly checks for the default class should be included when the snapshots already have that class to test against.

As you mentioned in our conversation, it was decided in an RTL meeting that testing explicitly for classes should be done. I can see merit in doing so, especially if we don't want snapshots necessarily being used to test that attributes have a specific value, so I'm not willing to block over that. Mentioning it in case anyone else may have an opinion either way after seeing this in action.

thatblindgeye · 2022-04-11T12:38:14Z

packages/react-core/src/components/TextInputGroup/__tests__/TextInputGroup.test.tsx

+    /** the below expectation is to verify that that the component isn't always rendering with the text used to verify
+    child rendering in the next test */
+    expect(screen.queryByText('Test')).not.toBeInTheDocument();


For this, would checking that the children length is 0 (or something similar) be viable for this test instead?

I can get why you would want that, since this expectation only makes sense with the context of the following test, but I feel like checking the length of the children property isn't a very "RTL" approach. @jeffpuzzo thoughts?

I don't think lines 15 through 17 should be included at all since you're already verifying the parent component is visible without children which is the objective of the test, correct?

I partially agree?

15-17 aren't strictly necessary to verify that it renders without children, but in the same way that the next test verifies chilldren are rendered (by testing that when "Test" is passed as a child it is rendered to the page), this ensures that the inverse is true (by testing that when "Test" isn't passed it isn't rendered to the page). Essentially it protects from flaws in the test suite, like the component potentially always rendering "Test".

I figure that it goes along with the other aspect of testing that it renders without children for the same reason that checking for "Test" is valid for checking that children are rendered. If you disagree though and think it's either unnecessary or should be a separate test I'm not super committed to it.

I do disagree. We're specifically rendering this component with no children (aka a self-closing tag). All we care about is that the component still renders.

Testing that text we're purposefully not including in render doesn't exist doesn't make sense to me, as much as it might appear more-so as an inverse of the other test. If someone updates the test to include "Test" as a child after the fact for some reason, then we have bigger problems.

I think my concern is that what it's actually testing for doesn't exactly fit with what the intention is. I'd say comparing to a snapshot of the component rendered without children might be better, but that's less an option if we only want the single snapshot. EDIT: beaten to it by Jeff

👍 I can understand that/those viewpoints. I'll update shortly.

jpuzz0

Looks great!

tlabaj · 2022-04-13T19:35:04Z

you just need to resolve conflicts and we can merge.

patternfly-build · 2022-04-13T21:51:18Z

Your changes have been released in:

eslint-plugin-patternfly-react@4.41.35
@patternfly/react-catalog-view-extension@4.53.35
@patternfly/react-charts@6.55.35
@patternfly/react-code-editor@4.43.35
@patternfly/react-console@4.53.35
@patternfly/react-core@4.202.35
@patternfly/react-docs@5.63.35
@patternfly/react-icons@4.53.35
@patternfly/react-inline-edit-extension@4.47.35
demo-app-ts@4.162.35
@patternfly/react-integration@4.164.35
@patternfly/react-log-viewer@4.47.35
@patternfly/react-styles@4.52.35
@patternfly/react-table@4.71.35
@patternfly/react-tokens@4.54.35
@patternfly/react-topology@4.49.35
@patternfly/react-virtualized-extension@4.49.35
transformer-cjs-imports@4.40.35

Thanks for your contribution! 🎉

wise-king-sullyman force-pushed the convert-tig-tests-to-rtl branch from 3127f68 to d1f1af7 Compare April 1, 2022 19:17

wise-king-sullyman requested a review from jpuzz0 April 1, 2022 20:15

jpuzz0 reviewed Apr 7, 2022

View reviewed changes

thatblindgeye self-requested a review April 8, 2022 16:22

thatblindgeye reviewed Apr 8, 2022

View reviewed changes

thatblindgeye reviewed Apr 11, 2022

View reviewed changes

kmcfaul approved these changes Apr 11, 2022

View reviewed changes

wise-king-sullyman requested review from jpuzz0 and thatblindgeye April 11, 2022 20:47

jpuzz0 approved these changes Apr 11, 2022

View reviewed changes

thatblindgeye approved these changes Apr 12, 2022

View reviewed changes

tlabaj approved these changes Apr 13, 2022

View reviewed changes

chore(TextInputGroup): update testing suite to new standards

8a260af

wise-king-sullyman force-pushed the convert-tig-tests-to-rtl branch from fa32de7 to 8a260af Compare April 13, 2022 20:18

update failing snapshot

7509348

tlabaj merged commit ba6cf3a into patternfly:main Apr 13, 2022

wise-king-sullyman deleted the convert-tig-tests-to-rtl branch April 14, 2022 13:14

wise-king-sullyman mentioned this pull request Jun 21, 2022

chore: update component unit tests to meet new standards with RTL #7594

Open

87 tasks


		const input = screen.getByRole('textbox');

		expect(input).toHaveAccessibleName('Test label');

Conversation

wise-king-sullyman commented Apr 1, 2022

Uh oh!

patternfly-build commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Apr 11, 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!

Choose a reason for hiding this comment

Uh oh!

jpuzz0 left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj commented Apr 13, 2022

Uh oh!

patternfly-build commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

patternfly-build commented Apr 1, 2022 •

edited

Loading

thatblindgeye Apr 11, 2022 •

edited

Loading