Skip to content

Conversation

@lucabezerra
Copy link
Contributor

@lucabezerra lucabezerra commented Aug 25, 2021

Disclaimer

There is one test in the Filters.test.tsx suite that wasn't modernized. It seems like we've found a bug on react-testing regarding using .find() to search for components in a tree that contains React Fragments (it can't find the component if it's a few levels deep). Decided to submit the PR anyways as all the other tests have been modernized.

UPDATE: disclaimer no longer needed, as the aforementioned suite has already been modernized here: #4458

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 25, 2021

size-limit report

Path Size
cjs 143.11 KB (0%)
esm 96.81 KB (0%)
esnext 140.11 KB (0%)
css 33.96 KB (0%)

@lucabezerra lucabezerra force-pushed the filters-form-modernize-tests branch from 6d7aed4 to b4d79e8 Compare August 26, 2021 14:17
@lucabezerra lucabezerra marked this pull request as ready for review August 26, 2021 16:13
@lucabezerra lucabezerra requested a review from a team August 26, 2021 16:13
expect(resourceFilters.find(Popover).first().props().active).toBe(true);
trigger(shortcut, 'onClick');
expect(resourceFilters.find(Popover).first().props().active).toBe(false);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test wasn't modernized. See the PR's description (disclaimer) to find out why.

Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

@lucabezerra do we have an issue or PR to address the find fix? I don't want to forget about this legacy test. Happy to merge this in its current state.

@lucabezerra
Copy link
Contributor Author

@alex-page I was talking to @BPScott about this and his idea was that we'd eventually search for the test-utilities/legacy in the project once it's time to remove it and see that this test is still pending before the full deprecation can take place, so it won't be forgotten. Does that sound reasonable to you as well?

In the meantime I'm going to attempt fixing this find issue on react-testing, but I can't provide an estimate as I haven't done any work on that package yet.

@lucabezerra lucabezerra merged commit c5f19e8 into main Aug 26, 2021
@lucabezerra lucabezerra deleted the filters-form-modernize-tests branch August 26, 2021 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants