chore(BackToTop) update tests to new standarts by dominik-petrik · Pull Request #7833 · patternfly/patternfly-react

dominik-petrik · 2022-08-16T10:55:54Z

What: Closes #7813

Additional issues: There seems to be an issue with toBeVisible(). When component visibility is controlled by css property, toBeVisible reports the component as visible even though it has css property that should hide the component (eg. display: none)

Probably a JSDOM limitation. Actually, the problem is that the css isn't actually loaded into the document in JSDOM. If it were, then that would work. If you can come up with a custom jest transform that could insert the css file into the document during tests, then you'd be set.

I've worked around that by checking for hidden class instead("pf-m-hidden").

patternfly-build · 2022-08-16T11:06:14Z

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

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

nicolethoen · 2022-08-18T20:11:49Z

@wise-king-sullyman
I'm trying to learn and absorb as much as I can by watching you review these test revamp PRs.
In his case, I see @dominik-petrik is using getByTestId('backtotop') to query for the test results. In the case where the rendered component is a role-less div is this the best way to query? Or is there a better alternative?

nicolethoen · 2022-08-18T20:12:54Z

In the case of the BackToTop component, there is a button inside the component, is that something we could/should be querying by?

wise-king-sullyman · 2022-08-18T20:39:07Z

@nicolethoen IMO yeah for anything that can be ascertained by grabbing the button with a getByRole query that would definitely be preferable.

For things that we need to grab the containing div for it's a bit more up to debate. I think I would rather see us grab the button, then reach up from it and grab its parent rather than getting the div directly using a test id here, but that wouldn't be universally true.

wise-king-sullyman

This is a strong first swing at this! I do have a few requests for additional tests and like @nicolethoen would like to see some tweaks to the queries being used though.

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

thatblindgeye

Awesome work on this! Other than Austn's comment, only had one comment below

thatblindgeye · 2022-08-22T13:54:35Z

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

+  expect(screen.getByTestId('backtotop').firstChild).toBeVisible();
+  expect(screen.getByTestId('backtotop').firstChild).not.toHaveClass('pf-m-hidden');


Since the earlier tests are just checking for the pf-m-hidden class, could we remove the toBeVisible line here?

wise-king-sullyman

I have a few nits that I would like to see addressed, but none are true blockers so I'm happy to approve 🙂

Really awesome work on this!

wise-king-sullyman · 2022-09-02T20:27:53Z

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

+      <p>{variant}</p>
+      <p>{iconPosition}</p>


Nit: can you update these to have some additional text which you include in your getByText query to ensure that we don't get false positive because of text elsewhere in the component?

Suggested change

{variant}

{iconPosition}

Variant: {variant}

Icon position: {iconPosition}

wise-king-sullyman · 2022-09-02T20:32:42Z

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

+  expect(screen.getByRole(`button`).parentElement).toHaveClass('pf-m-hidden');
+});
+
+test('Renders BackToTop when isAlwaysVisible prop is set', () => {


Nit: can you adjust this test name since it isn't really testing that BackToTop renders, but rather that it renders without the hidden class?

wise-king-sullyman · 2022-09-02T20:48:00Z

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

+test('Passes correct text content to button child component', () => {
+  render(<BackToTop title="Back to the future" />);
+
+  expect(screen.getByText('Back to the future')).toBeVisible();


Nit: here I would rather see us use the getByRole query to select the button and assert that it's text is what we expect using the toHaveTextContent matcher.

wise-king-sullyman · 2022-09-02T21:12:09Z

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

+test('Passes correct icon to button child component', () => {
+  render(<BackToTop />);
+
+  expect(screen.getByTestId('icon').firstChild).toBeTruthy();
+});


Nit: for the icon I would rather see us do something like what I did for the angle right icon in the accordion toggle where we mock it out (since the SVG isn't actually a responsibility of BackToTop), and then I would just assert that the icon div contains the text you pass in for the mock so that we know it's being sent to Button properly.

* chore(BackToTop) update tests to new standarts * add more tests * add a few more tests, update queries * add more tests, mocking in progress * mock button child element, test props passed to it * update user-event calls

dominik-petrik requested a review from wise-king-sullyman August 16, 2022 10:56

dominik-petrik marked this pull request as draft August 17, 2022 12:59

dominik-petrik force-pushed the backToTop branch from ba6cdad to f615544 Compare August 17, 2022 13:28

dominik-petrik marked this pull request as ready for review August 17, 2022 14:11

nicolethoen approved these changes Aug 18, 2022

View reviewed changes

gitdallas approved these changes Aug 18, 2022

View reviewed changes

wise-king-sullyman requested changes Aug 18, 2022

View reviewed changes

thatblindgeye reviewed Aug 22, 2022

View reviewed changes

dominik-petrik force-pushed the backToTop branch from f615544 to 234ba4c Compare August 30, 2022 11:57

dominik-petrik marked this pull request as draft August 30, 2022 11:58

dominik-petrik added 6 commits September 2, 2022 19:39

chore(BackToTop) update tests to new standarts

3b66004

add more tests

cfbd193

add a few more tests, update queries

932787d

add more tests, mocking in progress

ac92d8e

mock button child element, test props passed to it

5617229

update user-event calls

c58e76f

dominik-petrik force-pushed the backToTop branch from 30fb1eb to c58e76f Compare September 2, 2022 17:55

dominik-petrik marked this pull request as ready for review September 2, 2022 18:16

dominik-petrik requested review from thatblindgeye and wise-king-sullyman September 2, 2022 18:17

wise-king-sullyman approved these changes Sep 2, 2022

View reviewed changes

thatblindgeye approved these changes Sep 6, 2022

View reviewed changes

tlabaj approved these changes Sep 7, 2022

View reviewed changes

tlabaj merged commit af109db into patternfly:main Sep 7, 2022

		expect(screen.getByTestId('backtotop').firstChild).toBeVisible();
		expect(screen.getByTestId('backtotop').firstChild).not.toHaveClass('pf-m-hidden');

Conversation

dominik-petrik commented Aug 16, 2022

Uh oh!

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

Uh oh!

Uh oh!

nicolethoen commented Aug 18, 2022

Uh oh!

nicolethoen commented Aug 18, 2022

Uh oh!

wise-king-sullyman commented Aug 18, 2022

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

patternfly-build commented Aug 16, 2022 •

edited

Loading