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.

Sign up for GitHub

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

Remove debug and inspect flags from the arguments sent to the child #5068

Merged
merged 2 commits into from
Dec 12, 2017
+35 −22
Merged

Remove debug and inspect flags from the arguments sent to the child #5068

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22c4e3f
Remove `debug` and `inspect` flags from the arguments sent to the child
SimenB Dec 12, 2017
14381ee
address PR comments
SimenB Dec 12, 2017
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
15 changes: 9 additions & 6 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@

### Fixes

* `[jest-worker]` Remove `debug` and `inspect` flags from the arguments sent to
the child ([#5068](https://github.com/facebook/jest/pull/5068))
* `[jest-config]` Use all `--testPathPattern` and `<regexForTestFiles>` args in
`testPathPattern`([#5066](https://github.com/facebook/jest/pull/5066))
* `[jest-cli]` Do not support `--watch` inside non-version-controlled environments
([#5060](https://github.com/facebook/jest/pull/5060))
`testPathPattern` ([#5066](https://github.com/facebook/jest/pull/5066))
* `[jest-cli]` Do not support `--watch` inside non-version-controlled
environments ([#5060](https://github.com/facebook/jest/pull/5060))
* `[jest-config]` Escape Windows path separator in testPathPattern CLI arguments
([#5054](https://github.com/facebook/jest/pull/5054)
* `[jest-jasmine]` Register sourcemaps as node environment to improve performance with jsdom ([#5045](https://github.com/facebook/jest/pull/5045))
* `[jest-jasmine]` Register sourcemaps as node environment to improve
performance with jsdom ([#5045](https://github.com/facebook/jest/pull/5045))
* `[pretty-format]` Do not call toJSON recursively
([#5044](https://github.com/facebook/jest/pull/5044))
* `[pretty-format]` Fix errors when identity-obj-proxy mocks CSS Modules
Expand Down Expand Up @@ -66,8 +69,8 @@

### Features

* `[jest-config]` Add `testEnvironmentOptions` to apply to jsdom options or node context.
([#5003](https://github.com/facebook/jest/pull/5003))
* `[jest-config]` Add `testEnvironmentOptions` to apply to jsdom options or node
context. ([#5003](https://github.com/facebook/jest/pull/5003))
* `[jest-jasmine2]` Update Timeout error message to `jest.timeout` and display
current timeout value ([#4990](https://github.com/facebook/jest/pull/4990))
* `[jest-runner]` Enable experimental detection of leaked contexts
Expand Down
25 changes: 13 additions & 12 deletions docs/GlobalAPI.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ environment. You don't have to require or import anything to use them.
Runs a function after all the tests in this file have completed. If the function
returns a promise, Jest waits for that promise to resolve before continuing.

Optionally, you can provide a `timeout` (in milliseconds) for specifying how long to wait
before aborting. _Note: The default timeout is 5 seconds._
Optionally, you can provide a `timeout` (in milliseconds) for specifying how
long to wait before aborting. _Note: The default timeout is 5 seconds._

This is often useful if you want to clean up some global setup state that is
shared across tests.
Expand Down Expand Up @@ -66,8 +66,8 @@ Runs a function after each one of the tests in this file completes. If the
function returns a promise, Jest waits for that promise to resolve before
continuing.

Optionally, you can provide a `timeout` (in milliseconds) for specifying how long to wait
before aborting. _Note: The default timeout is 5 seconds._
Optionally, you can provide a `timeout` (in milliseconds) for specifying how
long to wait before aborting. _Note: The default timeout is 5 seconds._

This is often useful if you want to clean up some temporary state that is
created by each test.
Expand Down Expand Up @@ -112,8 +112,8 @@ If you want to run some cleanup just once, after all of the tests run, use
Runs a function before any of the tests in this file run. If the function
returns a promise, Jest waits for that promise to resolve before running tests.

Optionally, you can provide a `timeout` (in milliseconds) for specifying how long to wait
before aborting. _Note: The default timeout is 5 seconds._
Optionally, you can provide a `timeout` (in milliseconds) for specifying how
long to wait before aborting. _Note: The default timeout is 5 seconds._

This is often useful if you want to set up some global state that will be used
by many tests.
Expand Down Expand Up @@ -157,8 +157,8 @@ Runs a function before each of the tests in this file runs. If the function
returns a promise, Jest waits for that promise to resolve before running the
test.

Optionally, you can provide a `timeout` (in milliseconds) for specifying how long to wait
before aborting. _Note: The default timeout is 5 seconds._
Optionally, you can provide a `timeout` (in milliseconds) for specifying how
long to wait before aborting. _Note: The default timeout is 5 seconds._

This is often useful if you want to reset some global state that will be used by
many tests.
Expand Down Expand Up @@ -327,8 +327,9 @@ test('did not rain', () => {
```

The first argument is the test name; the second argument is a function that
contains the expectations to test. The third argument (optional) is `timeout` (in milliseconds)
for specifying how long to wait before aborting. _Note: The default timeout is 5 seconds._
contains the expectations to test. The third argument (optional) is `timeout`
(in milliseconds) for specifying how long to wait before aborting. _Note: The
default timeout is 5 seconds._

> Note: If a **promise is returned** from `test`, Jest will wait for the promise
> to resolve before letting the test complete. Jest will also wait if you
Expand Down Expand Up @@ -358,8 +359,8 @@ When you are debugging a large codebase, you will often only want to run a
subset of tests. You can use `.only` to specify which tests are the only ones
you want to run.

Optionally, you can provide a `timeout` (in milliseconds) for specifying how long to wait
before aborting. _Note: The default timeout is 5 seconds._
Optionally, you can provide a `timeout` (in milliseconds) for specifying how
long to wait before aborting. _Note: The default timeout is 5 seconds._

For example, let's say you had these tests:

Expand Down
4 changes: 2 additions & 2 deletions packages/jest-worker/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ to `3`, pass `Infinity` to allow endless retries.
#### `forkOptions: Object` (optional)

Allow customizing all options passed to `childProcess.fork`. By default, some
values are set (`cwd` and `env`), but you can override them and customize the
rest. For a list of valid values, check
values are set (`cwd`, `env` and `execArgv`), but you can override them and
customize the rest. For a list of valid values, check
[the Node documentation](https://nodejs.org/api/child_process.html#child_process_child_process_fork_modulepath_args_options).

#### `computeWorkerKey: (method: string, ...args: Array<any>) => ?string` (optional)
Expand Down
12 changes: 10 additions & 2 deletions packages/jest-worker/src/__tests__/worker.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ import {
let Worker;
let forkInterface;
let childProcess;
let properProcess;

beforeEach(() => {
jest.mock('child_process');
properProcess = process;

childProcess = require('child_process');
childProcess.fork.mockImplementation(() => {
Expand All @@ -41,14 +43,19 @@ beforeEach(() => {

afterEach(() => {
jest.resetModules();
// eslint-disable-next-line no-native-reassign
process = properProcess;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like ideas here... require('process'); and mock it properly?

});

it('passes fork options down to child_process.fork, adding the defaults', () => {
const child = require.resolve('../child');

Object.assign(process, {execArgv: ['--inspect', '-p']});
Copy link
Member Author

Choose a reason for hiding this comment

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

I find it funny that eslint does not yell at me for native-reassign here

Copy link
Contributor

Choose a reason for hiding this comment

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

The process => properProcess dance does not prevent from modifying the global object, since Object.assign will assign to the real process. Instead you might want to save execArgv at the beginning, and re-assign to process on the afterEach.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really care that I modify it, it's sandboxed for this test in particular, right?

Good idea on just messing with the part I need to mess with, though


new Worker({
forkOptions: {
cwd: '/tmp',
execArgv: ['--no-warnings'],
execPath: 'hello',
},
maxRetries: 3,
workerPath: '/tmp/foo/bar/baz.js',
Expand All @@ -58,7 +65,8 @@ it('passes fork options down to child_process.fork, adding the defaults', () =>
expect(childProcess.fork.mock.calls[0][1]).toEqual({
cwd: '/tmp', // Overridden default option.
env: process.env, // Default option.
execArgv: ['--no-warnings'], // Added option.
execArgv: ['-p'], // Filtered option.
execPath: 'hello', // Added option.
silent: true, // Default option.
});
});
Expand Down
3 changes: 3 additions & 0 deletions packages/jest-worker/src/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ export default class {
{
cwd: process.cwd(),
env: process.env,
// suppress --debug / --inspect flags while preserving others (like --harmony)
// inspired by https://github.com/rvagg/node-worker-farm/blob/f63d988c307a6805e03b1650f8ef0fb7ca6f1546/lib/fork.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the comment? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe not. It's a pure copy, though :P

execArgv: process.execArgv.filter(v => !/^--(debug|inspect)/.test(v)),
silent: true,
},
this._options.forkOptions,
Expand Down