Switch to Jest #635

evocateur · 2017-03-02T03:22:51Z

Jest rocks my socks. It's faster than mocha+nyc, and more accurate in its coverage report.

The --watch mode is a legit dream. (yarn test -- --watch)

Non-scientific comparisons:

	mocha	jest
`yarn test`	15s	7s
`yarn run coverage`	20s	8s

You'll notice files we've completely forgotten to cover, which are entirely absent from the nyc output. Have I mentioned it's fast? 🚀

evocateur · 2017-03-02T03:29:35Z

CI all failed for a variety of reasons 😭

spudly · 2017-03-02T13:40:06Z

+1 on this. When my team switched from AVA to jest our test execution time for a single (large) package went from 5 minutes to 7 seconds.

bcoe · 2017-03-11T23:57:46Z

@evocateur this sounds great. Jest uses the same underlying instrumentation logic as nyc, but doesn't rely on source-maps (we've never quite hammered all the kinks out of the source-maps in babel codebases).

evocateur · 2017-03-13T16:56:29Z

@bcoe Indeed! I'm a big fan of nyc and its underlying utilities.

evocateur · 2017-03-14T23:48:49Z

appveyor.yml

@@ -23,7 +23,7 @@ test_script:
  - node --version
  - npm --version
  - yarn --version
-  - yarn run ci
+  - yarn run ci -- --runInBand


Without this, the Windows builds have all sorts of impossible-to-debug race conditions with the filesystem operations (fixture copy, etc). With it, it runs super fast (relative to "normal" multi-process Jest runs on Windows).

evocateur · 2017-03-14T23:49:57Z

bin/lerna.js

@@ -1,11 +1,12 @@
 #!/usr/bin/env node
+"use strict";


I got tired of my editor yelling at me with lint errors whenever I opened this file. Easy enough to start linting it automatically, as we no longer support node < v4.

evocateur · 2017-03-14T23:51:21Z

package.json

-    "ci": "npm run lint && npm run test",
+    "lint": "eslint . --ignore-path .gitignore",
+    "fix": "npm run lint -- --fix",
+    "pretest": "npm run lint -- --cache",


This is pretty big. Contributors that run tests locally shouldn't have to remember to also yarn run lint to avoid silly lint errors breaking their PR builds.

evocateur · 2017-03-14T23:52:43Z

package.json

@@ -51,6 +53,7 @@
  "devDependencies": {
    "babel-cli": "^6.7.5",
    "babel-eslint": "^6.0.2",
+    "babel-jest": "^19.0.0",


Now that we're using yarn in CI, this is less important. Without it, however, npm 2 (the default for node v4) would fail to run tests because babel-jest wasn't a sibling of jest.

evocateur · 2017-03-14T23:53:53Z

package.json

+      "src/**/*.js"
+    ],
+    "modulePathIgnorePatterns": [
+      "<rootDir>/test/fixtures"


Jest does aggressive pre-caching of package.json files, apparently, and this was seriously harshing the mellow of a CI process when booting up the tests. So we ignore it. 😎

evocateur · 2017-03-14T23:55:49Z

src/FileSystemUtilities.js

@@ -21,7 +21,7 @@ export default class FileSystemUtilities {

  @logger.logifyAsync()
  static mkdirp(filePath, callback) {
-    mkdirp(filePath, callback);
+    mkdirp(filePath, { fs }, callback);


Passing graceful-fs to mkdirp is an effort to minimize race conditions when conducting tons of parallel filesystem operations.

(These were especially noticeable because jest by default tries to run as many tests in parallel processes as possible)

evocateur · 2017-03-14T23:56:24Z

src/commands/ImportCommand.js

@@ -38,7 +38,7 @@ export default class ImportCommand extends Command {
    this.targetDir = path.join(targetBase, externalRepoBase);

    try {
-      if (fs.statSync(this.targetDir)) {
+      if (FileSystemUtilities.existsSync(this.targetDir)) {


This didn't need to be a statSync since the result was never used.

evocateur · 2017-03-14T23:57:57Z

test/BootstrapCommand.js

@@ -35,7 +35,7 @@ describe("BootstrapCommand", () => {
      bootstrapCommand.runPreparations();

      bootstrapCommand.runCommand(exitWithCode(0, (err) => {
-        if (err) return done(err);
+        if (err) return done.fail(err);


Apparently Jasmine uses this pattern, contra Mocha.

evocateur · 2017-03-15T00:00:38Z

test/ImportCommand.js

+    let externalDir;
+
+    beforeEach(() =>
+      Promise.resolve()


This started out as a parallel Promise.all(), but the synchronous process.chdir() inside initFixture()'s promise chain made that non-deterministic, hence the ordering.

evocateur · 2017-03-15T00:03:15Z

test/PackageUtilities.js

+  describe("public API", () => {
+    let testDir;
+
+    beforeEach(() => initFixture("PackageUtilities/basic").then((dir) => {


This fixture used to be initialized for every test in this file, despite over half of them doing another fixture init (for filtering) and ignoring this. Seemed wasteful.

…changes

…sync

…dir afterEach resolves in the next tick

… only

…m sucks

cpojer · 2017-03-17T00:44:42Z

Hey, wow, thanks for doing this.

evocateur · 2017-03-17T02:57:54Z

One step closer to dogfooding the whole stack :D

…

On Mar 16, 2017, at 17:44, Christoph Pojer ***@***.***> wrote: Hey, wow, thanks for doing this. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

JaKXz · 2017-04-07T01:38:43Z

@evocateur this is great, well done! I'm curious about one statement though:

more accurate in its coverage report [than nyc + mocha]

AFAIK jest uses a lot of istanbul's libs under the hood for collecting coverage information, especially babel-plugin-istanbul. Could you expand / provide examples on how it's more accurate, and I'm hoping that will help us make nyc more accurate :) thanks!

e.g. for files that are not required in tests, you would use nyc's --all flag to see them included in the coverage report. I agree our documentation leaves something to be desired which I am dedicating cycles to improve ATM so your feedback here would definitely help make my work more effective 😄

evocateur · 2017-04-07T02:01:59Z

@JaKXz Indeed, jest does use istanbul's libs! I didn't mean to disparage nyc at all, but I do recall significant differences in the coverage display, even without --all (or jest's equivalent).

When I spoke to @bcoe about this disparity, I recall he said something about the source maps were causing problems with dynamically-transpiled ES6, or something to that effect. It was a few weeks ago, and I can't remember offhand where the thread was.

More than anything, I think the jest transition yielded more accurate coverage reporting because we are no longer hacking babel-register into mocha, and benefitting from jest's "batteries-included" approach.

bcoe · 2017-04-07T02:38:49Z

@evocateur @JaKXz would love to get to the bottom of the source-map discrepancy; @JaKXz we shoudl make an effort to capture discrepancies when possible, and start to get some failing tests in place on nyc.

lock · 2018-12-27T16:20:50Z

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

evocateur added the WIP label Mar 2, 2017

evocateur force-pushed the jest branch from 14c82f2 to 3ede09b Compare March 4, 2017 02:53

evocateur added Tag: Internal and removed WIP labels Mar 4, 2017

evocateur force-pushed the jest branch 4 times, most recently from 1b89029 to d4da6fc Compare March 8, 2017 02:38

evocateur requested review from gigabo, hzoo and doug-wade March 8, 2017 22:54

evocateur force-pushed the jest branch from 7180e98 to 874eae0 Compare March 10, 2017 02:21

evocateur force-pushed the jest branch 4 times, most recently from b252fa1 to d55e631 Compare March 14, 2017 22:51

evocateur commented Mar 14, 2017

View reviewed changes

evocateur commented Mar 15, 2017

View reviewed changes

Daniel Stockman added 19 commits March 16, 2017 13:58

InitCommand: add tests

671ab45

update upstream test

3ecb1c8

test: done(err) => done.fail(err)

9bd346c

GitUtilities: ignore stdio of isInitialized() call, it's just spam

0dbdf72

deps: add graceful-fs (explicit)

9cf9fd6

FileSystemUtilities: use graceful-fs, pass opts.fs to mkdirp

cebdecb

deps: upgrade rimraf

b30d458

FileSystemUtilities: add statSync()

bf351aa

ImportCommand: use statSync and existsSync from FileSystemUtilities

e495df3

test: use graceful-fs everywhere

e674e87

fixtureUtils: gitInit -> one shell-chained command

1a2922a

fixtureUtils: get CURRENT_SHA outside getTempDir(), because it never …

9a4f364

…changes

fixtureUtils: provide graceful-fs overrides for mkdirpAsync & rimrafA…

517aee9

…sync

ci: run tests serially (--runInBand) to reduce contention

b989ad9

initFixture: beforeEach chdir and return testDir in same tick, but ch…

c0b41f8

…dir afterEach resolves in the next tick

ci: limit jest workers in appveyor to emulate --runInBand for windows…

e2c5c18

… only

ci: haha just kidding windows, --runInBand because your fs parallelis…

5b3a547

…m sucks

migrate upstream tests

25f5d6d

ChildProcessUtilities: more test coverage

d550b5f

evocateur force-pushed the jest branch from ef83b19 to d550b5f Compare March 16, 2017 21:00

evocateur merged commit ed912f4 into lerna:master Mar 16, 2017

evocateur deleted the jest branch March 16, 2017 21:25

evocateur mentioned this pull request Mar 30, 2017

[Internal] Investigate test issues #207

Closed

lock bot locked as resolved and limited conversation to collaborators Dec 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to Jest #635

Switch to Jest #635

evocateur commented Mar 2, 2017

evocateur commented Mar 2, 2017 •

edited

spudly commented Mar 2, 2017

bcoe commented Mar 11, 2017

evocateur commented Mar 13, 2017

evocateur Mar 14, 2017

evocateur Mar 14, 2017

evocateur Mar 14, 2017

evocateur Mar 14, 2017

evocateur Mar 14, 2017

evocateur Mar 14, 2017

evocateur Mar 14, 2017

evocateur Mar 14, 2017

evocateur Mar 15, 2017

evocateur Mar 15, 2017

cpojer commented Mar 17, 2017

evocateur commented Mar 17, 2017 via email

JaKXz commented Apr 7, 2017 •

edited

evocateur commented Apr 7, 2017

bcoe commented Apr 7, 2017

lock bot commented Dec 27, 2018

Switch to Jest #635

Switch to Jest #635

Conversation

evocateur commented Mar 2, 2017

evocateur commented Mar 2, 2017 • edited

spudly commented Mar 2, 2017

bcoe commented Mar 11, 2017

evocateur commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Mar 17, 2017

evocateur commented Mar 17, 2017 via email

JaKXz commented Apr 7, 2017 • edited

evocateur commented Apr 7, 2017

bcoe commented Apr 7, 2017

lock bot commented Dec 27, 2018

evocateur commented Mar 2, 2017 •

edited

JaKXz commented Apr 7, 2017 •

edited