Skip to content

Conversation

@ssidorchik
Copy link
Contributor

@ssidorchik ssidorchik commented Oct 5, 2018

  • BREAKING CHANGE?

Description

The Enzyme mount, render, shallow helpers accept only 2 arguments. So, this fix spreads the passed enzymeOptions into the second one before context and childContextTypes properties.

If context or childContextTypes is passed through the options, should be there any warning that these properties will be overridden?

Checklist

  • 👌 design updates are Garden Designer approved (add the
    designer as a reviewer)
  • 💅 view component styling is based on a Garden CSS
    component
  • 🌐 Styleguidist demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 💂‍♂️ includes new unit and snapshot tests
  • 📒 any new files are included in the packages src/index.js export
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

Copy link
Contributor

@austingreendev austingreendev left a comment

Choose a reason for hiding this comment

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

Good catch. I misread the enzyme docs when I originally created this.

Copy link
Contributor

@austingreendev austingreendev left a comment

Choose a reason for hiding this comment

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

Looks like CI is failing actually. Let's take a look and see if there are any bad usages in the MenuContainer.spec.js tests

@ssidorchik
Copy link
Contributor Author

This one seems to be tricky, since it doesn't fail when runs on its own. In Enzyme docs it's mentioned that tests may affect each other if they are using the same DOM. And it is suggested to use unmount for cleanup. I tried to do this for the first test and it fixes the failing test. But this doesn't look like the solid solution. @Austin94 what do you think about running unmount for every test in the afterEach block?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.835% when pulling 7aa5abb on ssidorchik/fix-enzyme-options into f267055 on master.

Copy link
Contributor

@austingreendev austingreendev left a comment

Choose a reason for hiding this comment

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

Looks like it was just some DOM leakage from us testing appendToNode={document.body}. I'm good with this now.

@austingreendev austingreendev merged commit 6cb703c into master Oct 8, 2018
@austingreendev austingreendev deleted the ssidorchik/fix-enzyme-options branch October 8, 2018 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants