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

feat(jest): bump to 24 #957

Merged
merged 12 commits into from Feb 18, 2019
Merged

feat(jest): bump to 24 #957

merged 12 commits into from Feb 18, 2019

Conversation

thymikee
Copy link
Contributor

@thymikee thymikee commented Jan 26, 2019

Jest 24 is out, so why not bump it? :)

Let's wait until 24.0.1 is out before continuing on it (e2e tests are affected by jestjs/jest#7707).

EDIT: This still needs some work to be done with regards to babel-jest integration

@kulshekhar
Copy link
Owner

Thanks for opening this PR @thymikee 👍
I don't think I'll have time before this weekend but if this remains open/unresolved by then, I'll do what I can to get this passing and out!

package.json Outdated
@@ -70,7 +70,7 @@
"yargs-parser": "10.x"
},
"peerDependencies": {
"jest": ">=22 <24"
"jest": ">=22"

Choose a reason for hiding this comment

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

Considering the changes to babel-core (bridge) in this PR, it is safe to say it won't work with Jest <24.
Also considering that Jest has changed quite a bit between versions, it doesn't hurt to stay conservative and still explicitly require <25 IMHO.

Suggested change
"jest": ">=22"
"jest": ">=24 <25"

@SimenB
Copy link
Contributor

SimenB commented Feb 1, 2019

Probably wanna tweak the presets as well - I think all config but the transformer can be removed?

@thymikee
Copy link
Contributor Author

thymikee commented Feb 1, 2019

Would be cool to simplify it. Configuration of this preset is, let's say it lightly, pretty complicated :P

@jrr
Copy link

jrr commented Feb 6, 2019

Jest 2.4.1.0 released yesterday, including the changes from jestjs/jest#7707 : https://github.com/facebook/jest/releases/tag/v24.1.0

Is this unblocked now?

@thymikee
Copy link
Contributor Author

thymikee commented Feb 6, 2019

@jrr yup, working on upgrading the deps, these e2e tests are a pain to work with 😅

@thymikee
Copy link
Contributor Author

thymikee commented Feb 6, 2019

I've also bumped Jest typings to 24 and it spawned some new errors. Can somebody with TS experience can take a look? Help appreciated :)

@orta
Copy link
Contributor

orta commented Feb 7, 2019

Took a look while something long running was happening, part of me is wondering if the implementation for mockImplementation is off in DefinitelyTyped/DefinitelyTyped#32579

Either that or this repo has consistently used mockImplementation wrong, I'm not too sold on that because it works, but maybe Jest has always been liberal about accepting the thunk in mockImplementation?

e.g.

jest.mock('fs')
const fs = mocked(_fs)

// fs.existsSync = (path: string) => boolean

// Fails
fs.existsSync.mockImplementation(() => true)
// Passes
fs.existsSync.mockImplementation(() => () => true)

If jest accepts passing in the direct function instead of the thunk to the function, then maybe the types should too?

@orta
Copy link
Contributor

orta commented Feb 7, 2019

I bet this is fixed by DefinitelyTyped/DefinitelyTyped#32816 - I wonder if it's worth making these mocks a Partial instead of the full objects, a lot of the as anys that I am looking at adding are because the mock is a partial version of the obj. That's a really natural pattern for mocks.

@orta
Copy link
Contributor

orta commented Feb 7, 2019

orta@28fdcd7 should handle a bunch of the type issues when that PR is merged and updated in here. Might be worth skipping the types/jest update for now.

@nasreddineskandrani
Copy link

nasreddineskandrani commented Feb 7, 2019

@orta you should ban any and so as any with a combination of tslint rule noAny + compiler noImplicitAny Flag
It's better to go with Partial in my opinion.

@antoinebrault
Copy link

antoinebrault commented Feb 7, 2019

I bet this is fixed by DefinitelyTyped/DefinitelyTyped#32816 - I wonder if it's worth making these mocks a Partial instead of the full objects, a lot of the as anys that I am looking at adding are because the mock is a partial version of the obj. That's a really natural pattern for mocks.

@orta @thymikee Your problem seems to be that MockWithArgs should be :

interface MockWithArgs<T> extends Function, jest.MockInstance<ReturnType<T>, ArgumentsOf<T>>

@armano2
Copy link
Contributor

armano2 commented Feb 14, 2019

@thymikee @orta fixes to @types/jest are merged
https://www.npmjs.com/package/@types/jest v24.0.4

@Jessidhia
Copy link

The problem, when I tested, is that this is incompatible with babel-jest. While it may work if you are only using tsc compilation mode, if you are also using babel postprocessing (for e.g. styled-components) this will crash.

@coveralls
Copy link

coveralls commented Feb 14, 2019

Pull Request Test Coverage Report for Build 2384

  • 7 of 7 (100.0%) changed or added relevant lines in 5 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+1.0%) to 91.916%

Files with Coverage Reduction New Missed Lines %
src/config/config-set.ts 2 93.53%
src/cli/config/migrate.ts 2 86.67%
Totals Coverage Status
Change from base Build 2380: 1.0%
Covered Lines: 910
Relevant Lines: 955

💛 - Coveralls

@artola
Copy link

artola commented Feb 14, 2019

The problem, when I tested, is that this is incompatible with babel-jest. While it may work if you are only using tsc compilation mode, if you are also using babel postprocessing (for e.g. styled-components) this will crash.

I agree .. I found this problem because we use babel plugins after ts and they break with v24.1

@kulshekhar
Copy link
Owner

would it be possible for someone to test this with a real project? That would increase the confidence in releasing a new version to some extent.

I'll give this a day or two to wait for some feedback and will then merge it in and publish a new version even if there isn't any.

@hermanbanken
Copy link

Running this with a real project now...

@thymikee
Copy link
Contributor Author

Mind releasing a temporary version to npm with that? Would be easier to test e.g. for jest-preset-angular cc @ahnpnl @wtho

@omril1
Copy link

omril1 commented Feb 18, 2019

Can you update it in NPM as next tag?

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 18, 2019

maybe can release it as alpha @kulshekhar ?

@kulshekhar
Copy link
Owner

ok, I'll do that after work today

@hermanbanken
Copy link

Works just fine with one of our (kinda big: 16 suites/231 tests) applications! 🎉

@kulshekhar
Copy link
Owner

Works just fine with one of our (kinda big: 16 suites/231 tests) applications!

ok, in that case I'll just release the full version later today to save on time. I mean, folks can pin it down to 23.x if it turns out to be a problem. Not ideal but doesn't seem like a dealbreaker

@orta
Copy link
Contributor

orta commented Feb 18, 2019

Also confirmed it works, ran it against Danger:

Test Suites: 43 passed, 43 total
Tests: 1 skipped, 410 passed, 411 total
Snapshots: 44 passed, 44 total
Time: 13.179s

@kulshekhar kulshekhar merged commit 61d31b4 into kulshekhar:master Feb 18, 2019
@kulshekhar
Copy link
Owner

v24.0.0 has been published. Thank you everyone for getting this through!

virtuoushub pushed a commit to virtuoushub/jest-preset-angular that referenced this pull request Feb 19, 2019
@artola
Copy link

artola commented Feb 19, 2019

@kulshekhar Moving to jest v24 also means that jest-config starts to use babel-jest v24.

My tests break with the following error:

  ● Test suite failed to run

    Passing cached plugin instances is not supported in babel.loadPartialConfig()

      at forEach.item (node_modules/@babel/core/lib/config/partial.js:120:13)
          at Array.forEach (<anonymous>)
      at loadPartialConfig (node_modules/@babel/core/lib/config/partial.js:118:27)
      at TsJestTransformer.process (node_modules/ts-jest/dist/ts-jest-transformer.js:110:32)

This problem still persists.

@belgattitude
Copy link

belgattitude commented Feb 19, 2019

@kulshekhar Thanks for your time and efforts into this project, I was about to test the P/R today, look I've been a bit late. With the new release, I face the same error as @artola mentioned.

I guess it might work merging #960, but I found out that I could simply remove reference to .babelrc from my jest.config.js. I guess I don't need it (anymore ?).

    globals: {
        window: {},
        'ts-jest': {
            tsConfig: './tsconfig.jest.json',
            compiler: 'typescript',
            // COMMENT THIS, and the partialConfig problem disappear with ts-jest 24
            // babelConfig: '.babelrc',
        },
    },

@artola can you try ? Works for me.

@Jessidhia
Copy link

Jessidhia commented Feb 19, 2019

That will "work" if you have no need for babel plugins when running tests. That'll be the case in a lot of cases but an easy example where you do need them is, for example, with styled-components (especially if you use the css prop), or other things that aren't just plain spec transforms.

@belgattitude
Copy link

@Jessidhia Good explanation thanks.

@artola
Copy link

artola commented Feb 19, 2019

@belgattitude @Jessidhia I have in the global babelConfig: true, and if I comment that line, then no transform is done, I have a bunch of plugins and preset-env, needed.

I have no clue how to go around Passing cached plugin instances is not supported in babel.loadPartialConfig().

Without a solution, I can not profit of v24.

@SimenB
Copy link
Contributor

SimenB commented Feb 19, 2019

I find it a bit odd this was merged (or at least released) without fixing the babel-jest integration, or a big note somewhere that it's currently broken. Maybe edit the OP to include the info?

@kulshekhar
Copy link
Owner

kulshekhar commented Feb 19, 2019

I find it a bit odd this was merged (or at least released) without fixing the babel-jest integration, or a big note somewhere that it's currently broken. Maybe edit the OP to include the info?

Yes, it's not ideal. From my previous post

ok, in that case I'll just release the full version later today to save on time. I mean, folks can pin it down to 23.x if it turns out to be a problem. Not ideal but doesn't seem like a dealbreaker

It was a tradeoff between

  • delaying it and waiting for someone to fix it and then test it (not sure when I'll personally have time next to fix this)
  • releasing it so that those who were active here and want to use jest 24 and don't have their setup broken by this can get to use it

Maybe edit the OP to include the info?

Yeah, I think that's a good idea. I'll update the OP later today after work and also add a note to the readme (PR welcome too!) that reflects this

Additionally, if someone thinks that becoming a collaborator would help them get this (or other issues) moving faster, just let me know

@ivanfilhoz
Copy link

ivanfilhoz commented Feb 19, 2019

Everything passing here. Thanks to @thymikee, @kulshekhar and others!

@GeeWee
Copy link
Collaborator

GeeWee commented Feb 19, 2019

For everyone with an interest in ts-jest, as you can probably tell we are a little short-staffed. We'd love to get some more active contributors :)

@artola
Copy link

artola commented Feb 20, 2019

@thymikee @kulshekhar Regarding the issue with babel and the cached config Passing cached plugin instances is not supported in babel.loadPartialConfig(), I found that if we return config = {}; in this line ... then everything goes fine, no error.

config = loadOptions(base) as BabelConfig

I hope that this is the tip of the iceberg, but for sure it is not a fix, neither I have a complete understanding of of the other conditions around this block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet