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.

Sign up for GitHub

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

Jump to bottom

Deps: Update Jest to 22 #20943

Merged
merged 5 commits into from
Dec 20, 2017
Merged

Deps: Update Jest to 22 #20943

merged 5 commits into from
Dec 20, 2017

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Dec 19, 2017 •
edited
Loading

Updates Jest to v22.

This PR also gets us off of a beta release of jest-docblock 👍

Thanks @gziolo for the nudge WordPress/gutenberg#4073 (review)

Notable improvements:

  • The mess of requestAnimationFrame warnings shown during tests are gone 🎉
  • Codeframe is 💯
  • jest-docblock out of beta
  • More

Follow up

Testing

  1. Are tests passing? 😁

@sirreal sirreal self-assigned this Dec 19, 2017
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=update/jest-22

@gziolo
Copy link
Member

gziolo commented Dec 19, 2017

CircleCi doesn't look as happy as we do :D

@sirreal
Copy link
Member Author

sirreal commented Dec 19, 2017

CircleCi doesn't look as happy as we do :D

Nope, not ready yet, still in progress.

Only 2 failures though 😁

@sirreal
Add fullscreen to test to bypass Node.contains check
4a9e934 
This fixes a test issue by sidestepping it. I prefer this to focus on
the Jest upgrade, but it would be nice to migrate these tests to enzyme
which would likely solve this problem.
@sirreal
Fix lib/accept tests
91ddc41
sirreal
sirreal commented Dec 19, 2017
View reviewed changes
client/lib/accept/test/index.js
document.querySelector( '.button.is-primary' ).click();
expect( document.querySelector( '.accept-dialog' ) ).to.be.null;
Copy link
Member Author

Choose a reason for hiding this comment

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

It was surprisingly difficult to come to this simple solution this test fix.

Copy link
Member

Choose a reason for hiding this comment

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

Lol, should work as expected when you compare with one of the previous tests :)

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 19, 2017
@sirreal sirreal requested review from ockham, gziolo and samouri December 19, 2017 11:31
@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 19, 2017
@gziolo
Copy link
Member

gziolo commented Dec 19, 2017

There are some strange errors coming from eslines. Did you investigate them?

@sirreal sirreal added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 19, 2017
@sirreal
Copy link
Member Author

sirreal commented Dec 19, 2017

There are some strange errors coming from eslines. Did you investigate them?

Those were a surprise, yes I've started investigating.

@sirreal
Rollback eslint-plugin-jest upgrade
175d26b 
This was causing issues with eslint-eslines on CI
@sirreal
Copy link
Member Author

sirreal commented Dec 19, 2017

The upgraded eslint-plugin-jest was incompatible with the versions of some other eslint related packages we're currently using. Rather than enter into dependency-upgrade-hell, I've rolled back and now CI is 💚

Ready for review!

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 19, 2017
@sirreal sirreal added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 19, 2017
ockham
ockham approved these changes Dec 19, 2017
View reviewed changes
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

❤️ it, 🚢 it!

@sirreal sirreal added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 20, 2017
@sirreal sirreal merged commit 7d98b9e into master Dec 20, 2017
@sirreal sirreal deleted the update/jest-22 branch December 20, 2017 05:43
@gziolo
Copy link
Member

gziolo commented Dec 20, 2017

So they bumped version to 22.0.3, interesting :)

@sirreal
Copy link
Member Author

sirreal commented Dec 21, 2017

I've just tried the new experimental --detectLeaks:

npm run test-client -- --detectLeaks

I expected some unusual behavior, but it fails for every single test, both client and server. I'm curious what we're doing to cause that 🤔

@gziolo
Copy link
Member

gziolo commented Dec 21, 2017

Let me know if you discover what is causing those leaks. It might be the fact that we disable network traffic in here: https://github.com/Automattic/wp-calypso/blob/master/test/client/setup-test-framework.js#L11.

You can also try to compare with Gutenberg which has a slightly different setup.

@allendav allendav mentioned this pull request Jan 11, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ockham ockham ockham approved these changes

@gziolo gziolo Awaiting requested review from gziolo

@samouri samouri Awaiting requested review from samouri

Assignees

@sirreal sirreal

Labels
Framework [Tests] Includes Tests [Type] Janitorial
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sirreal @matticbot @gziolo @ockham