Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update activeElement tests to use modern testing utils #3070

Merged
merged 1 commit into from
Jun 23, 2020

Conversation

BPScott
Copy link
Member

@BPScott BPScott commented Jun 23, 2020

WHY are these changes introduced?

An imminent bump to SK and thus Jest exposes a case where enzyme doesn't handle component mounting to jsdom in a way that results in spec compliant focus management, which then results in tests that assert on document.activeElement failing.
Fortunately react-testing doesn't have this problem, so we can just usethat instead.

Gory details: enzymejs/enzyme#2337 (comment)

WHAT is this pull request doing?

Migrates a handful of test cases from enzyme to use react-testing

How to 🎩

Run tests and see they still pass

@github-actions
Copy link
Contributor

github-actions bot commented Jun 23, 2020

🟢 No significant changes to src/**/*.tsx were detected.

@BPScott BPScott force-pushed the prepare-for-new-sk branch from c3e998e to 187b9a0 Compare June 23, 2020 01:23
@BPScott BPScott requested a review from kaelig June 23, 2020 01:27
@BPScott BPScott force-pushed the prepare-for-new-sk branch from 187b9a0 to 51ec767 Compare June 23, 2020 01:31
An imminent bump to SK and thus Jest exposes a case enzyme doesn't
handle component mounting to jsdom in a way that is spec compliant,
which then results in test that assert on document.activeElement failing.
Fortunatly react-testing doesn't have this problem, so we can just use
that instead
@BPScott BPScott force-pushed the prepare-for-new-sk branch from 51ec767 to b1ac67b Compare June 23, 2020 02:04
@BPScott BPScott requested a review from alex-page June 23, 2020 02:33
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.

Thanks @BPScott this looks good. The open issue is also super great context.

Copy link
Contributor

@kaelig kaelig left a comment

Choose a reason for hiding this comment

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

Thank you for doing this!

Is there any internal documentation about how to test focus that we could point to in the body of this PR?

@@ -257,15 +245,15 @@ describe('<TrapFocus />', () => {

listenerMap.keydown({
keyCode: Key.Tab,
target: lastNode(trapFocus),
target: trapFocus.find('input')!.domNode,
Copy link
Contributor

Choose a reason for hiding this comment

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

This (and a few other instances) slightly diverges from the intent of the test but I feel good about this change.

@BPScott
Copy link
Member Author

BPScott commented Jun 23, 2020

Is there any internal documentation about how to test focus that we could point to in the body of this PR?

Unfortunately there is none that I am aware of. But from working through this stuff it boils down to:

  • use react-testing
  • use expect(document.activeElement).toBe(someNode) instead of expect(someNode).toBe(document.activeElement) as that way around gives more meaaningful error messages.

@BPScott BPScott merged commit 9afbf46 into master Jun 23, 2020
@BPScott BPScott deleted the prepare-for-new-sk branch June 23, 2020 17:35
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.

3 participants