chore: Migrate tests to RTL (3) #7031

jpuzz0 · 2022-03-09T17:45:11Z

What: Closes #7030

patternfly-build · 2022-03-09T17:55:05Z

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

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

nicolethoen · 2022-03-23T19:43:34Z

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

-    expect(view.props().id).toBe('empty-state-icon');
-  });
-
-  test('Wrap icon in a div', () => {


why did you delete this test? is it testing the 'container' variant?

Neither of these icon tests made much sense to me. I see now that it is wrapped in a div when "icon" variant isn't specified. I can add it back with something similar.

nicolethoen · 2022-03-24T13:33:55Z

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

+  });
+
+  test('FormSelect passes value and event to onChange handler', () => {
+    const myMock = jest.fn();


The old test checked that values and events were passed as props to the mocked event handler. Can you also do that here?

These are implementation details that this test shouldn't care about, but I can add at least that the mock was called with 'mr'. There is no longer a simulated event, so I can either grab the whole real event object;

{"_reactName": "onChange", "_targetInst": null, "bubbles": true, "cancelable": false, "currentTarget": null, "defaultPrevented": false, "eventPhase": 3, "isDefaultPrevented": [Function functionThatReturnsFalse], "isPropagationStopped": [Function functionThatReturnsFalse], "isTrusted": false, "nativeEvent": {"isTrusted": false}, "target": <select aria-invalid="false" aria-label="Some label" class="pf-c-form-control" data-ouia-component-id="OUIA-Generated-FormSelect-default-8" data-ouia-component-type="PF4/FormSelect" data-ouia-safe="true"><option class="" value="mr">Mr</option></select>, "timeStamp": 1648149977372, "type": "change"}

and paste that in, or exclude it since again, this is a detail we should not be concerned about. Rather when this test is rewritten it should verify a DOM change based on the selected option.

nicolethoen · 2022-03-24T13:40:15Z

packages/react-core/src/components/FormSelect/__tests__/__snapshots__/FormSelect.test.tsx.snap

-  </select>
-</div>
-`;
+exports[`FormSelect required FormSelect input 1`] = `"<select aria-label=\\"required FormSelect\\" class=\\"pf-c-form-control\\" aria-invalid=\\"false\\" data-ouia-component-type=\\"PF4/FormSelect\\" data-ouia-safe=\\"true\\" data-ouia-component-id=\\"OUIA-Generated-FormSelect-default-7\\"><option class=\\"\\" value=\\"mr\\">Mr</option></select>"`;


I dont see anything in this snapshot (or the old snapshot, actually) that indicates the markup has changed because the required flag was set...

If it's not as simple as just updating the snapshot creation to include something that's missing, then we can address this when we update all tests, i guess.

The problem was the prop was wrong and is a perfect example of a false-positive snapshot test. The prop was meant to be isRequired having inspected the component further.

I've updated the test to verify the required attribute is present when the prop is specified instead of taking a snapshot.

nicolethoen · 2022-03-24T13:55:52Z

packages/react-core/src/components/LabelGroup/__tests__/__snapshots__/LabelGroup.test.tsx.snap

-  </div>
-</div>
-`;
+exports[`LabelGroup label group with category and tooltip 1`] = `"<ul class=\\"pf-c-label-group__list\\" aria-labelledby=\\"pf-random-id-4\\" role=\\"list\\" data-testid=\\"label-group-test-id\\"><li class=\\"pf-c-label-group__list-item\\"><span class=\\"pf-c-label\\"><span class=\\"pf-c-label__content\\">1.1</span></span></li></ul>"`;


neither the old nor the new snapshot have a tooltip :/ maybe we should update these tests with a comment so we can be sure to readdress them?

I'll add a comment.

nicolethoen · 2022-03-24T13:57:21Z

packages/react-core/src/components/LabelGroup/__tests__/__snapshots__/LabelGroup.test.tsx.snap

-  </div>
-</div>
-`;
+exports[`LabelGroup label group with category 1`] = `"<ul class=\\"pf-c-label-group__list\\" aria-labelledby=\\"pf-random-id-1\\" role=\\"list\\" data-testid=\\"label-group-test-id\\"><li class=\\"pf-c-label-group__list-item\\"><span class=\\"pf-c-label\\"><span class=\\"pf-c-label__content\\">1.1</span></span></li></ul>"`;


The category is missing from this snapshot

nicolethoen · 2022-03-24T17:36:41Z

packages/react-core/src/components/LabelGroup/__tests__/__snapshots__/LabelGroup.test.tsx.snap

-  </div>
-</div>
-`;
+exports[`LabelGroup label group with closable category 1`] = `"<ul class=\\"pf-c-label-group__list\\" aria-labelledby=\\"pf-random-id-2\\" role=\\"list\\" data-testid=\\"label-group-test-id\\"><li class=\\"pf-c-label-group__list-item\\"><span class=\\"pf-c-label\\"><span class=\\"pf-c-label__content\\">1.1</span></span></li></ul>"`;


the category is missing from this snapshot

thatblindgeye · 2022-03-23T19:52:17Z

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

+    render(<EmptyStateBody data-testid="body-test-id" />);
+    expect(screen.getByTestId('body-test-id').className).toContain('pf-c-empty-state__body');


Would it be worth checking that passing in a custom class works here like the original test?

Yeah I agree with that, I can add that back in.

thatblindgeye · 2022-03-23T19:53:52Z

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

+    render(<EmptyStateSecondaryActions data-testid="actions-test-id" />);
+    expect(screen.getByTestId('actions-test-id').className).toContain('pf-c-empty-state__secondary');


Same as above re: passing in a class name

thatblindgeye · 2022-03-24T12:27:49Z

packages/react-core/src/components/Form/__tests__/FormGroup.test.tsx

  test('should render form group required variant', () => {
-    const view = render(
-      <FormGroup isRequired label="label" fieldId="id">
+    render(
+      <FormGroup isRequired label="label" fieldId="id" data-testid="form-group-test-id">
        <input id="id" />
      </FormGroup>
    );
-    expect(view.container).toMatchSnapshot();
+    expect(screen.getByTestId('form-group-test-id').outerHTML).toMatchSnapshot();


I believE this test and the test on line 45 are testing for the same thing. If nothing else this comment can serve as a note as to what to update in the future after all of these PRs are merged in.

Yeah, it's the same. I'll remove this one.

thatblindgeye · 2022-03-24T13:12:03Z

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

+  test('validated error FormSelect input', () => {
+    render(
+      <FormSelect validated={ValidatedOptions.error} aria-label="validated FormSelect">
+        <FormSelectOption key={1} value={props.options[1].value} label={props.options[1].label} />
+      </FormSelect>
+    );
+    expect(screen.getByLabelText('validated FormSelect').outerHTML).toMatchSnapshot();
+  });


Looks like this and the Invalid FormSelect Input test are essentially testing the same thing. Same as another comment above that this could be a note for future update if not removed in this PR.

thatblindgeye · 2022-03-24T18:28:36Z

...es/react-core/src/components/LoginPage/__tests__/__snapshots__/LoginFooterItem.test.tsx.snap

-  </div>
-</div>
-`;
+exports[`LoginFooterItem className is added to the root element 1`] = `"<a target=\\"_blank\\" href=\\"#\\"></a>"`;


The class isn't being added here, though might be similar to some comments Nicole made about worth noting to readdress in the future.

The component doesn't pass it in is why. I'm going to update the component for this since the prop exists, I assume it shouldn't omit the className as it's doing here:

nicolethoen · 2022-03-24T18:38:05Z

...es/react-core/src/components/LoginPage/__tests__/__snapshots__/LoginFooterItem.test.tsx.snap

-  </div>
-</div>
-`;
+exports[`LoginFooterItem className is added to the root element 1`] = `"<a target=\\"_blank\\" href=\\"#\\"></a>"`;


all these snapshots are missing the root element

nicolethoen · 2022-03-24T18:40:20Z

...-core/src/components/LoginPage/__tests__/__snapshots__/LoginMainFooterBandItem.test.tsx.snap

-  />
-</div>
-`;
+exports[`LoginMainFooterBandItem with custom node 1`] = `"<div>My custom node</div>"`;


this is missing the root element.

nicolethoen · 2022-03-24T18:45:08Z

packages/react-core/src/components/Form/__tests__/__snapshots__/FormGroup.test.tsx.snap

-  </div>
-</div>
-`;
+exports[`FormGroup should render form group validated error variant 1`] = `"<div data-testid=\\"form-group-test-id\\" class=\\"pf-c-form__group\\"><div class=\\"pf-c-form__group-label\\"><label class=\\"pf-c-form__label\\" for=\\"label-id\\"><span class=\\"pf-c-form__label-text\\">label</span></label> </div><div class=\\"pf-c-form__group-control\\"><input id=\\"id\\"></div></div>"`;


This snapshot contains nothing indicating that it's an error variant

nicolethoen · 2022-03-24T18:49:30Z

packages/react-core/src/components/Form/__tests__/__snapshots__/FormGroup.test.tsx.snap

-  </div>
-</div>
-`;
+exports[`FormGroup should render form group variant with function helperText 1`] = `"<div data-testid=\\"form-group-test-id\\" class=\\"pf-c-form__group\\"><div class=\\"pf-c-form__group-label\\"><label class=\\"pf-c-form__label\\" for=\\"label-id\\"><span class=\\"pf-c-form__label-text\\">Label</span></label> </div><div class=\\"pf-c-form__group-control\\"><input id=\\"label-id\\"><div>label</div></div></div>"`;


I dont see any helpertext in this snapshot

or in the old one :/

I'm seeing it actually, since helperText is just a div with the text label, you'll need to look for that.

nicolethoen · 2022-03-24T18:50:42Z

packages/react-core/src/components/Form/__tests__/__snapshots__/FormGroup.test.tsx.snap

-  </div>
-</div>
-`;
+exports[`FormGroup should render form group variant with function label 1`] = `"<div data-testid=\\"form-group-test-id\\" class=\\"pf-c-form__group\\"><div class=\\"pf-c-form__group-label\\"><label class=\\"pf-c-form__label\\" for=\\"id\\"><span class=\\"pf-c-form__label-text\\"><div>label</div></span></label> </div><div class=\\"pf-c-form__group-control\\"><input aria-label=\\"input\\"></div></div>"`;


again, not seeing the .pf-c-form__helper-text. Maybe i'm looking for the wrong thing?

Same as the other test with the return function, it's just a div with the text label, which does exist in the snapshot.

nicolethoen · 2022-03-24T18:50:54Z

packages/react-core/src/components/Form/__tests__/__snapshots__/FormGroup.test.tsx.snap

-  </div>
-</div>
-`;
+exports[`FormGroup should render form group variant with node helperText 1`] = `"<div data-testid=\\"form-group-test-id\\" class=\\"pf-c-form__group\\"><div class=\\"pf-c-form__group-label\\"><label class=\\"pf-c-form__label\\" for=\\"label-id\\"><span class=\\"pf-c-form__label-text\\">Label</span></label> </div><div class=\\"pf-c-form__group-control\\"><input id=\\"label-id\\"><h1>Header</h1></div></div>"`;


again no .pf-c-form__helper-text

This time I do see the helperText, where in this test the prop is a div with the text "Header" which does exist here.

jpuzz0 force-pushed the chore/rtl-migrate-bundle-3 branch 2 times, most recently from 9cf40e8 to 4ef4ca3 Compare March 10, 2022 17:28

thatblindgeye self-requested a review March 21, 2022 19:01

chore: migrate tests to rtl (3)

236c28c

jpuzz0 force-pushed the chore/rtl-migrate-bundle-3 branch from 4ef4ca3 to 236c28c Compare March 23, 2022 14:02

nicolethoen self-requested a review March 23, 2022 19:38

kmcfaul approved these changes Mar 24, 2022

View reviewed changes

nicolethoen requested changes Mar 24, 2022

View reviewed changes

thatblindgeye requested changes Mar 24, 2022

View reviewed changes

nicolethoen requested changes Mar 24, 2022

View reviewed changes

address feedback

3378e69

thatblindgeye approved these changes Mar 25, 2022

View reviewed changes

nicolethoen approved these changes Mar 26, 2022

View reviewed changes

tlabaj merged commit 72f1977 into patternfly:main Mar 29, 2022

		render(<EmptyStateBody data-testid="body-test-id" />);
		expect(screen.getByTestId('body-test-id').className).toContain('pf-c-empty-state__body');

		render(<EmptyStateSecondaryActions data-testid="actions-test-id" />);
		expect(screen.getByTestId('actions-test-id').className).toContain('pf-c-empty-state__secondary');

chore: Migrate tests to RTL (3) #7031

chore: Migrate tests to RTL (3) #7031

Uh oh!

Conversation

jpuzz0 commented Mar 9, 2022

Uh oh!

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

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

jpuzz0 Mar 24, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

patternfly-build commented Mar 9, 2022 •

edited

Loading

jpuzz0 Mar 24, 2022 •

edited

Loading