Skip to content

Use jest#7455

Merged
danez merged 1 commit into
masterfrom
pr-6179
Mar 3, 2018
Merged

Use jest#7455
danez merged 1 commit into
masterfrom
pr-6179

Conversation

@danez

@danez danez commented Feb 28, 2018

Copy link
Copy Markdown
Member
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
Comment thread test/testSetupFile.js Outdated
@@ -0,0 +1,2 @@
/* eslint-env jasmine */
jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000;

Copy link
Copy Markdown
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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed

Comment thread .babelrc.js
@@ -32,11 +43,4 @@ if (process.env.BABEL_ENV === "cov") {
config.plugins.push("babel-plugin-istanbul");

Copy link
Copy Markdown
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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

good catch

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

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@hzoo

hzoo commented Feb 28, 2018

Copy link
Copy Markdown
Member

@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

danez commented Feb 28, 2018

Copy link
Copy Markdown
Member Author

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

Comment thread .travis.yml
- '9'
- '8'
- '6'
- '4'

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

if (!deprecationWarningShown) {
deprecationWarningShown = true;

const deprecationError = new Error(

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

hzoo commented Feb 28, 2018

Copy link
Copy Markdown
Member

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

babel-bot commented Feb 28, 2018

Copy link
Copy Markdown
Collaborator

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

@existentialism existentialism left a comment

Copy link
Copy Markdown
Member

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

danez commented Mar 1, 2018

Copy link
Copy Markdown
Member Author

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

SimenB commented Mar 1, 2018

Copy link
Copy Markdown
Contributor

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

@danez

danez commented Mar 1, 2018

Copy link
Copy Markdown
Member Author

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

Andarist commented Mar 1, 2018

Copy link
Copy Markdown
Member

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

hzoo commented Mar 1, 2018

Copy link
Copy Markdown
Member

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

@Andarist

Andarist commented Mar 1, 2018

Copy link
Copy Markdown
Member

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

hzoo commented Mar 1, 2018

Copy link
Copy Markdown
Member

@Andarist

Andarist commented Mar 1, 2018

Copy link
Copy Markdown
Member

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