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: print helpful err msg on test-dns-ipv6.js #3501

Closed
wants to merge 1 commit into from

Conversation

john-yan
Copy link

Test sometimes fail on this assertion but no useful error message was generated for debugging. Modify the test to generate useful debugging message.

@@ -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());
Copy link
Contributor

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());

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

@trevnorris
Copy link
Contributor

one comment. otherwise LGTM.

@mscdex mscdex added dns Issues and PRs related to the dns subsystem. test Issues and PRs related to the tests. labels Oct 23, 2015
@Trott
Copy link
Member

Trott commented Oct 24, 2015

LGTM

@thefourtheye
Copy link
Contributor

Test sometimes fail on this assertion...

Hmmm, then we should find the root cause and fix it. Do you have any instances to show?

@john-yan
Copy link
Author

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

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 25, 2015
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>
@Trott
Copy link
Member

Trott commented Oct 25, 2015

Landed in f4c0ed4

@Trott Trott closed this Oct 25, 2015
@john-yan
Copy link
Author

Hello @Trott , would you please also back port (cherry-pick) it to v4.x? Thanks.

@Trott
Copy link
Member

Trott commented Oct 25, 2015

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

@Trott
Copy link
Member

Trott commented Oct 25, 2015

@john-yan I've added the lts-watch-v4.x tag so @jasnell or whoever is doing LTS back-porting can make the call.

@thefourtheye
Copy link
Contributor

Ha ha ha @nodejs/punished

rvagg pushed a commit that referenced this pull request Oct 26, 2015
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>
@rvagg rvagg mentioned this pull request Oct 27, 2015
jasnell pushed a commit that referenced this pull request Oct 28, 2015
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>
@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

Landed in v4.x-staging in 9e05af7

jasnell pushed a commit that referenced this pull request Oct 29, 2015
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants