Jest implementation and test on counter example #324

LucasPerso · 2017-04-04T15:28:17Z

Should solve #324

christophehurpeau · 2017-04-05T07:07:56Z

config/babelrc.js

@@ -28,6 +29,7 @@ export default (env, options) => ({
    plugins: [
        // Make optional the explicit import of React in JSX files
        pluginReactRequire,
+        pluginTransformRuntime,


That's a good question. It should work without it @LucasPerso

@johangirod We tried to remove it and it did not transpile tests written in ES6

Can we test it again ? Maybe it was for another reason ? And in any case, we should at least add it only for the test environment.

christophehurpeau · 2017-04-05T07:11:21Z

bin/vitamin.js

@@ -6,15 +6,17 @@ import path from 'path';
 import rimraf from 'rimraf';
 import { spawn } from 'child_process';
 import fs from 'fs';
+import { spawn as npmRunSpawn } from 'npm-run';


Why not use yarn run with process_child.spawn ?

Using npm-run guarantees us that it will run jest from vitamin's node_modules. If we don't use it, it can causes issues when jest is called from another place (such as linked dependencies or global install)

Well, I wasn't a big fan of using a test script inside the package.json of vitaminjs, because, in this context, we are not using it for testing vitaminjs but for testing the application itself.
But, now that you're pointing it, I realised that we could use the yarn jest command, that comes directly with yarn, without needing to modify the package.json, and with no extra dependencies.

What do you think, @LucasPerso @christophehurpeau ?

alepee · 2017-04-05T09:25:29Z

config/jest/jestrc.js

+    },
+    transformIgnorePatterns: ['/node_modules/(?!vitaminjs).*'],
+    moduleNameMapper: {
+        '\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$': '<rootDir>/__mocks__/fileMock.js',


Is it the same than url-loader extensions?
https://github.com/Evaneos/vitaminjs/blob/master/config/build/webpack.config.common.js#L71

Yeup, it should be the same as url loader. Maybe we can extract them into a variable and import it into the url-loader and here ?

alepee · 2017-04-05T09:26:59Z

config/jest/jestrc.js

+    transformIgnorePatterns: ['/node_modules/(?!vitaminjs).*'],
+    moduleNameMapper: {
+        '\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$': '<rootDir>/__mocks__/fileMock.js',
+        '\\.(css|less)$': 'identity-obj-proxy',


Why do we support less there OR why do we support only css and less

That's a pretty good question :)

Since we do not support others stylesheets than CSS, I suggest we keep CSS only.

alepee · 2017-04-05T09:29:24Z

package.json

@@ -4,7 +4,7 @@
  "description": "The build toolchain against JavaScript Fatigue",
  "main": "./src/shared/index.js",
  "scripts": {
-    "test": "yarn run lint",
+    "test": "yarn run jest",


so yarn is a dependence of vitamin?
wouldn't be better to start test with ./node_modules/.bin/jest …

christophehurpeau · 2017-04-05T10:05:12Z

bin/vitamin.js

-    commonBuild(webpackConfigTest, 'tests', { hot, dev: DEV }, launchTest);
+const test = (args) => {
+    spawn(
+        vitaminResolve('node_modules', '.bin', 'jest'),


should be appResolve, no?

christophehurpeau · 2017-04-05T10:05:56Z

package.json

    "isomorphic-style-loader": "^1.1.0",
+    "jest": "^19.0.2",


jest must be a peer dependency, else it won't be in .bin if I remember right

dependencies binaries are included in ./node_modules/.bin as well.
@LucasPerso @johangirod confirmation?

@alepee yarnpkg/yarn#760

But we can consider going to node_modules/jest/bin/jest.js instead :)

or installing jest-cli with node_modules/jest-cli/bin/jest.js

ok you mean in my_project/node_modules/.bin i thought to resolve it directly from my_project/node_modules/vitaminsjs/node_modules/.bin with vitaminResolve
but we already having this conversation ^^

@christophehurpeau, correct me if I'm mistaken, but the issue explicitely state that

[CLI dependencies] will be stored inside {A}/node_modules/B/node_modules/.bin/*

If we use npmRun, the issue is already taking care of, as we are going to search in vitaminjs/node_modules, and if not found resolve to the next parent directory.

christophehurpeau · 2017-04-05T10:06:54Z

.eslintrc.json

@@ -2,7 +2,8 @@
    "extends": "airbnb",
    "parser": "babel-eslint",
    "env": {
-        "es6": true
+        "es6": true,
+        "jest": true


perhaps you can only say you're in a jest env in the test directory ?

👍
I think it should be removed from here, and added at the discretion of the test files. Either by adding a .eslintrc.json inside the test folder, or simply by adding

/* eslint env jest */

at the beginning of the test file.

👍 let's do that, it will make more sense since env:jest is not true most of the time

johangirod · 2017-04-05T10:07:35Z

bin/vitamin.js

-    npmRunSpawn(
-        'jest',
+    spawn(
+        vitaminResolve('node_modules', '.bin', 'jest'),


Mmmmh, it's not guaranteed to work, and it should not work if jest is not installed inside vitaminJS.
It's better to use just spawn(yarn jest), and then, add
'--' before the list of arguments.

But in the same time, I'm not sure it needs to be changed.
The advantage of npmRunSpawn over yarn jest beeing:

It does not require yarn to be globally installed to work

It spawn only one child process, unlike yarn, who will spawn one for itself and one for the command

@christophehurpeau ?

running (yarn || npm) run jest requires the user to add "jest": "jest" in its scripts. It's better to call node_modules/.bin/jest I think

In any case, yarn jest does not seem to work even if yarn is installed globally.

👍 yarn should not be required to use any part of vitamin.

and i agree with you @christophehurpeau, seems better to use directly vitamin's node_modules/.bin/jest since we ship with it. (i don't know why yet, but it seems to have some drawbacks

node_modules/.bin/jest

The problem is that jest can be installed inside vitaminJS node_modules (in the case of a link, or when the user also installed jest with another version), or inside the app node_modules (in case of standard installation).

If we call node_modules/.bin/jest we should resolve "à la" npm, in order to prevent issues with that.
That's exactly what npmRun.spawn does.

when you resolve it with npmRun what is the sequence with local, global and dependencies?

LucasPerso · 2017-04-05T10:09:43Z

package.json

@@ -4,7 +4,6 @@
  "description": "The build toolchain against JavaScript Fatigue",
  "main": "./src/shared/index.js",
  "scripts": {
-    "test": "yarn run lint",


This script should be use in the future for running test on vitaminJS.

Yes, but it will be something like

"test": "yarn run lint && yarn jest",

So no need to remove it, especially since it's use by circleci :)

My bad then ! I'll add it again ;)

I'm not sure if it's a good practice to lint when you run test...

should be circleci to invoke the first one then the second no?

johangirod · 2017-05-10T13:45:46Z

Todo :

Remove breaking
Add a vitaminjs-jest package which acts as a feature flag
Add a plugin key which can contains vitaminjs-plugin-jest

LucasPerso · 2017-05-11T11:56:51Z

package.json

    "babel-eslint": "^7.2.1",
    "eslint": "^3.19.0",
    "eslint-config-airbnb": "^14.1.0",
    "eslint-plugin-import": "^2.2.0",
    "eslint-plugin-jsx-a11y": "^4.0.0",
    "eslint-plugin-react": "^6.10.3",
+    "lerna": "^2.0.0-rc.4",


Y a deux fois Lerna :)

LucasPerso · 2017-05-11T12:00:33Z

packages/vitaminjs-plugin-jest/README.md

+
+There you go. You can launch jest with
+```
+vitaminjs test


C'est sensé être vitamin test si je ne m'abuse

Nop, pour l'instant vitamin cli est installé sous vitaminjs :(
https://github.com/Evaneos/vitaminjs-cli/blob/master/package.json#L7

Ca va changer, mais en attendant, je met ça comme ça :)

LucasPerso · 2017-05-11T14:18:25Z

Moved to #364

LucasPerso force-pushed the add-jest branch 3 times, most recently from a46c1e4 to 573fa1f Compare April 4, 2017 15:34

LucasPerso requested review from christophehurpeau, alepee and johangirod April 4, 2017 15:36

LucasPerso modified the milestone: 2.0 Apr 4, 2017

christophehurpeau reviewed Apr 5, 2017

View reviewed changes

alepee suggested changes Apr 5, 2017

View reviewed changes

LucasPerso force-pushed the add-jest branch from 573fa1f to 852da8e Compare April 5, 2017 09:46

christophehurpeau reviewed Apr 5, 2017

View reviewed changes

johangirod reviewed Apr 5, 2017

View reviewed changes

LucasPerso commented Apr 5, 2017

View reviewed changes

alepee approved these changes Apr 7, 2017

View reviewed changes

johangirod force-pushed the add-jest branch from 6d8aadd to d138831 Compare May 11, 2017 10:58

feat: add jest implementation

013c6ab

LucasPerso commented May 11, 2017

View reviewed changes

Lucas and others added 6 commits May 11, 2017 14:19

test(examples): add tests on Counter example

5cc632f

refactor(jest): code review

dcb3138

chore: share same regex between url-loader and jest moduleNameMapper

7df01c8

chore(eslint): remove jest env

f839264

chore: rollback npmRun.spawn following code review

86a9667

chore: use lerna architecture for repo

5bb12ff

johangirod force-pushed the add-jest branch from d138831 to c72f9d3 Compare May 11, 2017 13:16

feat(test): allow to use vitamin-plugin-jest for testing with jest

01ce7e2

johangirod force-pushed the add-jest branch from c72f9d3 to 01ce7e2 Compare May 11, 2017 13:32

fix(test): add a bootstrap for lerna

86cefed

LucasPerso closed this May 11, 2017

LucasPerso deleted the add-jest branch May 12, 2017 09:18

Jest implementation and test on counter example #324

Jest implementation and test on counter example #324

Conversation

LucasPerso commented Apr 4, 2017 • edited by johangirod 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

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

johangirod Apr 5, 2017 • 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

christophehurpeau Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alepee Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

johangirod Apr 5, 2017 • 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

johangirod commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasPerso commented May 11, 2017

LucasPerso commented Apr 4, 2017 •

edited by johangirod

Loading

johangirod Apr 5, 2017 •

edited

Loading

christophehurpeau Apr 5, 2017 •

edited

Loading

alepee Apr 5, 2017 •

edited

Loading

johangirod Apr 5, 2017 •

edited

Loading