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: expect error for test_lookup_ipv6_hint on FreeBSD #2724

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 7, 2015

FreeBSD does not support the V4MAPPED flag so expect an error.

/cc @jbergstroem @thefourtheye

FreeBSD does not support the V4MAPPED flag so expect an error.
@mscdex mscdex added dns Issues and PRs related to the dns subsystem. test Issues and PRs related to the tests. freebsd Issues and PRs related to the FreeBSD platform. labels Sep 7, 2015
assert(err instanceof Error);
assert.strictEqual(err.code, 'EAI_BADFLAGS');
assert.strictEqual(err.hostname, 'www.google.com');
assert.ok(/getaddrinfo EAI_BADFLAGS/.test(err.message));
Copy link
Contributor

Choose a reason for hiding this comment

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

Simply assert

Copy link
Member Author

Choose a reason for hiding this comment

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

I used assert.ok() to match the style of the other regular expression-based assertions in the file. (There are five other instances.)

@thefourtheye
Copy link
Contributor

This is explicit and I like it. LGTM

@Trott
Copy link
Member Author

Trott commented Sep 7, 2015

A test should be added that uses flags that FreeBSD supports, but I'd prefer that to be in a separate PR rather than bundled with this one. I'm just making a note here so I actually do it.

@Trott
Copy link
Member Author

Trott commented Sep 7, 2015

Obligatory CI, although the CI won't touch this code other than linting it: https://ci.nodejs.org/job/node-test-pull-request/263/

@Trott
Copy link
Member Author

Trott commented Sep 7, 2015

Having a conversation with myself a bit here, but in response to my comment about there needing to be another PR with another test: Upon further investigation, there' probably doesn't need to be another test in addition to this one. It looks like dns only accepts two flags, and there's already a test for the other one. There could be another test (to make sure the flag works properly with ipv6, I suppose). But strictly speaking, I don't think we need it. It depends how many combinations we want to test. A code coverage tool would be useful to have here to see if we're missing any code paths which might argue for another test.

@jbergstroem
Copy link
Member

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Sep 9, 2015
FreeBSD does not support the V4MAPPED flag so expect an error.

This is a partial fix for nodejs#2468.
It only fixes it on FreeBSD. Failures on other platforms are due to
other reasons and need to be fixed separately.

PR-URL: nodejs#2724
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Fixes: nodejs#2468
@Trott
Copy link
Member Author

Trott commented Sep 9, 2015

Landed in f8152df

@Trott Trott closed this Sep 9, 2015
@akroeger1984 akroeger1984 mentioned this pull request Sep 9, 2015
Trott added a commit that referenced this pull request Sep 11, 2015
FreeBSD does not support the V4MAPPED flag so expect an error.

This is a partial fix for #2468.
It only fixes it on FreeBSD. Failures on other platforms are due to
other reasons and need to be fixed separately.

PR-URL: #2724
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Fixes: #2468
Trott added a commit that referenced this pull request Sep 12, 2015
FreeBSD does not support the V4MAPPED flag so expect an error.

This is a partial fix for #2468.
It only fixes it on FreeBSD. Failures on other platforms are due to
other reasons and need to be fixed separately.

PR-URL: #2724
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Fixes: #2468
@rvagg rvagg mentioned this pull request Sep 12, 2015
@Fishrock123 Fishrock123 mentioned this pull request Sep 13, 2015
7 tasks
@rvagg rvagg mentioned this pull request Sep 22, 2015
@Trott Trott deleted the test-dns-freebsd2 branch January 9, 2022 22:03
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. freebsd Issues and PRs related to the FreeBSD platform. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants