Use Jest as test runner #744

thangngoc89 · 2016-09-14T12:24:21Z

WARNING: Huge diffs

thangngoc89 · 2016-09-15T11:21:13Z

Well, there are 2 integration tests in integration-tests/cli.js that need to be fixed. And I have no idea how to fix them. I'll try to spawn those process instead of execute them

thangngoc89 · 2016-09-15T15:15:10Z

Help wanted:

So I have a test file that execute phenomic cli here

That script has require("babel-register") inside it

But when I run the test, it throws error about Syntax Error on spread operator

https://travis-ci.org/MoOx/phenomic/jobs/159970393#L744-L751

I can run that external script manually without a problem

So my question is: Does Jest interfere with babel-register or something ?

To reproduce the error message:

git clone git@github.com:MoOx/phenomic.git
cd phenomic
git checkout jest
npm install
npm run test-phenomic-theme-base

thangngoc89 · 2016-09-15T20:12:33Z

@MoOx turn out, the whole problem is because Jest set process.env.NODE_ENV === "test" and we have a proper babel setting for test env.

I see development and production is duplicated here

Can I move them to default babel config?

MoOx · 2016-09-15T20:15:13Z

No you cannot. Just duplicate again to "test". if you move to global conf, this will make duplicate for webpack-*. I have written a blog post about this mess, will publish this soon ;)

thangngoc89 · 2016-09-15T20:16:55Z

Hum. If that is the case, it's really a mess. Babel should have an option that allow us to choose when and when not to extend babel config.

FYI, I'll set NODE_ENV to development than ;)

MoOx · 2016-09-15T20:18:04Z

You can duplicate, it's just for us, this won't affect docs & boilerplate setup. ANyway it's just a 3 lines section.

thangngoc89 · 2016-09-15T20:19:57Z

Yeah. I see. Now that problem was fixed, we have a bigger problem.

Error: listen EADDRINUSE :::8081

Bed time, will look in the in tmr.

thangngoc89 · 2016-09-15T20:46:02Z

All's green (local). Ready for review. Now I can run the hold test suite without freezing my PC.

MoOx

I have made some comment but I will need to do that in multiple passes as it's too huge. I don't like expect api at all and I think we should do this using all api to minimize diff. We are going to miss something and I don't want that :(

MoOx · 2016-09-15T20:54:31Z

.gitignore

@@ -13,7 +13,7 @@ coverage
 lib

 # tests results
-**/__tests__/_output*
+**/__tests__/output*


Can you do the opposite?

MoOx · 2016-09-15T20:54:54Z

.lgtm

@@ -0,0 +1,2 @@
+approvals = 1
+pattern = "(?i):shipit:|:\\+1:|LGTM"


Please rebase correctly and remove this file (have been removed yesterday)

MoOx · 2016-09-15T20:55:44Z

integration-tests/build-result.js

@@ -1,11 +1,11 @@
-import test from "ava"


why did you move tests folder? I don't like this name a lot. We might find something better in the future.

This is me trying to make jest run test from this folder only. See jestjs/jest#1696

I'll rename it back

MoOx · 2016-09-15T20:57:01Z

package.json

-    "tests": "ava",
-    "#integration-tests": "needs this order (index test the build, and cli is cleaning it)",
-    "integration-tests": "ava __tests__/index.js && ava __tests__/cli.js",
+    "integration-tests": "jest --config '{\"testRegex\": \"integration-tests\", \"setupTestFrameworkScriptFile\": \"<rootDir>/scripts/jest-framework-setup.js\"}'",


Not a fan at all of this inline config. We cannot just have a file in this tests folder?

Just realize that jest accept a file path for configuration. Will fix this.

MoOx · 2016-09-15T20:57:26Z

package.json

-    "coverage": "nyc report --reporter=text-lcov | coveralls",
+    "all-tests": "cross-env TESTING=1 jest --coverage",
+    "postall-tests": "npm run test-phenomic-theme-base && npm run docs",
+    "test": "cross-env TESTING=1 jest",


Maybe we should rely on NODE_ENV=test instead of TESTING

There is 1 problem with using NODE_ENV=test. The configurator changes NODE_ENV when we use --production flag. And it kills jest

MoOx · 2016-09-15T21:06:39Z

src/static/to-html/__tests__/index.js

@@ -31,8 +31,6 @@ test("don't break if there is nothing to transform", async (t) => {
 })

 test("writeAllHTMLFiles", (t) => {
-  t.plan(3)
-


You cannot just remove that. We need to be sure all assertions are called.

MoOx · 2016-09-15T21:08:15Z

src/static/routes-to-urls/__tests__/index.js

+    routesToUrls(routesNoMatches, collection, { log })
+
+    expect(log).toBeCalled()
+    expect(log.mock.calls[0][0]).toContain(


That's why I don't like expect. This make us too relying on the test framework :'(

We can pull in expect from npm or chai if you prefer. Jest works great with them. But I am not sure about pretty diff.

I would prefer to get a similar api as what we have currently. jest-ava-api might be still used and improved :)

Hum... What are you suggesting? AVA doesn't have this built-in. We used sinon for this job.

Here for example, we were passing a callback directly. No need to use jest mock since you can pass a log function. I am just not sure how to handle the t.plan pattern.

Yup. We need t.plan or similar to do callback testing like this

MoOx · 2016-09-15T21:08:53Z

src/redux/modules/__tests__/pages.js

@@ -25,7 +25,7 @@ test("should have action to handle get", async (t) => {
 })

 test("should have action to handle refresh", async (t) => {
-  t.plan(3)
+  // t.plan(3)


Not an an option. We need to handle that somehow.

Yup. That's why I commented out instead of deleted it

MoOx · 2016-09-15T21:09:04Z

src/redux/modules/__tests__/pages.js


 import * as module from "../pages"
 import reducer from "../pages"

 process.env.PHENOMIC_USER_PATHNAME = "/"

 test("should have action to handle get", async (t) => {
-  t.plan(3)
+  // t.plan(3)


Not an an option. We need to handle that somehow.

Yup. That's why I commented out instead of deleted it

There is no reference in Jest's docs for this kind of validation.

MoOx · 2016-09-15T21:10:35Z

src/loader/__tests__/index.js

+          }
+        })
+        // t.plan(2+5) // 2, err, warn, 5   => array
+        // t.end()


How is this check made?

Yup. That's why I commented out instead of deleted it

MoOx · 2016-09-16T06:19:57Z

src/bin/commands/setup/__tests__/inquirer.js

-      t.pass()
-      t.end()
-    }
+describe("bin > commands > setup > inquirer", () => {


are describe mandatory? What are they for?

nope. I removed them

MoOx · 2016-09-16T06:23:13Z

src/static/routes-to-urls/__tests__/index.js

+    routesToUrls(routesNoMatches, collection, { log })
+
+    expect(log).toBeCalled()
+    expect(log.mock.calls[0][0]).toContain(


Here for example, we were passing a callback directly. No need to use jest mock since you can pass a log function. I am just not sure how to handle the t.plan pattern.

thangngoc89 · 2016-09-16T22:29:04Z

I gave up on this branch. If you take this over, check out f4b13e4 and starts from there

MoOx · 2016-10-24T20:19:23Z

I am finishing this.

MoOx · 2016-10-28T19:58:25Z

I don't like having to add a babel env test section in base theme but I will find a way to hide all this config behind a preset before cutting a release.

thangngoc89 force-pushed the jest branch from 112a1ec to 9d447e6 Compare September 14, 2016 18:52

thangngoc89 mentioned this pull request Sep 14, 2016

While running as root .gz files gets created on working directory #545

Closed

thangngoc89 force-pushed the jest branch 2 times, most recently from 72ffbea to d7030ba Compare September 15, 2016 20:41

thangngoc89 added the status: review needed label Sep 15, 2016

thangngoc89 force-pushed the jest branch from de799f5 to 4db08d1 Compare September 15, 2016 21:12

MoOx suggested changes Sep 15, 2016

View reviewed changes

MoOx reviewed Sep 16, 2016

View reviewed changes

thangngoc89 force-pushed the jest branch 2 times, most recently from e8e0f0f to 41b25c3 Compare September 16, 2016 21:47

MoOx added needs changes and removed status: review needed labels Sep 28, 2016

MoOx mentioned this pull request Oct 8, 2016

Coverage is broken... #815

Closed

MoOx self-assigned this Oct 24, 2016

Use Jest as test runner

1af9ab4

MoOx force-pushed the jest branch 5 times, most recently from c13d849 to 2d40339 Compare October 26, 2016 20:18

MoOx force-pushed the jest branch 5 times, most recently from dfba0de to 1ee2185 Compare October 27, 2016 06:39

Use Jest, round 2

39f0aec

MoOx force-pushed the jest branch from e5383fa to 39f0aec Compare October 27, 2016 13:08

MoOx added status: accepted and removed needs changes labels Oct 28, 2016

MoOx merged commit f8ab913 into master Oct 28, 2016

MoOx deleted the jest branch October 28, 2016 19:58

		@@ -0,0 +1,2 @@
		approvals = 1
		pattern = "(?i):shipit:\|:\\+1:\|LGTM"

Use Jest as test runner #744

Use Jest as test runner #744

Conversation

thangngoc89 commented Sep 14, 2016 • edited Loading

thangngoc89 commented Sep 15, 2016

thangngoc89 commented Sep 15, 2016

thangngoc89 commented Sep 15, 2016

MoOx commented Sep 15, 2016

thangngoc89 commented Sep 15, 2016

MoOx commented Sep 15, 2016

thangngoc89 commented Sep 15, 2016

thangngoc89 commented Sep 15, 2016 • edited Loading

MoOx left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thangngoc89 Sep 15, 2016 • edited Loading

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

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

thangngoc89 commented Sep 16, 2016 • edited Loading

MoOx commented Oct 24, 2016

MoOx commented Oct 28, 2016

thangngoc89 commented Sep 14, 2016 •

edited

Loading

thangngoc89 commented Sep 15, 2016 •

edited

Loading

thangngoc89 Sep 15, 2016 •

edited

Loading

thangngoc89 commented Sep 16, 2016 •

edited

Loading