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

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants