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

Use jest #7455

Merged
merged 1 commit into from
Mar 3, 2018
Merged

Use jest #7455

merged 1 commit into from
Mar 3, 2018

Conversation

danez
Copy link
Member

@danez danez commented Feb 28, 2018

Q                       A
Fixed Issues? #6179 #6442
Patch: Bug Fix? n
Major: Breaking Change? n
Minor: New Feature? n
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

Rebase of #6179. Seems i removed my fork repo. :)

All tests work, babel-register is skipped for now. But I have an idea how to fix it, by mocking pirates and just checking what babel-register is doing, pirates is covered with tests anyway so we don't have to test it.

@danez danez added the PR: Internal 🏠 A type of pull request used for our changelog categories label Feb 28, 2018
@@ -0,0 +1,2 @@
/* eslint-env jasmine */
jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000;
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use jest.setTimeout

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -32,11 +43,4 @@ if (process.env.BABEL_ENV === "cov") {
config.plugins.push("babel-plugin-istanbul");
Copy link
Contributor

Choose a reason for hiding this comment

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

should this still be here? Jest adds it itself (and you removed it from package.json)

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch

.babelrc.js Outdated
@@ -40,7 +40,6 @@ const config = {

if (process.env.BABEL_ENV === "cov") {
config.auxiliaryCommentBefore = "istanbul ignore next";
Copy link
Contributor

Choose a reason for hiding this comment

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

jest also does this

Copy link
Member Author

Choose a reason for hiding this comment

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

:) even better

Copy link
Contributor

Choose a reason for hiding this comment

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

@hzoo
Copy link
Member

hzoo commented Feb 28, 2018

@danez maybe you didn't see #6442? we were going to merge that but not sure the relation @rogeliog

oh nvm I see you referenced it

@danez
Copy link
Member Author

danez commented Feb 28, 2018

I think there is no need to use the mocha runner, because just using the default jest runner works fine.

.travis.yml Outdated
- '8'
- '6'
- '4'
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated? We are we dropping node 4 testing already?

Copy link
Member

Choose a reason for hiding this comment

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

Can use node environment and keep 4? (like in babel-upgrade)

Copy link
Member

Choose a reason for hiding this comment

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

If it's because of jest, it still works on node 4 if you use the node environment - https://github.com/babel/babel-upgrade works fine with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok cool, I revert then.

@@ -180,15 +180,17 @@ export default function(
if (!deprecationWarningShown) {
deprecationWarningShown = true;

const deprecationError = new Error(
Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with just not doing the warning anymore and making this a breaking change. We can just fix this automatically with https://github.com/babel/babel-upgrade

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do this in a separate PR.

@rogeliog
Copy link

I agree with @danez! The Mocha runner is a good option when the complexity of the codebase makes it hard to fully migrate to Jest or as an intermediate step. Seem that I'll close it in favor of this, I can reopen it if needed.

@hzoo
Copy link
Member

hzoo commented Feb 28, 2018

Ok well if this works I'm for it too 👍 , thanks for your efforts @rogeliog, sorry we didn't actually end up merging the PR 😅

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 28, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7039/

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

Let's get this landed!

@danez
Copy link
Member Author

danez commented Mar 1, 2018

Ok I hope the coverage is now correct. I had to readd the babel-plugin-istanbul as some of our tests test the code in lib and not src. Wherever possible I changed to test src directly, but the fixture runner which runs stuff in separate envs couldn't be changed easily.

Okay I reverted this change and instead we test the transpiled code in lib, so that we have no change here atm, because I remembered that we did that on purpose at least for babylon as we want to test the compiled files/bundles. We can still change that later. babel-jest is now only used to compile the test files itself.

@SimenB
Copy link
Contributor

SimenB commented Mar 1, 2018

Anything jest can do to help/improve, or is it just a quirk (feature) of babel's tests?

@danez
Copy link
Member Author

danez commented Mar 1, 2018

I think that we in the past implemented some features that jest might already do, like running each test in a spearate vm (https://github.com/babel/babel/blob/master/packages/babel-helper-transform-fixture-test-runner/src/index.js#L87).
The other thing is that we bundle some packages with rollup/webpack and we want to test the bundle rather than the source in this cases to ensure that the bundles work.
We might be able to get rid of some of this now, but I want to make this PR as small as possible.

@Andarist
Copy link
Member

Andarist commented Mar 1, 2018

Super cool 👍I was wondering - it should be possible to migrate to builtin snapshots instead of custom setup in the future, right? This would allow easier test updates

@hzoo
Copy link
Member

hzoo commented Mar 1, 2018

I actually like the individual files lol compared to reading the snapshot output now lol

@Andarist
Copy link
Member

Andarist commented Mar 1, 2018

What's the difference though? A single snapshot format? Maybe it's possible to integrate with snapshot system to output whatever we want in whatever directory we want, just have it backed by jest's built-in system for this. expected files are a form of snapshots anyway

@hzoo
Copy link
Member

hzoo commented Mar 1, 2018

@Andarist
Copy link
Member

Andarist commented Mar 1, 2018

Actually I have no idea if its possible to change the whole format, it's for sure possible to write custom serializer, but not sure if a snapshot file can be an actual js file, not assigned to exports etc (jest snapshots are actually requireable). Worth investigating in the future

@danez danez merged commit 3e95830 into master Mar 3, 2018
@danez danez deleted the pr-6179 branch March 3, 2018 09:58
aminmarashi pushed a commit to aminmarashi/babel that referenced this pull request Mar 17, 2018
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants