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

Upgrade some dependencies: React 16, Enzyme 3 and Jest #2813

Closed
wants to merge 3 commits into from

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Sep 27, 2017

Checklist:

  • Upgrade to the stable version of React
  • Upgrade Jest (MIT license)
  • Upgrade Enzyme to v3
  • Upgrade to React 16 in unit tests

The trickiest part here is the Enzyme and React upgrade in Unit tests. I think we should run the same version we use on Prod on our unit tests. Also, Enzyme 3 comes with some annoying breaking changes.

This change has the side effect of showing some warnings on npm install because some React libraries do not support React 16. I haven't checked yet if there are newer versions for these dependencies (will do). I personally the warning is worth seeing to remind us what we need to update.

There are still two unit tests I'm not able to fix with Enzyme3 (the one marked as skipped)

@youknowriad youknowriad added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Sep 27, 2017
@youknowriad youknowriad self-assigned this Sep 27, 2017
@youknowriad youknowriad requested review from gziolo and aduth September 27, 2017 16:23
@codecov
Copy link

codecov bot commented Sep 27, 2017

Codecov Report

Merging #2813 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2813      +/-   ##
==========================================
- Coverage   33.81%   33.73%   -0.08%     
==========================================
  Files         190      190              
  Lines        5678     5676       -2     
  Branches      992      991       -1     
==========================================
- Hits         1920     1915       -5     
- Misses       3181     3183       +2     
- Partials      577      578       +1
Impacted Files Coverage Δ
editor/selectors.js 94.73% <0%> (-2.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a949b7...ad316ab. Read the comment docs.

@youknowriad
Copy link
Contributor Author

@gziolo If you have some ideas on how to fix these skipped tests, that would be awesome.

} );

it( 'should render into provider context', () => {
// skipped because it's not working with React/Enzyme upgrade
it.skip( 'should render into provider context', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to wait until Support Portals properly is resolved :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think this should block the PR? thoughts @aduth

Copy link
Member

Choose a reason for hiding this comment

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

I don't consider it as a blocker. I should phrase it explicitly. Just saying we would have to fix Enzyme in the first place or keep the shim that was used before.

@@ -64,7 +64,7 @@ const textEmbedBlock = {
category: 'embed',
};

describe( 'InserterMenu', () => {
describe.skip( 'InserterMenu', () => {
Copy link
Member

@gziolo gziolo Sep 29, 2017

Choose a reason for hiding this comment

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

I did some heave debugging here and it looks like we are having the same kind of issues that are reported in Enzyme repository here and here.

In our case it might be a mix of those two. I tried to add recommended wrapper.update(); call after the click event is simulated, but it didn't help. It's quite interesting scenario because the part of the component that depends on the component's state (

className={ `editor-inserter__tab ${ this.state.tab === 'embeds' ? 'is-active' : '' }` }
) gets properly updated, but the other part (
{ isShowingEmbeds && this.renderBlocks( visibleBlocksByCategory.embed ) }
) which also is conditionally rendered based on the components's state doesn't show up in the output of wrapper.debug() call. When I debugged it turned out that React component does everything properly behind the scenes. It looks like Enzyme gets out of sync.

@gziolo
Copy link
Member

gziolo commented Sep 29, 2017

I would wait a week or two with this PR. Mostly because there is one known issue which doesn't work anymore with Enzyme 3 reported in 2 places enzymejs/enzyme#1153 and enzymejs/enzyme#1163 and 2 React 16 features are not implemented yet: enzymejs/enzyme#1149 (Array-rendering components) and enzymejs/enzyme#1150 (Portals - which we use).

@gziolo
Copy link
Member

gziolo commented Oct 3, 2017

They published enzyme 3.1.0 and enzyme-adapter-react-16 1.0.1, but it makes even more tests to fail. I spent maybe 15 minutes trying to fix them, but I didn't have enough courage to get into debug mode :)

@youknowriad
Copy link
Contributor Author

@gziolo Should we close this for now. We can revisit in some weeks

@gziolo
Copy link
Member

gziolo commented Oct 4, 2017

Yes, it's a very good idea :)

@gziolo gziolo closed this Oct 4, 2017
@gziolo gziolo deleted the update/deps branch October 4, 2017 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants