[Test Modernization] Switch to using mountWithApp on ResourceItem and ResourceList tests. #4362

lucabezerra · 2021-08-05T17:45:11Z

WHY are these changes introduced?

Modernizes tests to use the new modern framework ({mountWithApp} from test-utilities).

WHAT is this pull request doing?

Updates tests using {mountWithAppProvider} from test-utilities/legacy to use {mountWithApp} from test-utilities

ghost · 2021-08-05T17:45:14Z

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

lucabezerra · 2021-08-05T17:47:02Z

src/components/ResourceItem/tests/ResourceItem.test.tsx

    it('does not call onClick when hitting keyUp on the item when no URL exists', () => {
      const onClick = jest.fn();
-      const wrapper = mountWithAppProvider(
+      const wrapper = mountWithApp(


Is it intentional to have this one test with mockSelectModeContext in the middle of all the other ones with mockDefaultContext?

I don't think so. Feel free to move it out of the default group

I'm wondering if we should move it out of the default group or change it's context to the default one, since the test itself seems related to its neighbours, it's just the context that's different (which could have been some autocomplete mistake). What do you think?

lucabezerra · 2021-08-05T17:48:08Z

src/components/ResourceItem/tests/ResourceItem.test.tsx


    it('renders a checked Checkbox if the item is in the selectedItems context', () => {
-      const wrapper = mountWithAppProvider(
+      const wrapper = mountWithApp(


This test is in the "SelectMode" describe block, but uses mockSelectableContext. Is this intentional?

github-actions · 2021-08-05T17:49:17Z

size-limit report

Path	Size
cjs	142.51 KB (0%)
esm	96.27 KB (0%)
esnext	139.44 KB (0%)
css	33.74 KB (0%)

kyledurand

Great job @lucabezerra 👏

UNRELEASED.md

kyledurand · 2021-08-10T14:45:40Z

src/components/ResourceItem/tests/ResourceItem.test.tsx

      );

-      expect(findByTestID(element, 'Item-Wrapper').prop('data-href')).toBe(url);
+      const div = element.find('div', {'data-href': url} as any);


You should be able to use toContainReactComponent here

expect(element).toContainReactComponent('div', {'data-href': url});

kyledurand · 2021-08-10T14:55:09Z

src/components/ResourceItem/tests/ResourceItem.test.tsx

    it('does not call onClick when hitting keyUp on the item when no URL exists', () => {
      const onClick = jest.fn();
-      const wrapper = mountWithAppProvider(
+      const wrapper = mountWithApp(


I don't think so. Feel free to move it out of the default group

kyledurand · 2021-08-10T15:05:02Z

src/components/ResourceItem/tests/ResourceItem.test.tsx

      );
-
-      expect(item.find('li').prop('data-href')).toBe('google.com');
+      const li = item.find('li', {'data-href': 'google.com'} as any);


This could probably use toContainReactComponent

kyledurand · 2021-08-10T15:05:52Z

src/components/ResourceItem/tests/ResourceItem.test.tsx

      );

-      expect(item.find('li').prop('data-href')).toBeUndefined();
+      const li = item.find('li', {'data-href': undefined} as any);


AndrewMusgrave

Left a comment about filtering by class names, ideally we wouldn't import the css module & css utilities. We're relying on the implementation logic in the test, which isn't ideal. Other than that, LGTM! 🎉

AndrewMusgrave · 2021-08-10T15:16:04Z

src/components/ResourceItem/tests/ResourceItem.test.tsx

+            styles.ResourceItem,
+            styles.selectable,
+            styles.selectMode,


Was there anything we could filter on, other than class names?

I was going to comment on this as well. Importing css modules into our tests will slow them down. Unfortunately we're already doing that in other areas and it is a nicer way to remove findByTestID

Yeah, I don't like it either. On top of the reasons you've mentioned, it kinda looks brittle to me (and sometimes complex to figure out which classes are expected depending on which props were passed to the component). I was hoping you folks would be able to suggest me a better alternative. Should I leave it like that, then?

Slow down will be negligible. The style file is already part of the module graph because ResourceItem itself imports it too. When Jest encounters .scss files it basically maps in identity-obj-proxy which is some very trivial JS that says "any key is valid, return that value as a string" - so styles.ResourceItem is 'ResourceItem', styles.selectable is 'selectable' etc.

Agree with Kyle, that the while not super great it's better than keeping using testID stuff, and I've got no better idea.

To reduce the need to play "guess all the classes' whachamole I think you might be able to use expect.stringContaining to reference just the one

wrapper .find('div', { className: expect.stringContaining(styles.ResourceItem) })!.trigger('onKeyUp', {key: 'Enter'})

e.g.

polaris-react/src/components/Button/tests/Button.test.tsx

Lines 424 to 426 in 746d2db

expect(button).toContainReactComponent(UnstyledButton, {

className: expect.stringContaining('removeUnderline'),

});

or use findWhere to write a filter function if that doesn't work

In addition to Ben's comment, here're a few ideas

find('div', {testID}) // we're generally against testID's but thought id mention it still works

use the class names directly so we don't reply on implementation logic find('div', {className: 'ResourceItem')

extract the logic so it's only in one location

const resourceItemSelector = {className: styles.ResourceItem'} ... component.find('div', resourceItemSelector)

Thanks, everyone!

As @BPScott had anticipated, the expect.stringContaining only works within the context of jest matchers, but I was able to introduce it to a few places anyway. I've used the findWhere everywhere else.

I've also incorporated a bit of @AndrewMusgrave 's suggestion to extract the logic into a single place, hopefully it's looking better now.

Thanks @kyledurand for spotting the places I've missed!

There are now just these 2 points before I can consider this done. Github won't let me paste the direct links to the discussions no matter what I try (it keeps removing the anchor from the link), so please copy the following anchors and paste it at the end of this PR's URL to go straight to them: #discussion_r683666010 and #discussion_r686332281

Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>

…pify/polaris-react into resourceitem-modernize-tests

BPScott · 2021-08-10T21:51:35Z

src/components/ResourceItem/tests/ResourceItem.test.tsx

  const ariaLabel = 'View Item';

+  const resourceItemSelector = {className: styles.ResourceItem};
+  const resourceItemFilter = (node: any) =>


Having two ways of doing this feels a little strange. It seems like resourceItemFilter does everything resourceItemSelector can but can additional cases. what if we stick with just resourceItemFilter.

I think we can type this better too: node: Node<unknown> where you can do import {Node} from '@shopify/react-testing'. Rathe than import from @shopify/react-testing in this file though, it might be nicer to do import {Node} from 'test-utiilities', and have src/test-utilities/react-testing.tsx rexport the Node type.

lucabezerra · 2021-08-30T16:12:25Z

This has been closed in favor of #4398, which uses the most recent implementation of react-testing, amongst other improvements.

lucabezerra commented Aug 5, 2021

View reviewed changes

lucabezerra changed the title ~~Switch to using mountWithApp on ResourceItem tests.~~ [Test Modernization] Switch to using mountWithApp on ResourceItem tests. Aug 5, 2021

Luca Bezerra added 5 commits August 9, 2021 13:42

Switch to using mountWithApp on ResourceItem tests.

04c4a89

Adding update to UNRELEASED.md

3c22e09

Removing testID props.

1e1357b

Fixing type-checking errors.

3252832

Modernizing tests for ResourceList.

2bf6d95

lucabezerra force-pushed the resourceitem-modernize-tests branch from 73d04ae to 2bf6d95 Compare August 9, 2021 17:43

Fixing lint errors.

6535c8c

lucabezerra marked this pull request as ready for review August 9, 2021 18:18

lucabezerra changed the title ~~[Test Modernization] Switch to using mountWithApp on ResourceItem tests.~~ [Test Modernization] Switch to using mountWithApp on ResourceItem and ResourceList tests. Aug 9, 2021

lucabezerra requested a review from a team August 10, 2021 13:48

Removing duplicate entry on UNRELEASED.md

c865613

alex-page approved these changes Aug 10, 2021

View reviewed changes

alex-page requested a review from AndrewMusgrave August 10, 2021 14:54

kyledurand approved these changes Aug 10, 2021

View reviewed changes

AndrewMusgrave approved these changes Aug 10, 2021

View reviewed changes

lucabezerra and others added 4 commits August 10, 2021 13:38

Update UNRELEASED.md

eb7a380

Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>

Merge branch 'main' into resourceitem-modernize-tests

c19b0a7

PR suggestions.

c2c3451

Merge branch 'resourceitem-modernize-tests' of https://github.com/Sho…

01e79b3

…pify/polaris-react into resourceitem-modernize-tests

BPScott reviewed Aug 10, 2021

View reviewed changes

Luca Bezerra and others added 3 commits August 11, 2021 10:31

Removing constant from tests and exporting Node from react-testing.

97de994

Better way of classList filtering and Node type exporting.

0be81d0

Merge branch 'main' into resourceitem-modernize-tests

75571d4

lucabezerra closed this Aug 30, 2021

alex-page deleted the resourceitem-modernize-tests branch March 22, 2022 04:18

	expect(button).toContainReactComponent(UnstyledButton, {
	className: expect.stringContaining('removeUnderline'),
	});

[Test Modernization] Switch to using mountWithApp on ResourceItem and ResourceList tests. #4362

[Test Modernization] Switch to using mountWithApp on ResourceItem and ResourceList tests. #4362

Uh oh!

Conversation

lucabezerra commented Aug 5, 2021

WHY are these changes introduced?

WHAT is this pull request doing?

Uh oh!

ghost commented Aug 5, 2021

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!

github-actions bot commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report

Uh oh!

kyledurand left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave 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!

BPScott Aug 10, 2021 • 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!

lucabezerra commented Aug 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions bot commented Aug 5, 2021 •

edited

Loading

BPScott Aug 10, 2021 •

edited

Loading