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

chore: upgrade jest to incorporate jsdom bugfix #5456

Merged
merged 6 commits into from
Mar 3, 2020

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Feb 27, 2020

This PR upgrades Jest to the latest version (v25.1.0) in order to incorporate a jsdom bugfix. The outdated jsdom dependency was causing test failures in our FloatingMenu component due to an upstream bug which would dispatch a focus event when the focus() method was called on an already focused element, leading to endless cycling between focus and blur event handler calls. Discovered while working on #5365

refs:
jsdom/jsdom#2621
jsdom/jsdom#2700

jsdom is also now handling document.activeElement differently from previous versions, so some tests have been revised to account for that breaking change

refs:
ant-design/ant-design#21185
jsdom/jsdom#2586
jsdom/jsdom#2723
enzymejs/enzyme#2337

Testing / Reviewing

Ensure none of our tests are broken

@emyarod emyarod requested a review from a team as a code owner February 27, 2020 03:40
@ghost ghost requested review from dakahn and tw15egan February 27, 2020 03:40
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @emyarod!

@netlify
Copy link

netlify bot commented Feb 27, 2020

Deploy preview for carbon-elements ready!

Built with commit a4469c5

https://deploy-preview-5456--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Feb 27, 2020

Deploy preview for carbon-components-react ready!

Built with commit a4469c5

https://deploy-preview-5456--carbon-components-react.netlify.com

@emyarod emyarod requested a review from asudoh February 27, 2020 05:12
@emyarod
Copy link
Member Author

emyarod commented Feb 27, 2020

re-requesting review due to some test refactoring (edited PR description for more context and added relevant tickets for context)

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

One comment - I see several tests are change to add a <div> under <body>. Wondering if it'll be cleaned up once the test finishes?

@emyarod
Copy link
Member Author

emyarod commented Feb 27, 2020

I remove them in test suites where they are reused, but I can add the line to each modified individual test case https://github.com/carbon-design-system/carbon/pull/5456/files#diff-e6bde0b208435880662cb06911ee85c1R246

@asudoh
Copy link
Contributor

asudoh commented Feb 28, 2020

Thank you for the update @emyarod! Given you added a clean-up code at the last line of each test case, it won't run if there are any assertions failures, etc. You could use a try ... catch block, but I tend to do:

describe('suite', () => {
  let div;

  it('does the right thing', () => {
    div = document.createElement('div');
    document.body.appendChild(div);
    (Your test)
  });

  afterEach(() => {
    if (div && div.parentNode) {
      div.parentNode.removeChild(div);
    }
    div = null;
  });
});

Note: Ensuring clean-up stuffs done after (each) tests avoids flaky tests.

Copy link
Contributor

@asudoh asudoh 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 the update @emyarod!

@joshblack joshblack merged commit 1b02dac into carbon-design-system:master Mar 3, 2020
@emyarod emyarod deleted the upgrade-jest-25 branch March 4, 2020 21:05
joshblack added a commit to joshblack/carbon that referenced this pull request Mar 10, 2020
…#5456)

* chore: upgrade jest

* test: adjust for new document.activeElement behavior in jsdom

* test: clean up container div

Co-authored-by: Josh Black <josh@josh.black>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants