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

test: ensure error function call on inspector disabled #23586

Closed

Conversation

@madeinjam
Copy link
Contributor

madeinjam commented Oct 12, 2018

If the process.config.variables.v8_enable_inspector
is undefined or set to 0 and the sendInspectorCommand
function is called, it should call the error function
and should NOT call the callback function.

Test added to cover ./internal/util/inspector.js L4
https://coverage.nodejs.org/coverage-be346d9d32e0aacc/root/internal/util/inspector.js.html#L4

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows [commit guidelines]
We are migrating towards using internalBinding(\'options\').getOptions()
instead of process.binding(\'config\') to access the value of the
--experimental-vm-modules command line option.
Copy link
Member

BridgeAR left a comment

Great first contribution!

@addaleax addaleax added the inspector label Oct 12, 2018
@addaleax

This comment has been minimized.


inspector.sendInspectorCommand(
common.mustNotCall('Inspector callback should not be called'),
common.mustCall(restoreProcessVar, 1),

This comment has been minimized.

Copy link
@Trott

Trott Oct 13, 2018

Member

Hi, @madeinjam! Welcome and thanks for the pull request! I believe restoreProcessVar() can be removed from the file. The process exits, so there's no need to reset the value.

This comment has been minimized.

Copy link
@madeinjam

madeinjam Oct 13, 2018

Author Contributor

Hi @Trott ! I made the change, thank you!

If the process.config.variables.v8_enable_inspector
is undefined or set to 0 and the sendInspectorCommand
function is called, it should call the error function
and should NOT call the callback function.
@madeinjam madeinjam force-pushed the madeinjam:feature/add-test-to-inspector branch from 3379a04 to 2431bca Oct 13, 2018
Since the process exists after the test, there is no need to backup and
restore the process.config.variables.v8_enable_inspector variable.
@sam-github sam-github force-pushed the nodejs:master branch from 882e022 to 9ed4646 Oct 13, 2018
@Trott

This comment has been minimized.

@Trott Trott added the fast-track label Oct 13, 2018
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 13, 2018

Collaborators, 👍 here to approve fast-tracking.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 13, 2018

Landed in 9d1c9d7

@Trott Trott closed this Oct 13, 2018
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 13, 2018

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

Trott added a commit to Trott/io.js that referenced this pull request Oct 13, 2018
We are migrating towards using internalBinding(\'options\').getOptions()
instead of process.binding(\'config\') to access the value of the
--experimental-vm-modules command line option.

PR-URL: nodejs#23586
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
burgerboydaddy added a commit to burgerboydaddy/node that referenced this pull request Oct 17, 2018
We are migrating towards using internalBinding(\'options\').getOptions()
instead of process.binding(\'config\') to access the value of the
--experimental-vm-modules command line option.

PR-URL: nodejs#23586
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
jasnell added a commit that referenced this pull request Oct 17, 2018
We are migrating towards using internalBinding(\'options\').getOptions()
instead of process.binding(\'config\') to access the value of the
--experimental-vm-modules command line option.

PR-URL: #23586
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Oct 20, 2018
We are migrating towards using internalBinding(\'options\').getOptions()
instead of process.binding(\'config\') to access the value of the
--experimental-vm-modules command line option.

PR-URL: #23586
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins added a commit that referenced this pull request Oct 30, 2018
We are migrating towards using internalBinding(\'options\').getOptions()
instead of process.binding(\'config\') to access the value of the
--experimental-vm-modules command line option.

PR-URL: #23586
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg added a commit that referenced this pull request Nov 28, 2018
We are migrating towards using internalBinding(\'options\').getOptions()
instead of process.binding(\'config\') to access the value of the
--experimental-vm-modules command line option.

PR-URL: #23586
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins added a commit that referenced this pull request Nov 29, 2018
We are migrating towards using internalBinding(\'options\').getOptions()
instead of process.binding(\'config\') to access the value of the
--experimental-vm-modules command line option.

PR-URL: #23586
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.