-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: print helpful err msg on test-dns-ipv6.js #3501
Conversation
@@ -165,7 +165,8 @@ TEST(function test_lookup_all_ipv6(done) { | |||
assert.ok(ips.length > 0); | |||
|
|||
ips.forEach(function(ip) { | |||
assert.ok(isIPv6(ip.address)); | |||
assert.ok(isIPv6(ip.address), | |||
'Invalid IPv6: ' + ip.address.toString()); |
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.
style nit: line up first and second lines. e.g.
assert.ok(isIPv6(ip.address),
'Invalid IPv6: ' + ip.address.toString());
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.
one comment. otherwise LGTM. |
LGTM |
Hmmm, then we should find the root cause and fix it. Do you have any instances to show? |
@thefourtheye we want to fix it too. We do have a case showing that this assertion fails but we don't know exactly what is wrong with the IP address that cause the failure, and we so far have no luck reproduce the problem. The fix is to add additional information to hopefully catch the wrong IP address when it happens again. |
The test sometimes fail on an assertion but no useful error message was generated for debugging. Modify the test to generate useful debugging message. PR-URL: nodejs#3501 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Landed in f4c0ed4 |
Hello @Trott , would you please also back port (cherry-pick) it to v4.x? Thanks. |
I thought CI had already run on this but it looks like it didn't. Here it is now: https://ci.nodejs.org/job/node-test-pull-request/612/ Sorry about that, everyone. (Fortunately, it's a trivial change exceedingly unlikely to do any harm.) |
Ha ha ha @nodejs/punished |
The test sometimes fail on an assertion but no useful error message was generated for debugging. Modify the test to generate useful debugging message. PR-URL: #3501 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
The test sometimes fail on an assertion but no useful error message was generated for debugging. Modify the test to generate useful debugging message. PR-URL: #3501 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Landed in v4.x-staging in 9e05af7 |
The test sometimes fail on an assertion but no useful error message was generated for debugging. Modify the test to generate useful debugging message. PR-URL: #3501 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Test sometimes fail on this assertion but no useful error message was generated for debugging. Modify the test to generate useful debugging message.