Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Dec 12, 2017

Summary
Using jest-worker when debugging caused it to crash. This ports over code from worker-farm which automatically filters away /--(debug|inspect)(-brk)?. (https://github.com/rvagg/node-worker-farm/blob/f63d988c307a6805e03b1650f8ef0fb7ca6f1546/lib/fork.js#L8-L11)

Test plan
Modified test to illustrate the filtering behaviour.

@SimenB
Copy link
Member Author

SimenB commented Dec 12, 2017

Markdown changes are me running yarn lint:md

@@ -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

@@ -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

});

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
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

@codecov-io
Copy link

codecov-io commented Dec 12, 2017

Codecov Report

Merging #5068 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5068      +/-   ##
==========================================
+ Coverage   60.75%   60.76%   +<.01%     
==========================================
  Files         198      198              
  Lines        6602     6603       +1     
  Branches        4        4              
==========================================
+ Hits         4011     4012       +1     
  Misses       2591     2591
Impacted Files Coverage Δ
packages/jest-worker/src/worker.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3db693...14381ee. Read the comment docs.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants