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: check and fail inspector-cluster-port-clash #14074

Closed
wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Jul 4, 2017

Currently this test fail when configured --without-inspector or
--without-ssl as it is expected to fail but the skipIfInspectorDisabled
check will exit as if the test was successful.

This commit checks if inspector support is available and fails the test
allowing the test to be skipped.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test, tools

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jul 4, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Jul 4, 2017

Because this is a known_issue test, another approach is to fail instead of skip when the inspector is not available. A similar example can be seen here.

@refack
Copy link
Contributor

refack commented Jul 4, 2017

FWIW alternatively you could simply add if (process.config.variables.v8_enable_inspector === 0) process.exit(1) and a nice comment

@refack
Copy link
Contributor

refack commented Jul 4, 2017

As for this PR's change, it's a bit too generic. If you want to pursue such a change, I'd rather have a special flag in the test (something like --inspector --or-fail)

@danbev
Copy link
Contributor Author

danbev commented Jul 5, 2017

Because this is a known_issue test, another approach is to fail instead of skip when the inspector is not available. A similar example can be seen here.

Ah great, that would be much simpler. Thanks, I'll update the PR.

Currently this test fail when configured --without-inspector or
--without-ssl as it is expected to fail but the skipIfInspectorDisabled
check will exit as if the test was sucessful.

This commit checks if inspector support is available and fails the test
allowing the test to be skipped.
@danbev danbev force-pushed the hasfailed-removedflags-check branch from c28f46e to 2d0d934 Compare July 5, 2017 04:09
@danbev danbev changed the title test: add removedFlags to test.py test: check and fail inspector-cluster-port-clash Jul 5, 2017
@danbev
Copy link
Contributor Author

danbev commented Jul 5, 2017

if (process.config.variables.v8_enable_inspector === 0) {
// When the V8 inspector is disabled, using either --without-inspector or
// --without-ssl, this test will not fail which it is expected to do.
// The following fail will allow this test to be skipped by failing it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add something to this comment about replacing this block with common.skipIfInspectorDisabled() when/if this test is moved out of known_issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll add a comment about that.

danbev added a commit to danbev/node that referenced this pull request Jul 6, 2017
Currently this test fail when configured --without-inspector or
--without-ssl as it is expected to fail but the skipIfInspectorDisabled
check will exit as if the test was sucessful.

This commit checks if inspector support is available and fails the test
allowing the test to be skipped.

PR-URL: nodejs#14074
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@danbev
Copy link
Contributor Author

danbev commented Jul 6, 2017

Landed in f651e40.

@cjihrig @refack Thanks!

@danbev danbev closed this Jul 6, 2017
@danbev danbev deleted the hasfailed-removedflags-check branch July 6, 2017 12:10
addaleax pushed a commit that referenced this pull request Jul 11, 2017
Currently this test fail when configured --without-inspector or
--without-ssl as it is expected to fail but the skipIfInspectorDisabled
check will exit as if the test was sucessful.

This commit checks if inspector support is available and fails the test
allowing the test to be skipped.

PR-URL: #14074
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Jul 11, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
Currently this test fail when configured --without-inspector or
--without-ssl as it is expected to fail but the skipIfInspectorDisabled
check will exit as if the test was sucessful.

This commit checks if inspector support is available and fails the test
allowing the test to be skipped.

PR-URL: #14074
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Currently this test fail when configured --without-inspector or
--without-ssl as it is expected to fail but the skipIfInspectorDisabled
check will exit as if the test was sucessful.

This commit checks if inspector support is available and fails the test
allowing the test to be skipped.

PR-URL: #14074
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants