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: use invalid host according to RFC2606 #14863

Closed
wants to merge 2 commits into from

Conversation

tniessen
Copy link
Member

Refs: #14781
Refs: https://tools.ietf.org/html/rfc2606#section-2

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@tniessen tniessen added the test Issues and PRs related to the tests. label Aug 16, 2017
@tniessen tniessen self-assigned this Aug 16, 2017
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 16, 2017
@silverwind
Copy link
Contributor

This could be used in more tests, for example test/internet/test-dns.js.

@tniessen
Copy link
Member Author

This could be used in more tests, for example test/internet/test-dns.js.

@silverwind Are you referring to this part of the file?

const invalidHost = 'fasdfdsaf';
assert.throws(() => {
dns.lookupService(invalidHost, 0, common.mustNotCall());
}, common.expectsError({
code: 'ERR_INVALID_OPT_VALUE',
type: TypeError,
message: `The value "${invalidHost}" is invalid for option "host"`
}));

@silverwind
Copy link
Contributor

silverwind commented Aug 17, 2017

@lpinca yes, that's another example, but I was refering to the file in internet tests, for example:

const req = dns.lookup('does.not.exist', 4, function(err, ip, family) {

const req = dns.lookup('nosuchhostimsure', function(err) {

@lpinca
Copy link
Member

lpinca commented Aug 17, 2017

@silverwind I guess the comment was for @tniessen.

Copy link
Contributor

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Thanks

@tniessen
Copy link
Member Author

Landed in 467385a. If you see any other occurrences which should be changed, feel free to comment here.

@tniessen tniessen closed this Aug 18, 2017
tniessen added a commit that referenced this pull request Aug 18, 2017
PR-URL: #14863
Refs: #14781
Refs: https://tools.ietf.org/html/rfc2606#section-2
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@MylesBorins
Copy link
Contributor

This appears to rely on a semver-major change. setting dont-land-on-v8.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants