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.

Sign up for GitHub

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

Jump to bottom

test: test-net-dns-error.js fails on SunOS #31780

Closed
wants to merge 2 commits into from
Closed

test: test-net-dns-error.js fails on SunOS #31780

wants to merge 2 commits into from

Conversation

batrla
Copy link
Contributor

@batrla batrla commented Feb 13, 2020

Test test-net-dns-error.js causes assertion failure on SunOS,
test expects ENOTFOUND, but OS returns EAI_FAIL.

Maximum length of a host name is 63 characters. Test test-net-dns-error.js
makes a connection attempt to invalid host name (longer than maximum by 1).
Such connection attempt on SunOS returns permanent failure (EAI_FAIL)
as invalid hostname won't be ever resolved.

Checklist

test: test-net-dns-error.js fails on SunOS
3c225fb 
Test test-net-dns-error.js causes assertion failure on SunOS,
test expects ENOTFOUND, but OS returns EAI_FAIL.

Maximum length of a host name is 63 characters. Test test-net-dns-error.js
makes a connection attempt to invalid host name (longer than maximum). Such
connection attempt on SunOS returns permanent failure (EAI_FAIL) as invalid
hostname won't be ever resolved.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 13, 2020
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 16, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29171/

@batrla
Copy link
Contributor Author

batrla commented Feb 19, 2020

Sorry, the suggested fix caused test regression in CI on SmartOS, where
the same test got ENOTFOUND instead of expected EAI_FAIL.
My testing was on Solaris 11.4 OS.

Test regression was either due to:

  • divergency between Solaris and SmartOS (both are being SunOS derivates)
  • difference in name services configuration on my machine and CI (perhaps
    nsswitch.conf(5) or resolv.conf(5))

I don't know exact cause. It seems to me it's not trivial to determine whether
the OS returns EAI_FAIL or ENOTFOUND. My proposal is to:

  • Allow both EAI_FAIL and ENOTFOUND in test-net-dns-error.js

    Let's assume that it is OS specific or resolver configuration specific
    whether the system actually returns EAI_FAIL or ENOTFOUND in response to
    attempt to resolve an illegal hostname using getaddrinfo(3C). EAI_FAIL
    indicates permanent failure:

    EAI_FAIL Non-recoverable failure in name resolution has occurred.

The proposal is dup of recent PR by @cjihrig in test-http-dns-error.js:
test: allow EAI_FAIL in test-http-dns-error.js #27500

@Trott
Copy link
Member

Trott commented Feb 20, 2020

  • Allow both EAI_FAIL and ENOTFOUND in test-net-dns-error.js

Yes, I think that would be the thing to do.

@Trott
Copy link
Member

Trott commented Feb 20, 2020

  • Allow both EAI_FAIL and ENOTFOUND in test-net-dns-error.js

Yes, I think that would be the thing to do.

You might also want to include a comment explaining why both are being allowed and linking to this pull request/discussion. Otherwise, someone might subsequently remove the allowance for EAI_FAIL (and CI will pass when they do).

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29248/

@batrla
Copy link
Contributor Author

batrla commented Feb 20, 2020

You might also want to include a comment explaining why both are being allowed and linking to this pull request/discussion. Otherwise, someone might subsequently remove the allowance for EAI_FAIL (and CI will pass when they do).

I added following comment:

// Resolving hostname > 63 characters may return EAI_FAIL (permanent failure).

If the comment is insufficient, I'll be happy to make it more explicit next time the test breaks.

@Trott
Copy link
Member

Trott commented Feb 21, 2020

If the comment is insufficient, I'll be happy to make it more explicit next time the test breaks.

That's good, but perhaps we should also mention specifically that it fails that way on SunOS and what version of SunOS? (This is just a suggestion though. I'm not blocking this from landing or anything.)

addaleax pushed a commit that referenced this pull request Mar 9, 2020
@addaleax
test: allow EAI_FAIL in test-net-dns-error.js
077f9dc 
Test test-net-dns-error.js causes assertion failure on SunOS,
test expects ENOTFOUND, but OS returns EAI_FAIL.

Maximum length of a host name is 63 characters.
Test test-net-dns-error.js makes a connection attempt to
invalid host name (longer than maximum). Such
connection attempt on SunOS returns permanent failure
(EAI_FAIL) as invalid hostname won't be ever resolved.

PR-URL: #31780
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@addaleax
Copy link
Member

addaleax commented Mar 9, 2020

Landed in 077f9dc

@addaleax addaleax closed this Mar 9, 2020
MylesBorins pushed a commit that referenced this pull request Mar 9, 2020
@MylesBorins
test: allow EAI_FAIL in test-net-dns-error.js
6545d1a 
Test test-net-dns-error.js causes assertion failure on SunOS,
test expects ENOTFOUND, but OS returns EAI_FAIL.

Maximum length of a host name is 63 characters.
Test test-net-dns-error.js makes a connection attempt to
invalid host name (longer than maximum). Such
connection attempt on SunOS returns permanent failure
(EAI_FAIL) as invalid hostname won't be ever resolved.

PR-URL: #31780
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@codebytere
test: allow EAI_FAIL in test-net-dns-error.js
e827b8e 
Test test-net-dns-error.js causes assertion failure on SunOS,
test expects ENOTFOUND, but OS returns EAI_FAIL.

Maximum length of a host name is 63 characters.
Test test-net-dns-error.js makes a connection attempt to
invalid host name (longer than maximum). Such
connection attempt on SunOS returns permanent failure
(EAI_FAIL) as invalid hostname won't be ever resolved.

PR-URL: #31780
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 23, 2020
@codebytere
test: allow EAI_FAIL in test-net-dns-error.js
0a3cde5 
Test test-net-dns-error.js causes assertion failure on SunOS,
test expects ENOTFOUND, but OS returns EAI_FAIL.

Maximum length of a host name is 63 characters.
Test test-net-dns-error.js makes a connection attempt to
invalid host name (longer than maximum). Such
connection attempt on SunOS returns permanent failure
(EAI_FAIL) as invalid hostname won't be ever resolved.

PR-URL: #31780
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
@codebytere
test: allow EAI_FAIL in test-net-dns-error.js
b4c407f 
Test test-net-dns-error.js causes assertion failure on SunOS,
test expects ENOTFOUND, but OS returns EAI_FAIL.

Maximum length of a host name is 63 characters.
Test test-net-dns-error.js makes a connection attempt to
invalid host name (longer than maximum). Such
connection attempt on SunOS returns permanent failure
(EAI_FAIL) as invalid hostname won't be ever resolved.

PR-URL: #31780
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@batrla @nodejs-github-bot @Trott @addaleax @bnoordhuis @jasnell @lpinca @cjihrig