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

feature/add-first-class-debugging-support-for-tests #1360

Closed
Closed
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
8 changes: 7 additions & 1 deletion packages/react-scripts/bin/react-scripts.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#!/usr/bin/env node
var spawn = require('cross-spawn');
var getDebugFlag = require('../utils/getDebugFlag');
Copy link
Contributor

Choose a reason for hiding this comment

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

For this to work after the eject, you'll probably have to add the right path here.

var script = process.argv[2];
var args = process.argv.slice(3);

Expand All @@ -8,9 +9,14 @@ case 'build':
case 'eject':
case 'start':
case 'test':
var scriptArgs = [require.resolve('../scripts/' + script)].concat(args);
var debugFlag = getDebugFlag(args);
if (debugFlag) {
scriptArgs.unshift(debugFlag);
Copy link
Author

Choose a reason for hiding this comment

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

note, running node --debug-brk ./node_modules/.bin/react-scripts test --debug-brk will biff on conflicting port access to 5858, if this wasn't clear already. I did not handle this case, but it may be worth discussion?

}
var result = spawn.sync(
'node',
[require.resolve('../scripts/' + script)].concat(args),
scriptArgs,
{stdio: 'inherit'}
);
process.exit(result.status);
Expand Down
3 changes: 3 additions & 0 deletions packages/react-scripts/config/jest/babelJestDebugConfig.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const babelJestDefaultConfig = require('./babelJestDefaultConfig');
const assign = require('object-assign');
module.exports = assign(babelJestDefaultConfig, { sourceMaps: true });
4 changes: 4 additions & 0 deletions packages/react-scripts/config/jest/babelJestDefaultConfig.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
module.exports = {
presets: [require.resolve('babel-preset-react-app')],
babelrc: false
}
7 changes: 2 additions & 5 deletions packages/react-scripts/config/jest/babelTransform.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,5 @@
*/

const babelJest = require('babel-jest');

module.exports = babelJest.createTransformer({
presets: [require.resolve('babel-preset-react-app')],
babelrc: false
});
const babelJestDebugConfig = require('./babelJestDebugConfig');
module.exports = babelJest.createTransformer(babelJestDebugConfig);
Copy link
Author

Choose a reason for hiding this comment

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

this was probably supposed to be Default v Debug?

11 changes: 11 additions & 0 deletions packages/react-scripts/config/jest/babelTransformDebug.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.
*/

const babelJest = require('babel-jest');
const babelJestDebugConfig = require('./babelJestDebugConfig');
module.exports = babelJest.createTransformer(babelJestDebugConfig);
12 changes: 6 additions & 6 deletions packages/react-scripts/scripts/eject.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ prompt(
path.join('config', 'jest', 'fileTransform.js'),
path.join('scripts', 'build.js'),
path.join('scripts', 'start.js'),
path.join('scripts', 'test.js')
path.join('scripts', 'test.js'),
path.join('utils', 'getDebugFlag.js')
];

// Ensure that the app folder is clean and we won't override any files
Expand Down Expand Up @@ -122,11 +123,10 @@ prompt(
console.log(cyan('Configuring package.json'));
// Add Jest config
console.log(' Adding ' + cyan('Jest') + ' configuration');
appPackage.jest = createJestConfig(
filePath => path.join('<rootDir>', filePath),
null,
true
);
appPackage.jest = createJestConfig({
resolve: filePath => path.join('<rootDir>', filePath),
isEjecting: true
});

// Add Babel config

Expand Down
28 changes: 22 additions & 6 deletions packages/react-scripts/scripts/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@
*/
// @remove-on-eject-end

/**
* Greetings! If you are here attempting to start a debugging session, please
* ensure that your debugger of choice is configured to enable source maps,
* otherwise your code may appear mangled by babel!
*/
Copy link
Author

Choose a reason for hiding this comment

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

🎉 greet the debugging user with some 🌶 🔥 tips

process.env.NODE_ENV = 'test';
process.env.PUBLIC_URL = '';

Expand All @@ -20,21 +25,32 @@ require('dotenv').config({silent: true});

const jest = require('jest');
const argv = process.argv.slice(2);
const debugFlag = require('../utils/getDebugFlag')(argv);
const isDebug = !!debugFlag;
const isRunInBand = argv.indexOf('--runInBand') > -1 || argv.indexOf('-i') > -1

// Watch unless on CI or in coverage mode
if (!process.env.CI && argv.indexOf('--coverage') < 0) {
if (!process.env.CI && argv.indexOf('--coverage') < 0 && !isDebug) {
argv.push('--watch');
}

// Force debug into single worker
if (isDebug) {
if (!isRunInBand) {
argv.push('--runInBand')
}
argv.splice(argv.indexOf(debugFlag), 1)
Copy link
Author

Choose a reason for hiding this comment

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

any of the node debug flags are not valid input to jest. something counter convention i did in this PR was allow a debug flag (like --debug-brk) to enter via argv, where previously all of the other args were jest specific. IMHO, this is OK. the filtering may be cleaner towards the top-of-the-file

}

// @remove-on-eject-begin
// This is not necessary after eject because we embed config into package.json.
const createJestConfig = require('../utils/createJestConfig');
const path = require('path');
const paths = require('../config/paths');
argv.push('--config', JSON.stringify(createJestConfig(
relativePath => path.resolve(__dirname, '..', relativePath),
path.resolve(paths.appSrc, '..'),
false
)));
argv.push('--config', JSON.stringify(createJestConfig({
resolve: relativePath => path.resolve(__dirname, '..', relativePath),
rootDir: path.resolve(paths.appSrc, '..'),
isDebug: isDebug
})));
// @remove-on-eject-end
jest.run(argv);
21 changes: 15 additions & 6 deletions packages/react-scripts/template/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ You can find the most recent version of this guide [here](https://github.com/fac
- [Disabling jsdom](#disabling-jsdom)
- [Experimental Snapshot Testing](#experimental-snapshot-testing)
- [Editor Integration](#editor-integration)
- [Debugging](#debugging)
- [Developing Components in Isolation](#developing-components-in-isolation)
- [Making a Progressive Web App](#making-a-progressive-web-app)
- [Deployment](#deployment)
Expand Down Expand Up @@ -480,7 +481,7 @@ Now you are ready to use the imported React Bootstrap components within your com

Flow is a static type checker that helps you write code with fewer bugs. Check out this [introduction to using static types in JavaScript](https://medium.com/@preethikasireddy/why-use-static-types-in-javascript-part-1-8382da1e0adb) if you are new to this concept.

Recent versions of [Flow](http://flowtype.org/) work with Create React App projects out of the box.
Recent versions of [Flow](http://flowtype.org/) work with Create React App projects out of the box.

To add Flow to a Create React App project, follow these steps:

Expand Down Expand Up @@ -938,10 +939,18 @@ This feature is experimental and still [has major usage issues](https://github.c

### Editor Integration

If you use [Visual Studio Code](https://code.visualstudio.com), there is a [Jest extension](https://github.com/orta/vscode-jest) which works with Create React App out of the box. This provides a lot of IDE-like features while using a text editor: showing the status of a test run with potential fail messages inline, starting and stopping the watcher automatically, and offering one-click snapshot updates.
If you use [Visual Studio Code](https://code.visualstudio.com), there is a [Jest extension](https://github.com/orta/vscode-jest) which works with Create React App out of the box. This provides a lot of IDE-like features while using a text editor: showing the status of a test run with potential fail messages inline, starting and stopping the watcher automatically, and offering one-click snapshot updates.

![VS Code Jest Preview](https://cloud.githubusercontent.com/assets/49038/20795349/a032308a-b7c8-11e6-9b34-7eeac781003f.png)

### Debugging

You can easily debug your tests by passing the usual debug flags to `npm test`.

For example:
- Run `npm test -- --debug-brk` to attach the the debugger using your debugger of choice (vscode, webstorm, etc).
- Run `npm test -- debug` to enter the CLI debugger.

## Developing Components in Isolation

Usually, in an app, you have a lot of UI components, and each of them has many different states.
Expand Down Expand Up @@ -1173,16 +1182,16 @@ GitHub Pages doesn't support routers that use the HTML5 `pushState` history API
### Heroku

Use the [Heroku Buildpack for Create React App](https://github.com/mars/create-react-app-buildpack).<br>
You can find instructions in [Deploying React with Zero Configuration](https://blog.heroku.com/deploying-react-with-zero-configuration).
You can find instructions in [Deploying React with Zero Configuration](https://blog.heroku.com/deploying-react-with-zero-configuration).

#### Resolving "Module not found: Error: Cannot resolve 'file' or 'directory'"

Sometimes `npm run build` works locally but fails during deploy via Heroku with an error like this:

```
```
remote: Failed to create a production build. Reason:
remote: Module not found: Error: Cannot resolve 'file' or 'directory'
MyDirectory in /tmp/build_1234/src
remote: Module not found: Error: Cannot resolve 'file' or 'directory'
MyDirectory in /tmp/build_1234/src
```

This means you need to ensure that the lettercase of the file or directory you `import` matches the one you see on your filesystem or on GitHub.
Expand Down
15 changes: 10 additions & 5 deletions packages/react-scripts/utils/createJestConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,18 @@
const fs = require('fs');
const paths = require('../config/paths');

module.exports = (resolve, rootDir, isEjecting) => {
module.exports = (opts) => {
const resolve = opts.resolve;
const rootDir = opts.rootDir || null;
const isEjecting = opts.isEjecting || false;
const isDebug = opts.isDebug || false;

// Use this instead of `paths.testsSetup` to avoid putting
// an absolute filename into configuration after ejecting.
const setupTestsFile = fs.existsSync(paths.testsSetup) ? '<rootDir>/src/setupTests.js' : undefined;

const babelTransform = isEjecting
? '<rootDir>/node_modules/babel-jest'
: resolve('config/jest/babelTransform' + (isDebug ? 'Debug' : '') + '.js')
Copy link
Author

@cdaringe cdaringe Apr 12, 2017

Choose a reason for hiding this comment

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

@Timer, 👀, this is where/why there were two babelTransform files. we are passing the babel config as a file path, therefore, to enable sourceMaps, i added a different file. if there's another way to get those babel settings passed to jest, we could pursue that. we could also set something in the ENV to drop it down to one transform file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I hadn't reviewed deep enough to have this understanding.
Thanks for explaining!

I'll snoop around and see what our best option(s) are. 😄

// TODO: I don't know if it's safe or not to just use / as path separator
// in Jest configs. We need help from somebody with Windows to determine this.
const config = {
Expand All @@ -29,9 +36,7 @@ module.exports = (resolve, rootDir, isEjecting) => {
testEnvironment: 'node',
testURL: 'http://localhost',
transform: {
'^.+\\.(js|jsx)$': isEjecting ?
'<rootDir>/node_modules/babel-jest'
: resolve('config/jest/babelTransform.js'),
'^.+\\.(js|jsx)$': babelTransform,
'^.+\\.css$': resolve('config/jest/cssTransform.js'),
'^(?!.*\\.(js|jsx|css|json)$)': resolve('config/jest/fileTransform.js'),
},
Expand Down
25 changes: 25 additions & 0 deletions packages/react-scripts/utils/getDebugFlag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* 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.
*/

const DEBUG_FLAGS = [ 'debug', '--debug-brk', '--inspect' ];

module.exports = function getDebugFlag(argv) {
for (var i in argv) {
if (argv.hasOwnProperty(i)) {
for (var j in DEBUG_FLAGS) {
if (DEBUG_FLAGS.hasOwnProperty(j)) {
if (argv[i] === DEBUG_FLAGS[j]) {
return DEBUG_FLAGS[j];
}
}
}
}
}
return null;
}