Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

Update to enzyme v3 #1012

Merged
merged 7 commits into from
Nov 13, 2017
Merged

Update to enzyme v3 #1012

merged 7 commits into from
Nov 13, 2017

Conversation

bjankord
Copy link
Contributor

Summary

  • Updated enzyme to latest stable release
  • Updated enzyme-to-json to latest stable release
  • Updated babel-jest to latest stable release

Additional Details

  • Replaced use of wrapper.unrendered with wrapper.instance().
  • Replaced use of wrapper.node with wrapper.getElement().
  • Removed jest tests for Base component which hit code that used require.ensure.
    • With updating to enzyme v3 and babel-jest v21, code that uses require.ensure throws errors in testing.
    • When searching around for this issue others have recommended shimming require.ensure when testing.
    • I tried adding the following code in various ways to shim the require.ensure calls in terra-i18n with no luck.
if (typeof require.ensure !== 'function') {
  require.ensure = (deps, cb) => cb(require);
}
  • Added nightwatch tests to cover the jest test which were removed for Base
    • With removing the jest tests that hit require.ensure, I added tests to nightwatch which cover the same functionality that was being tested before in jest for Base component

Resolves #871

@bjankord bjankord temporarily deployed to terra-core-deployed-pr-1012 November 12, 2017 13:56 Inactive
@emilyrohrbough
Copy link
Contributor

I am curious, did you try mocking require.ensure to load the translation data?

@@ -9,6 +9,17 @@ module.exports = resizeTo(['tiny', 'small', 'medium', 'large', 'huge', 'enormous
browser.expect.element('div').text.to.contain('en').before(waitInms);
},

// new
Copy link
Member

@kschuste kschuste Nov 13, 2017

Choose a reason for hiding this comment

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

nit: this comment could be a bit more descriptive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in this commit: de92c42

@@ -0,0 +1,22 @@
import React from 'react';

Copy link
Member

Choose a reason for hiding this comment

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

nit: have an extra line here between imports don't really need, also could remove an extra line after the imports as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in this commit: de92c42

@@ -8,6 +8,7 @@ it('should support rendering a string without translation', () => {
expect(base).toMatchSnapshot();
});


Copy link
Member

Choose a reason for hiding this comment

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

nit: added an extra line here not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in this commit: de92c42

@@ -8,7 +8,7 @@ it('should render the embedded content consumer container', () => {
src="https://www.google.com/"
/>);

const wrapper = shallow(embeddedContentConsumer);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason we're switching from shallow rendering to rendering the static html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The event listeners in the EmbeddedContentConsumer component were throwing errors after updating to enzyme v3 using the shallow method. My understanding on why this is an issue now is a bit rough but here's what I'm thinking. Attaching an event listener to the document with addEventListener means the event is not being handled by React's synthetic event system and I believe enyzme's shallow and mount methods use React's synthetic event system while render uses another implementation which supports addEventListener.

@bjankord bjankord temporarily deployed to terra-core-deployed-pr-1012 November 13, 2017 19:20 Inactive
@bjankord
Copy link
Contributor Author

@emilyrohrbough I looked around for a few ways to mock require.ensure, yet I got caught up on how exactly I might go about mocking the .ensure method so I ended up moving these tests from jest to nightwatch.

@bjankord bjankord temporarily deployed to terra-core-deployed-pr-1012 November 13, 2017 19:29 Inactive
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-1012 November 13, 2017 19:31 Inactive
@emilyrohrbough
Copy link
Contributor

@bjankord I think moving them to nightwatch is totally fine. I did a little digging myself since I was curious and it seem like the general suggestion is to move away from using require.ensure and actually use System.import (introduced with webpack v2) to do code splitting and dynamical loading. Might be something we should keep in mind for using/updating in the future.

@@ -3,7 +3,7 @@
exports[`renders a banner that has basic information 1`] = `
<ResponsiveElement
defaultElement={
<Component
<Unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "Unknown"?

Copy link
Contributor Author

@bjankord bjankord Nov 13, 2017

Choose a reason for hiding this comment

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

Not entirely sure, I tested this with shallow, mount, and render and all output the string, Unknown here. Looks like jest has some logic to determine the type of component for react elements / components which are passed in as props here.

With how _LargeDemographicsBannerDisplay and _SmallDemographicsBannerDisplay are exported, it seems jest is unable to determine what type they are and falls back to using the string Unknown where before it would use the string Component.

In _LargeDemographicsBannerDisplay and _SmallDemographicsBannerDisplay, we could update the exports to export the function expressions.

@bjankord bjankord merged commit 2b08c19 into master Nov 13, 2017
@bjankord bjankord deleted the enzyme-upgrade branch November 13, 2017 23:25
dylanklohr pushed a commit that referenced this pull request Apr 23, 2018
* Update to enzyme v3

* Remove unnecessary new lines

* Update Mutli and Single Selectable List Row snapshots
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants