Skip to content
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

Add Jest. #250

Merged
merged 1 commit into from
Aug 1, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ const clientESLintConfig = require('./config/eslint');

module.exports = Object.assign({}, clientESLintConfig, {
env: Object.assign({}, clientESLintConfig.env, {
node: true
node: true,
})
});
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ build
.DS_Store
*.tgz
my-app*
template/src/__tests__/__snapshots__/
3 changes: 2 additions & 1 deletion bin/react-scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ var args = process.argv.slice(3);

switch (script) {
case 'build':
case 'start':
case 'eject':
case 'start':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test command to main package.json that would work with --debug-template? Please see how we switch paths in config/paths.js. This lets us iterate super quickly on the template without running create-react-app every time we want to see if the commands still work.

You can rename the existing test command to e2e.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5652ad5

is this what you had in mind? Because test doesn't have a script associated with it I wrote a quick script to add a package.json that links to the preset, runs the test and ensures a snapshot file was created.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm a bit confused by this.

We only use shell scripts for our own infrastructure (end to end test and release script). We would like all user-ran scripts to be written in JavaScript.

Ideally I'd like to mirror what we do with start and build. Create a JS file in scripts directory called "test.js". It's fine if all it does is call Jest.

Then set that script as "test" command in package.json, identical to how we do build and start. This lets us work on Create React App locally without creating apps all the time. We just run those commands in the root folder, and they work (try npm start).

Finally, npm test needs to be a part of end to end test. To be clear e2e is our test that ensures Create React App works. It runs npm start and npm run build in root folder to test our development environment, then creates an app and runs them again to make sure generated app works, then ejects and runs them yet again. So this script is where you want to add "npm test" calls so that we know that testing infrastructure works and won't be broken by future PRs.

case 'test':
var result = spawn.sync(
'node',
[require.resolve('../scripts/' + script)].concat(args),
Expand Down
1 change: 1 addition & 0 deletions config/eslint.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ module.exports = {
browser: true,
commonjs: true,
es6: true,
jest: true,
node: true
},

Expand Down
11 changes: 11 additions & 0 deletions config/jest/CSSStub.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @flow
*/

module.exports = {};
11 changes: 11 additions & 0 deletions config/jest/FileStub.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @flow
*/

module.exports = "test-file-stub";
12 changes: 12 additions & 0 deletions config/jest/transform.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

const babelDev = require('../babel.dev');
const babelJest = require('babel-jest');

module.exports = babelJest.createTransformer(babelDev);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like the nicest way to ensure low-maintenance of this script as we add more stuff to babel-jest.

5 changes: 4 additions & 1 deletion global-cli/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,10 @@ function createApp(name, verbose, version) {
version: '0.0.1',
private: true,
};
fs.writeFileSync(path.join(root, 'package.json'), JSON.stringify(packageJson));
fs.writeFileSync(
path.join(root, 'package.json'),
JSON.stringify(packageJson, null, 2)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't really matter so much but the initial version of package.json wasn't formatted nicely.

);
var originalDirectory = process.cwd();
process.chdir(root);

Expand Down
12 changes: 8 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@
"url": "https://github.com/facebookincubator/create-react-app/issues"
},
"scripts": {
"start": "node scripts/start.js --debug-template",
"build": "node scripts/build.js --debug-template",
"create-react-app": "node global-cli/index.js --scripts-version \"$PWD/`npm pack`\"",
"test": "tasks/e2e.sh"
"e2e": "tasks/e2e.sh",
"start": "node scripts/start.js --debug-template",
"test": "node scripts/test.js --debug-template"
},
"files": [
"PATENTS",
Expand All @@ -30,6 +31,7 @@
"autoprefixer": "6.3.7",
"babel-core": "6.11.4",
"babel-eslint": "6.1.2",
"babel-jest": "14.1.0",
"babel-loader": "6.2.4",
"babel-plugin-syntax-trailing-function-commas": "6.8.0",
"babel-plugin-transform-class-properties": "6.11.5",
Expand Down Expand Up @@ -57,6 +59,7 @@
"fs-extra": "0.30.0",
"gzip-size": "3.0.0",
"html-webpack-plugin": "2.22.0",
"jest": "14.1.0",
"json-loader": "0.5.4",
"opn": "4.0.2",
"postcss-loader": "0.9.1",
Expand All @@ -70,8 +73,9 @@
},
"devDependencies": {
"bundle-deps": "1.0.0",
"react": "^15.2.1",
"react-dom": "^15.2.1"
"react": "^15.3.0",
"react-dom": "^15.3.0",
"react-test-renderer": "^15.3.0"
},
"optionalDependencies": {
"fsevents": "1.0.14"
Expand Down
12 changes: 11 additions & 1 deletion scripts/eject.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@
* of patent rights can be found in the PATENTS file in the same directory.
*/

var createJestConfig = require('./utils/create-jest-config');
var fs = require('fs');
var path = require('path');
var prompt = require('./utils/prompt');
var rimrafSync = require('rimraf').sync;
var spawnSync = require('cross-spawn').sync;
var prompt = require('./utils/prompt');

prompt(
'Are you sure you want to eject? This action is permanent.',
Expand All @@ -37,6 +38,9 @@ prompt(
path.join('config', 'polyfills.js'),
path.join('config', 'webpack.config.dev.js'),
path.join('config', 'webpack.config.prod.js'),
path.join('config', 'jest', 'CSSStub.js'),
path.join('config', 'jest', 'FileStub.js'),
path.join('config', 'jest', 'transform.js'),
path.join('scripts', 'build.js'),
path.join('scripts', 'start.js'),
path.join('scripts', 'utils', 'chrome.applescript'),
Expand All @@ -59,6 +63,7 @@ prompt(
// Copy the files over
fs.mkdirSync(path.join(appPath, 'config'));
fs.mkdirSync(path.join(appPath, 'config', 'flow'));
fs.mkdirSync(path.join(appPath, 'config', 'jest'));
fs.mkdirSync(path.join(appPath, 'scripts'));
fs.mkdirSync(path.join(appPath, 'scripts', 'utils'));

Expand Down Expand Up @@ -96,6 +101,11 @@ prompt(
});
delete appPackage.scripts['eject'];

appPackage.scripts.test = 'jest';
appPackage.jest = createJestConfig(
filePath => path.join('<rootDir>', filePath)
);

// explicitly specify ESLint config path for editor plugins
appPackage.eslintConfig = {
extends: './config/eslint.js',
Expand Down
6 changes: 5 additions & 1 deletion scripts/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@ module.exports = function(appPath, appName, verbose, originalDirectory) {

// Copy over some of the devDependencies
appPackage.dependencies = appPackage.dependencies || {};
appPackage.devDependencies = appPackage.devDependencies || {};
['react', 'react-dom'].forEach(function (key) {
appPackage.dependencies[key] = ownPackage.devDependencies[key];
});
['react-test-renderer'].forEach(function (key) {
appPackage.devDependencies[key] = ownPackage.devDependencies[key];
});

// Setup the script rules
appPackage.scripts = {};
['start', 'build', 'eject'].forEach(function(command) {
['start', 'build', 'eject', 'test'].forEach(function(command) {
appPackage.scripts[command] = 'react-scripts ' + command;
});

Expand Down
29 changes: 29 additions & 0 deletions scripts/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

process.env.NODE_ENV = 'test';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind documenting the behavior of NODE_ENV somewhere like in the README? I know you didn't start using it, but it seems to be undocumented at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not affecting anything in Jest itself but it is kind of a standard that people have adopted. For example, Relay's preprocessor behaves differently when this flag is set. Jest sets this in its binary (jest-cli/bin/jest.js) but since we are not it directly here we have to add it.


const createJestConfig = require('./utils/create-jest-config');
const jest = require('jest');
const path = require('path');
const paths = require('../config/paths');

const argv = process.argv.slice(2);

const index = argv.indexOf('--debug-template');
if (index !== -1) {
argv.splice(index, 1);
}

argv.push('--config', JSON.stringify(createJestConfig(
Copy link
Contributor

@keyz keyz Jul 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like we pass the config explicitly again -- is it because Jest can't use the config in package.json here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jest could but I specifically asked that we avoid polluting package.json with config options :-)

relativePath => path.resolve(__dirname, '..', relativePath),
path.resolve(paths.appSrc, '..')
)));

jest.run(argv);
28 changes: 28 additions & 0 deletions scripts/utils/create-jest-config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

module.exports = (resolve, rootDir) => {
const config = {
automock: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like that this turns off automocking by default. Do you think Jest itself should / will soon switch that to the default behavior for everyone? It might confuse people when, if they start with create-react-app and use Jest, the behavior is a lot different than if they just start with Jest directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i really like that too.
@cpojer i though the default automock: false was in the plan for 14.0, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been my plan to slowly move away from automocking as a default and towards explicit mocks using jest.mock('path/to/module'). This, the react-native-preset and the Jest documentation recommend disabling automocking but I have been too chicken to make this the default.

Let's do it for Jest 15, which should be out in less than a month or so?

moduleNameMapper: {
'^[./a-zA-Z0-9$_-]+\\.(jpg|png|gif|eot|svg|ttf|woff|woff2|mp4|webm)$': resolve('config/jest/FileStub.js'),
'^[./a-zA-Z0-9$_-]+\\.css$': resolve('config/jest/CSSStub.js')
},
persistModuleRegistryBetweenSpecs: true,
scriptPreprocessor: resolve('config/jest/transform.js'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these require paths be relative? I'm confused as to how this is currently able to find the config folder

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It resolves them either to <rootDir> in the ejected mode, which is the dir that package.json is in or in any other state it resolves the full path from react-scripts to the config.

setupFiles: [
resolve('config/polyfills.js')
],
testEnvironment: 'node'
};
if (rootDir) {
config.rootDir = rootDir;
}
return config;
};
14 changes: 14 additions & 0 deletions tasks/e2e.sh
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ cd "$(dirname "$0")"
function cleanup {
echo 'Cleaning up.'
cd $initial_path
rm ../template/src/__tests__/__snapshots__/App-test.js.snap
rm -rf $temp_cli_path $temp_app_path
}

Expand Down Expand Up @@ -38,6 +39,7 @@ set -x

# npm pack the two directories to make sure they are valid npm modules
initial_path=$PWD

cd ..

# A hacky way to avoid bundling dependencies.
Expand Down Expand Up @@ -65,6 +67,10 @@ test -e build/static/js/*.js
test -e build/static/css/*.css
test -e build/static/media/*.svg

# Run tests
npm run test
test -e template/src/__tests__/__snapshots__/App-test.js.snap

# Pack CLI
cd global-cli
npm install
Expand All @@ -90,6 +96,10 @@ test -e build/static/js/*.js
test -e build/static/css/*.css
test -e build/static/media/*.svg

# Run tests
npm run test
test -e src/__tests__/__snapshots__/App-test.js.snap

# Test the server
npm start -- --smoke-test

Expand All @@ -103,6 +113,10 @@ test -e build/static/js/*.js
test -e build/static/css/*.css
test -e build/static/media/*.svg

# Run tests
npm run test
test -e src/__tests__/__snapshots__/App-test.js.snap

# Test the server
npm start -- --smoke-test

Expand Down
11 changes: 11 additions & 0 deletions template/src/__tests__/App-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import React from 'react';
import App from '../App';
import renderer from 'react-test-renderer';

describe('App', () => {
it('renders a welcome view', () => {
const instance = renderer.create(<App />);
const tree = instance.toJSON();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this API going to change? I remember there were talks about not wanting people to inspect the JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are likely to add more APIs to the instance to do selections (similar to enzyme, except component based, like instance.find(Button)). Right now people shouldn't do more than instance.toJSON() and instance.props.on*() calls. Everything else is subject to change.

expect(tree).toMatchSnapshot();
});
});