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: don't skip when common.mustCall() is pending #15421

Merged
merged 1 commit into from
Sep 17, 2017
Merged

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
@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

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

@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

@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

@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.

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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 17, 2017

@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
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jasnell pushed a commit that referenced this pull request Sep 20, 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
PR-URL: #15421
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 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: nodejs/node#15419
PR-URL: nodejs/node#15421
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 17, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.