Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Migrate Enzyme tests to React Testing Library #5299

Merged
merged 34 commits into from
Oct 15, 2020
Merged

Conversation

jeffstieler
Copy link
Contributor

This PR migrates all remaining Enzyme tests to React Testing Library.

Where reasonable, it also changes the test behavior to check user facing functionality rather than component props or specific HTML elements.

Heads up to @c-shultz @jconroy @allendav - this PR changes test code from your teams. Please have someone review!

Detailed test instructions:

  • Verify the JS tests pass
  • Review the new test code

Changelog Note:

Dev: Remove Enzyme in favor of React Testing Library

@jeffstieler jeffstieler added [Status] Needs Review tool: monorepo infrastructure type: technical debt This issue/PR represents/solves the technical debt of the project. labels Oct 8, 2020
@jeffstieler jeffstieler requested a review from a team October 8, 2020 15:11
Copy link
Contributor

@vbelolapotkov vbelolapotkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done @jeffstieler ! It looks great and I haven't noticed anything critical 🎉

Suggested alternative patterns in some places. They are not mandatory but IMO lead to better readability and tests output in case of failure. Also note, I've suggested most of alternative patterns in a single place but noticed they might be applied in many other places.

Hope that helps 🙂

Comment on lines 71 to 73
expect(
queryByText( 'No data recorded for the selected time period.' )
).not.toBeNull();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of things here:

  1. It's better use get query as it gives better output in case of error. More guides on queries are here.
  2. There is toBeInTheDocument matcher which makes more sense. This one is general and applies to many places.

expect( firstRow[ 2 ].value ).toBe( mockData[ 0 ].orders_count );
expect( firstRow[ 3 ].value ).toBe(
formatDecimal( mockData[ 0 ].net_revenue )
const leaderboard = container.querySelector(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with context and not sure how is important to check specific values, but we could replace the whole checks below with snapshot, expect ( container ).toMatchSnapshot(). What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my first pass through these tests I was hesitant to change their behavior too much. That said, introducing snapshot testing where we are just checking markup seems perfectly reasonable. Thanks for the nudge!

Comment on lines -28 to -41
test( 'should set the time-comparison mode prop by default', () => {
const reportChart = mount(
<ReportChart
path={ path }
primaryData={ data }
query={ {} }
secondaryData={ data }
selectedChart={ selectedChart }
/>
);
const chart = reportChart.find( 'Chart' );

expect( chart.props().mode ).toEqual( 'time-comparison' );
} );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't change DOM but only internal state of the component, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that DOM changes would result from this test, but they would be in the nested <Chart> component, not the <ReportChart> that is under test here.

I removed this test case because it felt really low value. It only covers the default case here:

const chartMode =
props.mode ||
getChartMode( selectedFilter, query ) ||
'time-comparison';

expect( summaryNumber.props().prevValue ).toBe( '500.25' );
expect( summaryNumber.props().delta ).toBe( 100 );
} );
expect( screen.getByText( '1,000.5' ) ).not.toBeEmptyDOMElement();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this check looks unclear to me, but I can miss some details here.
It's recommended to use query* selector for such cases, because get should throw if element/text is not found.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think replacing .not.toBeEmptyDOMElement() with .toBeInTheDocument() will make this test more clear. getBy* was chosen because the expectation is that the text can be found - the test should fail if it cannot be.

Comment on lines +50 to +52
expect(
container.getElementsByClassName( 'components-spinner' )
).toHaveLength( 0 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in other places for better readability:

Suggested change
expect(
container.getElementsByClassName( 'components-spinner' )
).toHaveLength( 0 );
expect(
container.querySelector( '.components-spinner' )
).not.toBeInTheDocument();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of:

expect(
	container.querySelector( '.components-spinner' )
).toBeNull();

querySelector() returns null when the selector can't be found.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will work, but iirc the output is more helpful when not.toBeInTheDocument() is used. I'd suggest breaking the test and compare the output. It's fine to go with any option you like more :)

Comment on lines +18 to +23
const { getByText } = render( <SetupNotice isSetupError={ true } /> );
expect(
getByText(
'Unable to set up the plugin. Refresh the page and try again.'
)
).toHaveClass( 'wc-admin-shipping-banner-install-error' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to match snapshot here? Class name by itself doesn't look like a good indicator that the error is displayed. I mean, snapshot test will catch both text change and class name change and it'll be easier to manage the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defer to @c-shultz here - I just ported team Heisenberg's tests.

Comment on lines 347 to 350
expect(
document.getElementById( 'woocommerce-admin-print-label' ).style
.display
).toBe( 'none' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be better addressed either with .not.toBeVisible (link) or with .toHaveStyle (link).

Comment on lines +526 to +528
expect(
container.querySelector( '.wc-admin-shipping-banner-blob p' )
.textContent
).toBe( activatedMessage );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be sufficient checking the text only here? Or check for the class name is essential?

Suggested change
expect(
container.querySelector( '.wc-admin-shipping-banner-blob p' )
.textContent
).toBe( activatedMessage );
expect(
getByText( activatedMessage )
).toBeInTheDocument();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also deferring to @c-shultz here - I just ported team Heisenberg's tests.

Comment on lines +24 to +26
expect(
container.getElementsByClassName( 'woocommerce-card' )
).toHaveLength( 1 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already snapshot matcher above, so I'd say this check is not necessary. If you still consider it as important, I'd recommend changing it into expect( container.querySelector( '.woocommerce-card' ) ).toBeInTheDocument(); to have better output in case of error.

Comment on lines 25 to 33
expect(
getByRole( 'option', { name: 'lorem 1' } )
).not.toBeEmptyDOMElement();
expect(
getByRole( 'option', { name: 'lorem 2' } )
).not.toBeEmptyDOMElement();
expect(
getByRole( 'option', { name: 'bar' } )
).not.toBeEmptyDOMElement();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not critical, but iterating over options makes sense to me here: options.forEach( option => expect( getByRole( 'option', { name: option.label } ) ).toBeInTheDocument() );

@psealock psealock added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed [Status] Needs Review labels Oct 11, 2020
@jeffstieler
Copy link
Contributor Author

@vbelolapotkov thank you for the thorough review! (I was only expecting folks on different teams to check the code their team added 😅 )

I'm going through your feedback now. Thanks again.

@vbelolapotkov
Copy link
Contributor

@vbelolapotkov thank you for the thorough review! (I was only expecting folks on different teams to check the code their team added 😅 )

I wish I understand this before 😜 But hope the feedback was helpful 😄 Good luck!

I'm going through your feedback now. Thanks again.

👍 you're welcome!

@jeffstieler jeffstieler added [Status] Needs Review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Oct 13, 2020
@jeffstieler
Copy link
Contributor Author

Thanks again for the review @vbelolapotkov! I've addressed most of the feedback, and pinged other folks who can weigh in on what's important to test.

If this is looking good though, I think we should get it merged in before there are too many conflicts. 🙃

@joelclimbsthings
Copy link
Contributor

Hey @jeffstieler , looks great! I'd just been experimenting with react-testing-library for the last year, and enthusiastically approve of this change.

The only thing I'll note is that large snapshots such as client/analytics/components/leaderboard/test/__snapshots__/index.js.snap can be a bit of a bad smell at times, and test a lot of implementation details without providing a lot of clarity on the intentions of the developer. Kent Dodds writes a bit about this here. Not a big deal in this example, but something to keep in mind.

That said, looks good! ✔️

@samueljseay
Copy link
Contributor

@joelclimbsthings good point re snapshots. I think we should aim to remove snapshot testing altogether because I don't think we're getting tonnes of value from it. That said, I'd be happy to see that addressed later.

Copy link
Contributor

@samueljseay samueljseay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this @jeffstieler 🚢

@jeffstieler jeffstieler merged commit 1e0061d into main Oct 15, 2020
@jeffstieler jeffstieler deleted the dev/enzyme-to-rtl branch October 15, 2020 12:41
adrianduffell pushed a commit that referenced this pull request Nov 1, 2020
* Migrate leaderboard tests to RTL.

* Remove test of default prop value.

* Migrate ReportSummary tests to RTL.

* Migrate ActivityCard tests to RTL.

* Migrate ActivityCardPlaceholder tests to RTL.

* Migrate remaining ProductType tests to RTL.

* Migrate Card tests to RTL.

* Update RTL and user event packages.

* Migrate Date tests to RTL.

* Migrate D3Legend tests to RTL.

* Migrate D3Base tests to RTL.

* Migrate Gravatar tests to RTL.

* Migrate ImageUpload tests to RTL.

* Migrate ProductImage tests to RTL.

* Migrate Rating tests to RTL.

* Migrate Search tests to RTL.

* Migrate Plugins tests to RTL.

* Migrate SelectControl tests to RTL.

* Migrate Timeline tests to RTL.

Remove tests that inspect DOM since there are snapshots.

* Migrate DismissModal tests to RTL.

* Migrate SetupNotice tests to RTL.

* Migrate WelcomeCard tests to RTL.

* Fix setup error reason retrieval in ShippingBanner.

* Migrate ShippingBanner tests to RTL.

* Migrate RecommendedExtensions tests to RTL.

* Migrate KnowledgeBase tests to RTL.

* Rename enzyme setup file, modify to setup RTL.

* No need to import jest-dom in test files.

* Remove enzyme dependency.

* Use snapshot for testing Leaderboard markup.

* Switch from "not to be empty" to "be in the document".

* No need to waitFor() recordEvent mock.

* Be specific about clicking the "hide" button.

* Use toBeVisible() instead of checking style property.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tool: monorepo infrastructure type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants