-
Notifications
You must be signed in to change notification settings - Fork 30k
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: skip test_lookup_ipv6_hint on FreeBSD #2716
Conversation
/cc @jbergstroem |
Fixes 2 out of 8 systems listed for the test at #2468 |
CI: https://ci.nodejs.org/job/node-test-pull-request/259/ (Kind of pointless because the tests in |
LGTM |
FreeBSD does not support the V4MAPPED flag so skip the test that uses it.
d83bd58
to
46ac743
Compare
// FreeBSD does not support V4MAPPED flag. | ||
if (process.platform === 'freebsd') { | ||
console.log( | ||
'1..0 # Skipped: test_lookup_ipv6_hint is disabled on FreeBSD.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this message be changed to FreeBSD does not support V4MAPPED flag.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thefourtheye if so, perhaps squeeze both test_lookup_ipv6_hint
and v4mapped
in there? wouldn't tap just report internet/test-dns.js
and not the actual test name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbergstroem Unfortunately, nope. It will not know the specific test :( I thought that if we specified the actual reason, it would be better to understand why it is skipped.
Now there is another interesting thing here. If we skip more than one tests in a single file, then there will be two skip TAP lines. I wonder how the output will be displayed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thefourtheye yeah, not too much to do about "double skipping" unless we're keen to start rewriting larger parts of our tests. Long term we should look at replacing the current runner with something build on top of node-tap
or similar. There's an issue about this somewhere..
anyway, moving forward; I'm ok with either message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long term we should look at replacing the current runner with something build on top of node-tap or similar
cheers
LGTM |
I wonder if I should rewrite it so that it checks the error on FreeBSD rather than skips the test? That has the benefits of:
|
How about we do this instead? #2724 |
Closing in favor of #2724 |
FreeBSD does not support the V4MAPPED flag so skip the test that uses it.
I hacked the Makefile to run this on CI to confirm that it had been failing and that this change would cause it to pass. There are other failures, but they are for other tests. https://ci.nodejs.org/job/node-test-commit-other/518/