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

Replace Jasmine with jest-circus #3668

Merged
merged 3 commits into from Jun 7, 2017

Conversation

Projects
None yet
10 participants
@aaronabramov
Member

aaronabramov commented May 27, 2017

I apologize for the large PR. I spent 8 hours on the plane with no wifi :)

This PR replaces Jasmine with jest-circus

why

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).

  • The core data model of this framework is located in jest-circus/src/state.js where it defines the initial state.
  • The API is exposed in jest-circus/src/index.js
  • None of the API functions work with the state directly, but rather dispatch actions that describe "what happened in the framework"
  • everything that happens in the framework is handled in jest-circus/src/eventHandler.js
  • Every piece of data that flows through the framework is typed and all types located here jest-circus/types.js

Benefits of the approach

  1. 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 it inside another it. 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...)

  2. Debugging. Having a single event handler, we can add a single console.log statement to it and see everything what happens in our test run.
    example:

screen shot 2017-05-26 at 5 59 09 pm

Previously, every time we have a broken test that stalls i would spend hours trying to understand where exactly it happens.
  1. Extensibility. Because we don't have control over Jasmine, adding features to it is pain. Snapshot functionality is built on hacks and monkeypatches. And every new feature is harder and harder to add (we have a plan to introduce interactive snapshot updating). But with FLUX architecture all we need to do is to add another handler. e.g. snapshot logic looks like this right now:
const {addEventHandler} = require('jest-circus');

const eventHandler = event => {
  switch (event.name) {
    case 'test_start': {
      setExpectState({currentTestName: getTestID(event.test)});
      break;
    }
    case 'test_success':
    case 'test_failure': {
      addSnapshotErrorsToTestResult(event.test);
      break;
    }
  }
};

addEventHandler(eventHandler);

TODO:

All tests are currently passing.
To run the test suite with the new framework run:

./jest --testRunner '<rootDir>/packages/jest-circus/build/legacy_code_todo_rewrite/jest-adapter.js'

Although, there are a few cases that we don't have tests for that i still need to fix:

  • Write better integration with the rest of Jest (remove/refactor old code in legacy_code_todo_rewrite/ folder)
  • Throw if describe, test, beforeEach, ... is used when the tests are already running
  • Add configurable timeouts
  • add console, sourceMap values to TestResult
  • Make sure all requires within framework are internal and required once per worker only when needed?
  • test.concurrent
  • filter tests when run with --testNamePattern
  • snapshot files in tests aren't being cleaned up properly
  • Assertion counts

@aaronabramov aaronabramov changed the title from [WIP] Replace Jasmine to Replace Jasmine May 27, 2017

@cpojer

This comment has been minimized.

Contributor

cpojer commented May 30, 2017

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 (@jest-test-runner <name>) or using an array of globs in the config. What do you think?

More thorough review coming later this week.

beforeAll,
beforeEach,
describe,
it,

This comment has been minimized.

@fdaciuk

fdaciuk May 30, 2017

Is that mean we'll be able to import it instead of use the global one? ❤

This comment has been minimized.

@aaronabramov

aaronabramov May 30, 2017

Member

yeah! i think that will be much easier to do now :)

@yangshun

Apologies for the nitpicky comments on spelling. Feel free to ignore if they aren't important.

}
};
// Get suppressed errors form jest-matchers that weren't throw during

This comment has been minimized.

@yangshun

yangshun May 30, 2017

Member

I think there's a typo: from. And there's an extra space after form.

if (!fn) {
mode = 'skip'; // skip test if no fn passed
} else if (!mode) {
// if not set exlicitly, inherit from its parent describe

This comment has been minimized.

@yangshun

yangshun May 30, 2017

Member

Typo: explicitly.

testContext: TestContext,
{isHook}: {isHook?: boolean} = {isHook: false},
): Promise<any> => {
// If this fn accepts `done` callback we return a promise that fullfils as

This comment has been minimized.

@yangshun

yangshun May 30, 2017

Member

Typo: fulfills

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`, () => {

This comment has been minimized.

@yangshun

yangshun May 30, 2017

Member

Not sure if this was mistakenly changed to when; it was with previously, like the other tests here.

@ulrikstrid

This comment has been minimized.

ulrikstrid commented May 30, 2017

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?

@cpojer

This comment has been minimized.

Contributor

cpojer commented May 30, 2017

Yep, it'll get published separately.

@aaronabramov

This comment has been minimized.

Member

aaronabramov commented May 30, 2017

@yangshun thanks for catching that!

@aaronabramov aaronabramov force-pushed the aaronabramov:jest-framework-1 branch from 006d003 to 6887e1b May 30, 2017

@Daniel15

This comment has been minimized.

Member

Daniel15 commented May 31, 2017

I spent 8 hours on the plane with no wifi

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 😛

Having a single event handler

Will this make it easier to attach custom reporters? For example, we're using an AppVeyor reporter to properly show test runs in AppVeyor:

jest/testSetupFile.js

Lines 15 to 18 in 9b157c3

if (process.env.APPVEYOR_API_URL) {
// Running on AppVeyor, add the custom reporter.
jasmine.getEnv().addReporter(new jasmineReporters.AppVeyorReporter());
}
. 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.

@aaronabramov

This comment has been minimized.

Member

aaronabramov commented May 31, 2017

@Daniel15 i haven't looked into how jasmine-reporters work yet, but i assume it reports (sends a POST request?) for each test file separately?

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:

  1. from a single test file. This will report info about how many tests passed/failed/skipped within a single *-test.js file.
  2. from Jest process (aggregated results). This will report the same data as # 1, but for each -test.js. So it'll report things like: how many tests failed/passed/skipped across all of *-test.js files, how many *.-test.js files sucessfully ran, how many -test.js files had failed test or execution failures.

I believe that 99% of the time we want to use the second type of reporting, and that was enabled by #3349
I see reporting from Jasmine (or now jest-framework) to be useful mostly for debugging purposes. Like sticking a console.log into dispatcher and seeing what's going on in the run step-by-step

@Daniel15

This comment has been minimized.

Member

Daniel15 commented May 31, 2017

@dmitriiabramov - looks like jasmine-reporters' AppVeyor reporter caches the results internally and batches them 50 at a time when calling AppVeyor's API: https://github.com/larrymyers/jasmine-reporters/blob/master/src/appveyor_reporter.js#L223-L225. On the other hand, the XUnit implementation does no batching and sends them one by one: https://github.com/xunit/xunit/blob/560c77c3a48571fef3e4f129d93107f57b673261/src/xunit.runner.reporters/AppVeyorClient.cs#L103

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.

@ArtemGovorov

This comment has been minimized.

Contributor

ArtemGovorov commented May 31, 2017

@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:

  • jasmineStarted (not equivalent to the custom reporter's onRunStart because the former happens after loading the test file unlike the latter, which is an important difference for us)
  • suiteStarted,
  • suiteDone,
  • specStarted,
  • specDone (with the access to the matcherName, actual error object, actual and expected objects; all these are required to provide custom reporting, like custom coloured diffs, etc).

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).

@aaronabramov

This comment has been minimized.

Member

aaronabramov commented May 31, 2017

@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 actual, expected objects, but this part wasn't being populated since we rewrote jasmine matchers (back in august 2016), so most likely it won't be a problem

@Daniel15

This comment has been minimized.

Member

Daniel15 commented May 31, 2017

probably even create an adapter that'll keep the interface consistent with all your custom reporters that you're using right now

This is a really good idea - Maintain backwards compatibility with Jasmine reporters by including an adapter that exposes the old Jasmine API.

@ArtemGovorov

This comment has been minimized.

Contributor

ArtemGovorov commented May 31, 2017

@dmitriiabramov Great, thanks for confirming it!
Regarding actual and expected objects, they are actually passed right now. Late 2016 I have submitted 2 PRs that were merged:

and now matcherResult (with expected and actual) is a part of the error object which gets passed from jasmine's specDone in the list of failedExpectations.
Here are the unit tests for this: https://github.com/facebook/jest/pull/2572/files

@ArtemGovorov

This comment has been minimized.

Contributor

ArtemGovorov commented May 31, 2017

@dmitriiabramov So as long as the error object (that is important for us and I guess other reporters as well, because of the access to its stack) is passed in the new framework's specDone equivalent, it's all great.

@aaronabramov

This comment has been minimized.

Member

aaronabramov commented May 31, 2017

@ArtemGovorov oh wow! yeah, that totally makes sense. and yes, the test_failed event accepts a Test object that has errors: Array<Exception> property :) so it shouldn't cause you too much trouble!

@@ -0,0 +1,16 @@
{
"name": "jest-framework",

This comment has been minimized.

@cpojer

cpojer Jun 1, 2017

Contributor

We need to discuss naming…

@cpojer

This comment has been minimized.

Contributor

cpojer commented Jun 1, 2017

The changes you made unrelated to the new test runner, can you send a separate PR for those cleanups/fixes?

@aaronabramov aaronabramov force-pushed the aaronabramov:jest-framework-1 branch from 6887e1b to 7614749 Jun 1, 2017

[WIP] jest-framework
[WIP] jest-framework

[WIP] jest-framework

[WIP] Integrating with Jest

[WIP] jest-framework

[WIP] jest-framework

[WIP] jest-framework

@aaronabramov aaronabramov force-pushed the aaronabramov:jest-framework-1 branch from 7614749 to 657f967 Jun 1, 2017

@aaronabramov

This comment has been minimized.

Member

aaronabramov commented Jun 1, 2017

all PRs with changes affecting the rest of Jest were merged and this one is ready to go.
Still not sure about the naming though :)

@codecov-io

This comment has been minimized.

codecov-io commented Jun 2, 2017

Codecov Report

Merging #3668 into master will decrease coverage by 2.66%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
packages/jest-circus/src/run.js 0% <0%> (ø)
packages/jest-circus/src/utils.js 0% <0%> (ø)
packages/jest-runtime/src/index.js 76.37% <0%> (ø) ⬆️
.../src/legacy_code_todo_rewrite/jest-adapter-init.js 0% <0%> (ø)
...circus/src/legacy_code_todo_rewrite/jest-expect.js 0% <0%> (ø)
packages/jest-circus/src/index.js 0% <0%> (ø)
...ircus/src/legacy_code_todo_rewrite/jest-adapter.js 0% <0%> (ø)
packages/jest-circus/src/eventHandler.js 0% <0%> (ø)
packages/jest-circus/src/state.js 0% <0%> (ø)
packages/pretty-format/src/plugins/HTMLElement.js 89.18% <0%> (-5.1%) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b254715...705a799. Read the comment docs.

@aaronabramov aaronabramov force-pushed the aaronabramov:jest-framework-1 branch from 7afd7e1 to 86b00ca Jun 2, 2017

@cpojer cpojer changed the title from Replace Jasmine to Replace Jasmine with jest-circus Jun 2, 2017

@aaronabramov aaronabramov force-pushed the aaronabramov:jest-framework-1 branch from 8c59b50 to c50b356 Jun 2, 2017

const _formatError = (error: ?Exception): string => {
if (!error) {
return 'NO ERROR MESSAGE OR STACK TRACE SPECIFIED';

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

How is that possible?

This comment has been minimized.

@aaronabramov

aaronabramov Jun 7, 2017

Member
throw undefined

:(

@@ -0,0 +1,225 @@
/**

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

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.

This comment has been minimized.

@aaronabramov

aaronabramov Jun 7, 2017

Member

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:

  1. are mostly stable and don't need to be changed
  2. have too much noise (if/else, branching, loops, etc.)
  3. 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 = (

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

typo: getEachHook

This comment has been minimized.

@aaronabramov

aaronabramov Jun 7, 2017

Member

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));

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

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.

This comment has been minimized.

@aaronabramov

aaronabramov Jun 7, 2017

Member

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) => {

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

why not use async/await here?

This comment has been minimized.

@Kovensky

Kovensky Jun 5, 2017

This is specifically for converting a nodeback to Promise so can't, unless you use util-promisify.

This comment has been minimized.

@aaronabramov

aaronabramov Jun 7, 2017

Member

yeah. this function is an adapter between async/await world and done() world

const {status} = test;
if (!status) {
console.log(test);

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

remove this?

if (!status) {
console.log(test);
throw new Error('status should be present after tests are run');

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

"Status" and end the sentence in .. :)

titles.unshift(parent.name);
} while ((parent = parent.parent));
titles.shift(); // remove TOP_DESCRIBE_BLOCK_NAME

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

Is this lifted from Jasmine? Do we need this in circus?

This comment has been minimized.

@aaronabramov

aaronabramov Jun 7, 2017

Member

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`,

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

Sentence formatting please!

break;
}
case 'run_start': {
state.testTimeout = global[TEST_TIMEOUT_SYMBOL] || state.testTimeout;

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

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);

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

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).

This comment has been minimized.

@aaronabramov

aaronabramov Jun 7, 2017

Member

oh.. i'm sorry, i just sorted them alphabetically :D :D

expand: config.expand,
});
expect.extend({toMatchSnapshot, toThrowErrorMatchingSnapshot});
(expect: Object).addSnapshotSerializer = addSerializer;

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

We should add the addSnapshotSerializer type to Expect so we don't need to cast here.

import type {Event, State, EventHandler} from '../types';
const TOP_DESCRIBE_BLOCK_NAME = 'JEST_TOP_DESCRIBE_BLOCK';

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

CIRCUS instead of JEST ;)

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

Also, move this after the require calls pls.

makeTestResults,
} = require('./utils');
const run = async (): Promise<TestResults> => {

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

This function is beautiful btw.

}
};
const _callHook = (hook: Hook, testContext?: TestContext): Promise<any> => {

This comment has been minimized.

@cpojer

cpojer Jun 5, 2017

Contributor

Why is context optional here?

My suggestion:

  • Rename to context.
  • Make context the first arg.
  • Make it non-optional (if possible)

This comment has been minimized.

@aaronabramov

aaronabramov Jun 7, 2017

Member

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 :)

@cpojer

cpojer approved these changes Jun 5, 2017

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

This comment has been minimized.

Member

aaronabramov commented Jun 7, 2017

  1. It's probably 95% compatible with Jasmine (there's a few very minor differences)
  2. I want to make test context feature to be compatible with Jasmine for now, and once we get 99% feature parity propose a design to move to a new model
  3. I want to keep both frameworks alive for a while and migrate first Jest's own test suite to use it, second update www to use it and see if there's any unexpected issues. Once we're there i think i'd add an experemental (--circus?) flag to run the new framework and probably make it a default in the next major release

@aaronabramov aaronabramov force-pushed the aaronabramov:jest-framework-1 branch from c50b356 to 705a799 Jun 7, 2017

@aaronabramov

This comment has been minimized.

Member

aaronabramov commented Jun 7, 2017

alright. I addressed most of the issues and couldn't come up with a better name than circus :'(
i'll merge this PR and will be sending a lot of followups to fix things (improve compatibility with jasmine)
This is not used anywhere yet, so it should not affect Jest or any of the tests

@aaronabramov aaronabramov merged commit 7c3cef7 into facebook:master Jun 7, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@aaronabramov aaronabramov deleted the aaronabramov:jest-framework-1 branch Jun 7, 2017

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017

Replace Jasmine with jest-circus (facebook#3668)
* [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment