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.

Sign up for GitHub

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

Jump to bottom

test: don't skip when common.mustCall() is pending #15421

Merged
merged 1 commit into from
Sep 17, 2017
Merged

test: don't skip when common.mustCall() is pending #15421

merged 1 commit into from
Sep 17, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Sep 14, 2017

The test parallel/test-dgram-multicast-set-interface.js was calling common.skip() on hosts that do not support IPv6. However, by this point, there were several outstanding common.mustCall() invocations. The process.exit() in common.skip() triggered those common.mustCall()s as errors.

Fixes: #15419

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 14, 2017
@cjihrig cjihrig mentioned this pull request Sep 14, 2017
Closed
@mscdex mscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label Sep 14, 2017
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Sep 15, 2017 •
edited
Loading

We can use printSkipMessage(msg) before return if needed in such cases.

@mscdex
Copy link
Contributor

mscdex commented Sep 15, 2017

@vsemozhetbyt But does that cause any issues for non-IPv6 systems when one of the IPv4 tests fails? I don't know offhand if 'Skip' output overrides exit code checks when determining if the test file failed or anything like that...

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Sep 15, 2017 •
edited
Loading

@mscdex Well, I cannot foresee that, but printSkipMessage(msg) is simply a wrapper for console.log() now.

@vsemozhetbyt
Copy link
Contributor

FWIW, refs: #14016 (comment)

@Trott
Copy link
Member

Trott commented Sep 15, 2017

I think the approach in this PR is the right one. I don't think it should use printSkipMessage() because that will print a TAP message. Our TAP messages apply on a per-file basis. So unless we're skipping the entire test file, we should not print the skip message. Even if it's harmless, it will be confusing. If something should be printed, just go with console.error('Skipping test case that requires IPv6'); or something like that.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Sep 15, 2017 •
edited
Loading

@Trott So does printSkipMessage() endanger any test? What are the criteria of safe/unsafe using? It is used in ~ 10 tests now. Should we check them all? All of them used skip() previously with the same messages.

@mscdex
Copy link
Contributor

mscdex commented Sep 15, 2017

LGTM

@Trott
Copy link
Member

Trott commented Sep 16, 2017 •
edited
Loading

@Trott So does printSkipMessage() endanger any test?

I don't think so.

What are the criteria of safe/unsafe using?

It's probably always safe. If the test file exits with an error, it still counts as not ok.

It is used in ~ 10 tests now. Should we check them all? All of them used skip() previously with the same messages.

I don't know. If I see ok 23 # skip Insufficient flogiston pressure. in the results, I assume the entire test was skipped, not part of it. But (a) I'm not sure that the type of message will result with printSkipMessage() if there is other output as well and (b) I'm not sure everyone else will agree with me that a skip message like that suggests the entire file was skipped and not merely part of it.

Not sure who to ping as the most informed people about TAP and how we use it in Jenkins, so @nodjes/build @nodejs/testing

@Trott
Copy link
Member

Trott commented Sep 16, 2017

In the interest of expediting this to get CI to green, I'm totally OK with going with printSkipMessage() and figuring out if it's an issue later on. I'm also OK with landing as is and figuring it out later. Like, don't hold this up over my trivial concerns. :-D

@vsemozhetbyt
Copy link
Contributor

@Trott FWIW, we do have tests where printSkipMessage() means partial skipping. But in these very tests we previously had skip() with the same partial skipping. For example:

test-dgram-bind-default-address.js now
test-dgram-bind-default-address.js before printSkipMessage() emerges.

@cjihrig
test: don't skip when common.mustCall() is pending
631c59b 
The test parallel/test-dgram-multicast-set-interface.js was
calling common.skip() on hosts that do not support IPv6. However,
by this point, there were several outstanding common.mustCall()
invocations. The process.exit() in common.skip() triggered
those common.mustCall()s as errors.

Fixes: nodejs#15419
PR-URL: nodejs#15421
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 17, 2017

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

@cjihrig cjihrig merged commit 631c59b into nodejs:master Sep 17, 2017
@cjihrig cjihrig deleted the test branch September 17, 2017 01:26
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
@cjihrig @addaleax
test: don't skip when common.mustCall() is pending
3fc14dc 
The test parallel/test-dgram-multicast-set-interface.js was
calling common.skip() on hosts that do not support IPv6. However,
by this point, there were several outstanding common.mustCall()
invocations. The process.exit() in common.skip() triggered
those common.mustCall()s as errors.

Fixes: nodejs/node#15419
PR-URL: nodejs/node#15421
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
@cjihrig @jasnell
test: don't skip when common.mustCall() is pending
ba96c8f 
The test parallel/test-dgram-multicast-set-interface.js was
calling common.skip() on hosts that do not support IPv6. However,
by this point, there were several outstanding common.mustCall()
invocations. The process.exit() in common.skip() triggered
those common.mustCall()s as errors.

Fixes: #15419
PR-URL: #15421
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@jasnell jasnell mentioned this pull request Sep 20, 2017
Merged
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@cjihrig
test: don't skip when common.mustCall() is pending
eeedd77 
The test parallel/test-dgram-multicast-set-interface.js was
calling common.skip() on hosts that do not support IPv6. However,
by this point, there were several outstanding common.mustCall()
invocations. The process.exit() in common.skip() triggered
those common.mustCall()s as errors.

Fixes: nodejs/node#15419
PR-URL: nodejs/node#15421
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Jan 16, 2018
Closed
MylesBorins pushed a commit that referenced this pull request Jan 17, 2018
@cjihrig @MylesBorins
test: don't skip when common.mustCall() is pending
0912453 
The test parallel/test-dgram-multicast-set-interface.js was
calling common.skip() on hosts that do not support IPv6. However,
by this point, there were several outstanding common.mustCall()
invocations. The process.exit() in common.skip() triggered
those common.mustCall()s as errors.

Fixes: #15419
PR-URL: #15421
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@cjihrig @MylesBorins
test: don't skip when common.mustCall() is pending
4772ed9 
The test parallel/test-dgram-multicast-set-interface.js was
calling common.skip() on hosts that do not support IPv6. However,
by this point, there were several outstanding common.mustCall()
invocations. The process.exit() in common.skip() triggered
those common.mustCall()s as errors.

Fixes: #15419
PR-URL: #15421
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@cjihrig @MylesBorins
test: don't skip when common.mustCall() is pending
7b801b5 
The test parallel/test-dgram-multicast-set-interface.js was
calling common.skip() on hosts that do not support IPv6. However,
by this point, there were several outstanding common.mustCall()
invocations. The process.exit() in common.skip() triggered
those common.mustCall()s as errors.

Fixes: #15419
PR-URL: #15421
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

Assignees
No one assigned
Labels
dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.