Replace Jasmine with jest-circus #3668
Conversation
|
Holy shit! This is really, really good. I think we'll probably need to roll this out incrementally. The testFramework option is already customizable but cannot be changed per-test. We could make it so that the framework can be configured within a test file ( More thorough review coming later this week. |
packages/jest-framework/src/index.js
Outdated
| beforeAll, | ||
| beforeEach, | ||
| describe, | ||
| it, |
There was a problem hiding this comment.
Is that mean we'll be able to import it instead of use the global one? ❤
There was a problem hiding this comment.
yeah! i think that will be much easier to do now :)
| } | ||
| }; | ||
|
|
||
| // Get suppressed errors form jest-matchers that weren't throw during |
There was a problem hiding this comment.
I think there's a typo: from. And there's an extra space after form.
packages/jest-framework/src/utils.js
Outdated
| if (!fn) { | ||
| mode = 'skip'; // skip test if no fn passed | ||
| } else if (!mode) { | ||
| // if not set exlicitly, inherit from its parent describe |
packages/jest-framework/src/utils.js
Outdated
| testContext: TestContext, | ||
| {isHook}: {isHook?: boolean} = {isHook: false}, | ||
| ): Promise<any> => { | ||
| // If this fn accepts `done` callback we return a promise that fullfils as |
| fn(); | ||
| jestExpect(fn)[calledWith](); | ||
| }); | ||
|
|
||
| test(`${calledWith} works with ${mockName} and arguments that don't match`, () => { | ||
| const fn = getFunction(); | ||
| test(`${calledWith} works when arguments that don't match`, () => { |
There was a problem hiding this comment.
Not sure if this was mistakenly changed to when; it was with previously, like the other tests here.
|
Would it make sense for this to live in a separate repo outside of jest? Or will it get built and published outside of it anyway? |
|
Yep, it'll get published separately. |
|
@yangshun thanks for catching that! |
aaronabramov
force-pushed
the
jest-framework-1
branch
from
006d003 to
6887e1b
Compare
Oh man, all I do on planes is watch episodes of TV shows that I've already seen, and unsuccessfully try to sleep. You seem much more productive 😛
Will this make it easier to attach custom reporters? For example, we're using an AppVeyor reporter to properly show test runs in AppVeyor: https://github.com/facebook/jest/blob/9b157c3a7c325c3971b2aabbe4c8ab4ce0b0c56d/testSetupFile.js#L15-L18. I guess removing Jasmine will mean we need to write our own reporters rather than reusing existing open-source Jasmine reporters, but this could tie in nicely with #3536 if we could ship said reporters out-of-the-box. |
|
@Daniel15 i haven't looked into how Because of the way we run tests in Jest (multiple files in parallel, which means multiple Jasmine instances in parallel) we have two layers that we can report from:
I believe that 99% of the time we want to use the second type of reporting, and that was enabled by #3349 |
|
@DmitriiAbramov - looks like I agree with you - It might be easier to take the second approach you mentioned (processing the aggregated results). I completely missed #3349 - Thanks for the information! Sounds like we can already build these custom reporters even before switching away from Jasmine. |
|
@DmitriiAbramov We (in wallaby.js integration with jest) are heavily using the following jasmine's reporter interface functions to capture test execution results from jest:
Just an aggregated result will not be sufficient for us (or any integration that relies on the existing jasmine's reporter interface). So it would be great if there was a way to subscribe/hook into the test execution pipeline and get the same level of data (for each test as they execute). |
|
@ArtemGovorov yeah! you'll definitely be able to hook into the framework. and probably even create an adapter that'll keep the interface consistent with all your custom reporters that you're using right now. the only thing you won't get is the |
This is a really good idea - Maintain backwards compatibility with Jasmine reporters by including an adapter that exposes the old Jasmine API. |
|
@DmitriiAbramov Great, thanks for confirming it!
and now |
|
@DmitriiAbramov So as long as the |
|
@ArtemGovorov oh wow! yeah, that totally makes sense. and yes, the |
packages/jest-framework/package.json
Outdated
| @@ -0,0 +1,16 @@ | |||
| { | |||
| "name": "jest-framework", | |||
|
The changes you made unrelated to the new test runner, can you send a separate PR for those cleanups/fixes? |
aaronabramov
force-pushed
the
jest-framework-1
branch
from
6887e1b to
7614749
Compare
[WIP] jest-framework [WIP] jest-framework [WIP] Integrating with Jest [WIP] jest-framework [WIP] jest-framework [WIP] jest-framework
aaronabramov
force-pushed
the
jest-framework-1
branch
from
7614749 to
657f967
Compare
|
all PRs with changes affecting the rest of Jest were merged and this one is ready to go. |
Codecov Report
@@ Coverage Diff @@
## master #3668 +/- ##
==========================================
- Coverage 62.63% 59.97% -2.67%
==========================================
Files 183 191 +8
Lines 6702 7003 +301
Branches 6 6
==========================================
+ Hits 4198 4200 +2
- Misses 2501 2800 +299
Partials 3 3
Continue to review full report at Codecov.
|
aaronabramov
force-pushed
the
jest-framework-1
branch
from
7afd7e1 to
86b00ca
Compare
aaronabramov
force-pushed
the
jest-framework-1
branch
from
8c59b50 to
c50b356
Compare
| @@ -0,0 +1,225 @@ | |||
| /** | |||
There was a problem hiding this comment.
I really hate utils methods. Can we split up this file into many small modules or colocate the functions with where they are needed? I really want Jest to get rid of the word "util" in packages and file names entirely.
There was a problem hiding this comment.
what exactly do you not like about util methods?
these are actual pure utility functions that transform/massage the data. I took them out of the rest of the code because they:
- are mostly stable and don't need to be changed
- have too much noise (if/else, branching, loops, etc.)
- should almost never be looked at.
I really dislike colocating them with the files because after a few refactors they end up being coupled to the rest of the code and we end up with spaghetti code that's very hard to read :(
| return result; | ||
| }; | ||
|
|
||
| const getEachHooksForTest = ( |
There was a problem hiding this comment.
i couldn't come up with a better name, but it's supposed to be "get 'each' hooks", where "each hooks" is beforeEach or afterEach
| break; | ||
| } | ||
| } | ||
| } while ((block = block.parent)); |
There was a problem hiding this comment.
I wonder if there is something smarter we could do here instead of having to traverse up the tree every single time. Maybe a later optimization.
There was a problem hiding this comment.
yeah, i had a few ideas, but though they were premature optimization. i don't think we should worry about it yet
| // If this fn accepts `done` callback we return a promise that fullfills as | ||
| // soon as `done` called. | ||
| if (fn.length) { | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
This is specifically for converting a nodeback to Promise so can't, unless you use util-promisify.
There was a problem hiding this comment.
yeah. this function is an adapter between async/await world and done() world
packages/jest-circus/src/utils.js
Outdated
| const {status} = test; | ||
|
|
||
| if (!status) { | ||
| console.log(test); |
packages/jest-circus/src/utils.js
Outdated
|
|
||
| if (!status) { | ||
| console.log(test); | ||
| throw new Error('status should be present after tests are run'); |
There was a problem hiding this comment.
"Status" and end the sentence in .. :)
| titles.unshift(parent.name); | ||
| } while ((parent = parent.parent)); | ||
|
|
||
| titles.shift(); // remove TOP_DESCRIBE_BLOCK_NAME |
There was a problem hiding this comment.
Is this lifted from Jasmine? Do we need this in circus?
There was a problem hiding this comment.
we do. In order to use beforeEach or test at the top level we need to have a virtual top level describe that will wrap everything. But we also want to make it invisible for the outside world
| const {currentDescribeBlock} = state; | ||
| if (!currentDescribeBlock) { | ||
| throw new Error( | ||
| `currentDescribeBlock has to be there since we're finishing its definition`, |
| break; | ||
| } | ||
| case 'run_start': { | ||
| state.testTimeout = global[TEST_TIMEOUT_SYMBOL] || state.testTimeout; |
There was a problem hiding this comment.
Can you only reassign this if global[TEST_TIMEOUT_SYMBOL] is set? I don't like potential no-ops like foo = false || foo
| const {dispatch} = require('./state'); | ||
|
|
||
| const describe = (blockName: BlockName, blockFn: BlockFn) => | ||
| _dispatchDescribe(blockFn, blockName); |
There was a problem hiding this comment.
It doesn't really make sense to me that the _dispatchDescribe function is an inverse. You wouldn't have to wrap this function and you could expose it directly if you reversed them (unless you prefer the function wrapper to add .only and .skip onto it – in which case I'd still reverse the args).
There was a problem hiding this comment.
oh.. i'm sorry, i just sorted them alphabetically :D :D
| expand: config.expand, | ||
| }); | ||
| expect.extend({toMatchSnapshot, toThrowErrorMatchingSnapshot}); | ||
| (expect: Object).addSnapshotSerializer = addSerializer; |
There was a problem hiding this comment.
We should add the addSnapshotSerializer type to Expect so we don't need to cast here.
packages/jest-circus/src/state.js
Outdated
|
|
||
| import type {Event, State, EventHandler} from '../types'; | ||
|
|
||
| const TOP_DESCRIBE_BLOCK_NAME = 'JEST_TOP_DESCRIBE_BLOCK'; |
There was a problem hiding this comment.
Also, move this after the require calls pls.
| makeTestResults, | ||
| } = require('./utils'); | ||
|
|
||
| const run = async (): Promise<TestResults> => { |
| } | ||
| }; | ||
|
|
||
| const _callHook = (hook: Hook, testContext?: TestContext): Promise<any> => { |
There was a problem hiding this comment.
Why is context optional here?
My suggestion:
- Rename to
context. - Make
contextthe first arg. - Make it non-optional (if possible)
There was a problem hiding this comment.
it's optional because beforeAll and afterAll are shared between multiple tests and can't have a context object. Also we're removing it anyway :)
There was a problem hiding this comment.
This is really good, fantastic work. It's so simple and clean and I'm looking forward to building on top of this.
Feel free to go through my nits and then merge this PR. Here are some follow-up questions:
- What is the compatibility layer to Jasmine gonna look like?
- What is the proposal for passing context/state into test functions so we can make everything less magical and make concurrent tests work with snapshots etc.?
- What's your rollout plan? Are cutting over Jest 21 to this framework by default?
|
aaronabramov
force-pushed
the
jest-framework-1
branch
from
c50b356 to
705a799
Compare
|
alright. I addressed most of the issues and couldn't come up with a better name than circus :'( |
* [WIP] jest-framework [WIP] jest-framework [WIP] jest-framework [WIP] Integrating with Jest [WIP] jest-framework [WIP] jest-framework [WIP] jest-framework * afterAll hook fix * framework -> circus
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
I apologize for the large PR. I spent 8 hours on the plane with no wifi :)
This PR replaces Jasmine with
jest-circuswhy
Jasmine always made it hard for us to move fast. Since we don't own the codebase, it is hard to introduce new features, fix existing bugs, make design changes and just debug the code. On top of that Jasmine's codebase is not flow typed, which make the integration harder.
The goal of this PR is to replace Jasmine with a framework that mirrors the functionality, but at the same time simplifies the things as much as possible.
Design
This new framework is built using FLUX architecture (pretty much Redux, but with mutable data).
jest-circus/src/state.jswhere it defines the initial state.jest-circus/src/index.jsjest-circus/src/eventHandler.jsjest-circus/types.jsBenefits of the approach
Preventing errors. We can be more strict in our test runners. For example here https://github.com/facebook/jest/blob/master/packages/jest-matchers/src/__tests__/toThrowMatchers-test.js#L114-L130 we define
itinside anotherit. Jasmine won't even complain about it, and just skip this test, but in fact, after i fixed the definition of this test, i found that this test is actually broken. There is many other bad things that we'll be able to prevent during the runtime (examples are: nested before/after hooks, adding describe/test/beforeEach during the test run, etc...)Debugging. Having a single event handler, we can add a single
console.logstatement to it and see everything what happens in our test run.example:
Previously, every time we have a broken test that stalls i would spend hours trying to understand where exactly it happens.TODO:
All tests are currently passing.
To run the test suite with the new framework run:
Although, there are a few cases that we don't have tests for that i still need to fix:
legacy_code_todo_rewrite/folder)describe, test, beforeEach, ...is used when the tests are already runningconsole,sourceMapvalues toTestResulttest.concurrent--testNamePattern