test: skipIfInspectorDisabled cluster-inspect-brk #12757

danbev · 2017-04-30T06:23:41Z

When configured --without-ssl the inspect-brk option will not be
available and the process will exit with a exit value of 9 "Invalid
Argument/Bad option".

This commit adds a skipIfInspectorDisabled check since --without-ssl
implies that no inspector support is build as well.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

danbev · 2017-04-30T06:24:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7756/

targos · 2017-04-30T10:53:18Z

test/sequential/test-cluster-inspect-brk.js

@@ -1,5 +1,9 @@
 'use strict';
 const common = require('../common');
+if (!common.hasCrypto) {


I think it can just be common.skipIfInspectorDisabled()

IMHO not having SSL is not a reason to skip this test, that's a regression.
What @targos suggested makes more sense, and should be reflected in title and commit message, i.e. test: skip cluster-inspect-brk if inspector is disabled

danbev · 2017-04-30T11:20:20Z

Good point, I'll fix that. Thanks sön 30 apr. 2017 kl. 12:53 skrev Michaël Zasso <notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In test/sequential/test-cluster-inspect-brk.js <#12757 (comment)>: > @@ -1,5 +1,9 @@ 'use strict'; const common = require('../common'); +if (!common.hasCrypto) { I think it can just be common.skipIfInspectorDisabled() — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#12757 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaY32C-hCXevPx7VykTQUmHl-UcxT6Zks5r1GgigaJpZM4NMe75> .

refack · 2017-04-30T13:10:06Z

Is it the same with just --inspect? Isn't that a bigger problem?
IMHO we need to have inspector fallback to HTTP.

Ref: #12758

refack · 2017-04-30T13:22:23Z

test/sequential/test-cluster-inspect-brk.js

@@ -1,5 +1,9 @@
 'use strict';
 const common = require('../common');
+if (!common.hasCrypto) {


IMHO not having SSL is not a reason to skip this test, that's a regression.
What @targos suggested makes more sense, and should be reflected in title and commit message, i.e. test: skip cluster-inspect-brk if inspector is disabled

When configured --without-ssl the inspect-brk option will not be available and the process will exit with a exit value of 9 "Invalid Argument/Bad option". This commit adds a skipIfInspectorDisabled check since --without-ssl implies that no inspector support is build as well.

danbev · 2017-04-30T13:39:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7760/

refack

Thanks.

When configured --without-ssl the inspect-brk option will not be available and the process will exit with a exit value of 9 "Invalid Argument/Bad option". This commit adds a skipIfInspectorDisabled check since --without-ssl implies that no inspector support is build as well. PR-URL: nodejs#12757 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

danbev · 2017-05-02T05:52:19Z

Landed in 10ccf56

When configured --without-ssl the inspect-brk option will not be available and the process will exit with a exit value of 9 "Invalid Argument/Bad option". This commit adds a skipIfInspectorDisabled check since --without-ssl implies that no inspector support is build as well. PR-URL: nodejs#12757 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn · 2017-06-18T22:57:38Z

Should land with #12615

When configured --without-ssl the inspect-brk option will not be available and the process will exit with a exit value of 9 "Invalid Argument/Bad option". This commit adds a skipIfInspectorDisabled check since --without-ssl implies that no inspector support is build as well. PR-URL: #12757 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 30, 2017

mscdex added cluster Issues and PRs related to the cluster subsystem. inspector Issues and PRs related to the V8 inspector protocol labels Apr 30, 2017

targos reviewed Apr 30, 2017

View reviewed changes

refack mentioned this pull request Apr 30, 2017

build: --without-ssl and --without-intl imply new limitation (no debugger) #12758

Closed

refack suggested changes Apr 30, 2017

View reviewed changes

danbev force-pushed the add-hasCrypto-check-cluster-inspect-brk branch from 342e8e8 to 2cb2411 Compare April 30, 2017 13:38

danbev changed the title ~~test: add hasCrypto check to cluster-inspect-brk~~ test: skipIfInspectorDisabled cluster-inspect-brk Apr 30, 2017

refack approved these changes Apr 30, 2017

View reviewed changes

cjihrig approved these changes Apr 30, 2017

View reviewed changes

addaleax approved these changes Apr 30, 2017

View reviewed changes

targos approved these changes Apr 30, 2017

View reviewed changes

jasnell approved these changes May 1, 2017

View reviewed changes

danbev closed this May 2, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the lts-watch-v6.x label Jun 18, 2017

danbev deleted the add-hasCrypto-check-cluster-inspect-brk branch June 28, 2017 05:26

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Oct 16, 2017

MylesBorins removed the land-on-v6.x label Oct 16, 2017

MylesBorins mentioned this pull request Oct 16, 2017

test: make tests pass when configured --without-ssl #11631

Closed

2 tasks

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Oct 16, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: skipIfInspectorDisabled cluster-inspect-brk #12757

test: skipIfInspectorDisabled cluster-inspect-brk #12757

danbev commented Apr 30, 2017 •

edited

Loading

danbev commented Apr 30, 2017

targos Apr 30, 2017

refack Apr 30, 2017

danbev commented Apr 30, 2017 via email

refack commented Apr 30, 2017 •

edited

Loading

refack Apr 30, 2017

danbev commented Apr 30, 2017

refack left a comment

danbev commented May 2, 2017

gibfahn commented Jun 18, 2017

test: skipIfInspectorDisabled cluster-inspect-brk #12757

test: skipIfInspectorDisabled cluster-inspect-brk #12757

Conversation

danbev commented Apr 30, 2017 • edited Loading

Checklist

Affected core subsystem(s)

danbev commented Apr 30, 2017

targos Apr 30, 2017

Choose a reason for hiding this comment

refack Apr 30, 2017

Choose a reason for hiding this comment

danbev commented Apr 30, 2017 via email

refack commented Apr 30, 2017 • edited Loading

refack Apr 30, 2017

Choose a reason for hiding this comment

danbev commented Apr 30, 2017

refack left a comment

Choose a reason for hiding this comment

danbev commented May 2, 2017

gibfahn commented Jun 18, 2017

danbev commented Apr 30, 2017 •

edited

Loading

refack commented Apr 30, 2017 •

edited

Loading