-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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-dns-ipv6.js if ipv6 is unavailable #3444
Conversation
@@ -40,6 +40,11 @@ function checkWrap(req) { | |||
assert.ok(typeof req === 'object'); | |||
} | |||
|
|||
if (!common.hasIPv6) { | |||
console.log('1..0 # Skipped: ipv6 part of test, no IPv6 support'); |
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.
the entire test is ipv6. You might want to make copy specify the entire test has failed
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.
yeah, good catch. thanks.
Issue number: #3443 |
LGTM |
@@ -40,6 +40,11 @@ function checkWrap(req) { | |||
assert.ok(typeof req === 'object'); | |||
} | |||
|
|||
if (!common.hasIPv6) { |
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.
LGTM, but couldn't you move this a little closer to the top of the file?
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.
if it is put ahead of some of the variable declarations in the file then the process exit event won't pass.
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.
at least needs to be below these
https://github.com/john-yan/node/blob/master/test/internet/test-dns-ipv6.js#L10-L13
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.
done.
LGTM |
1 similar comment
LGTM |
@@ -12,6 +12,11 @@ var expected = 0, | |||
running = false, | |||
queue = []; | |||
|
|||
if (!common.hasIPv6) { | |||
console.log('1..0 # Skipped this test, no IPv6 support'); |
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.
One last nit, the style we use is 1..0 # Skipped:
:)
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.
updated. thanks.
Test should be skipped if ipv6 is unavailable on the running system. Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: #3444
Landed in a1886cf |
Test should be skipped if ipv6 is unavailable on the running system. Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: #3444
Test should be skipped if ipv6 is unavailable on the running system. Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: #3444
Landed in v4.x-staging in 49549c9 |
Test should be skipped if ipv6 is unavailable on the running system. Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: #3444
Test should be skipped if ipv6 is unavailable on the running system.