test: skipIfInspectorDisabled cluster-inspect-brk by danbev · Pull Request #12757 · nodejs/node

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

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

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

Uh oh!

test: skipIfInspectorDisabled cluster-inspect-brk#12757

test: skipIfInspectorDisabled cluster-inspect-brk#12757
danbev wants to merge 1 commit intonodejs:masterfrom
danbev:add-hasCrypto-check-cluster-inspect-brk

danbev commented Apr 30, 2017 •

edited

Loading

Uh oh!

danbev commented Apr 30, 2017

Uh oh!

targos Apr 30, 2017

Uh oh!

refack Apr 30, 2017

Uh oh!

danbev commented Apr 30, 2017 via email

Uh oh!

refack commented Apr 30, 2017 •

edited

Loading

Uh oh!

refack Apr 30, 2017

Uh oh!

danbev commented Apr 30, 2017

Uh oh!

refack left a comment

Uh oh!

danbev commented May 2, 2017

Uh oh!

gibfahn commented Jun 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Uh oh!

Conversation

danbev commented Apr 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented Apr 30, 2017

Uh oh!

targos Apr 30, 2017

Choose a reason for hiding this comment

Uh oh!

refack Apr 30, 2017

Choose a reason for hiding this comment

Uh oh!

danbev commented Apr 30, 2017 via email

Uh oh!

refack commented Apr 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack Apr 30, 2017

Choose a reason for hiding this comment

Uh oh!

danbev commented Apr 30, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

danbev commented May 2, 2017

Uh oh!

gibfahn commented Jun 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

danbev commented Apr 30, 2017 •

edited

Loading

refack commented Apr 30, 2017 •

edited

Loading