GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||
GeezerVison is showing github.com — test: add non-internet resolveAny tests by addaleax · Pull Request #13883 · nodejs/node Conversation
Member
This is a bit of a check to see how people feel about having this kind of test. Ref: #13137 /cc @nodejs/testing @XadillaX Checklist
Affected core subsystem(s)test/dns
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
cjihrig
approved these changes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a specific rule that can be turned off here?
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Not that it really matters, but why not just use standard quote characters here?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
But these are the proper quotation marks? ;) (All trolling aside, I’m just used to typing these if it’s not for code, that’s all.)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
There are a couple uses of split() on single characters in this PR. Would the string version work (split('.'))? It seems more readable.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@cjihrig I just literally forgot that split can work with plain strings. Updated :)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Let's do the opposite, publish this as |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
How about:
const kLengthMask = 0xC0; const kOffsetMask = ~0xC000;
I get the idiom, but I have to use my fingers to figure out 0xC000 === 0o140000 == 0b1100000000000000 === 0x3 << 14 and my mind just has to do that. Using a const will allow me to abstract it away 😅
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
😵
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
·@refack This is not a very helpful comment, I have no idea what you are trying to say.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Please define this as a const so it's more obvious what are flags you want.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Do you need this?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yes, the linter complains otherwise.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
You could replace it with /* eslint-disable required-modules */ at the top of the file the way it is in test/common/wpt.js. I think I'd prefer that. If it's leaking globals, then any tests that use it will complain, so we're covered without importing the common module.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@Trott Sorry for being so late, I’ve updated this PR with eslint-disable required-modules
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'm generally +1 on having this (and love seeing this kind of work) but my one concern is that there are no tests to verify that the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
If we run |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Just to mention it again, the main scenario that prompted this is checking how our code handles invalid responses, something we won’t be able to do with a “proper” DNS server. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yeah, good point :-) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Original commit message:
ares_parse_naptr_reply: make buffer length check more accurate
9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
for records parsed by `ares_parse_naptr_reply()`. However, that
function is designed to parse replies which also contain non-NAPTR
records; for A records, the `rr_len > 7` check will fail as there
are only 4 bytes of payload.
In particular, parsing ANY replies for NAPTR records was broken
by that patch.
Fix that by moving the check into the case in which it is already
known that the record is a NAPTR record.
Ref: c-ares/c-ares@18ea996
This is a bit of a check to see how people feel about having this kind of test. Ref: nodejs#13137 PR-URL: nodejs#13883 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
|
Added c-ares/c-ares@18ea996 because both the test proposed here and our existing internet tests ( CI is green: https://ci.nodejs.org/job/node-test-commit/11128/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@nodejs/lts The first commit here (e76c49d) should make it to v4.x and v6.x, it fixes a bug from this month’s security release. I’ve added |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Original commit message:
ares_parse_naptr_reply: make buffer length check more accurate
9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
for records parsed by `ares_parse_naptr_reply()`. However, that
function is designed to parse replies which also contain non-NAPTR
records; for A records, the `rr_len > 7` check will fail as there
are only 4 bytes of payload.
In particular, parsing ANY replies for NAPTR records was broken
by that patch.
Fix that by moving the check into the case in which it is already
known that the record is a NAPTR record.
Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
This is a bit of a check to see how people feel about having this kind of test. Ref: #13137 PR-URL: #13883 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
Original commit message:
ares_parse_naptr_reply: make buffer length check more accurate
9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
for records parsed by `ares_parse_naptr_reply()`. However, that
function is designed to parse replies which also contain non-NAPTR
records; for A records, the `rr_len > 7` check will fail as there
are only 4 bytes of payload.
In particular, parsing ANY replies for NAPTR records was broken
by that patch.
Fix that by moving the check into the case in which it is already
known that the record is a NAPTR record.
Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
Original commit message:
ares_parse_naptr_reply: make buffer length check more accurate
9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
for records parsed by `ares_parse_naptr_reply()`. However, that
function is designed to parse replies which also contain non-NAPTR
records; for A records, the `rr_len > 7` check will fail as there
are only 4 bytes of payload.
In particular, parsing ANY replies for NAPTR records was broken
by that patch.
Fix that by moving the check into the case in which it is already
known that the record is a NAPTR record.
Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
Original commit message:
ares_parse_naptr_reply: make buffer length check more accurate
9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
for records parsed by `ares_parse_naptr_reply()`. However, that
function is designed to parse replies which also contain non-NAPTR
records; for A records, the `rr_len > 7` check will fail as there
are only 4 bytes of payload.
In particular, parsing ANY replies for NAPTR records was broken
by that patch.
Fix that by moving the check into the case in which it is already
known that the record is a NAPTR record.
Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
Original commit message:
ares_parse_naptr_reply: make buffer length check more accurate
9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
for records parsed by `ares_parse_naptr_reply()`. However, that
function is designed to parse replies which also contain non-NAPTR
records; for A records, the `rr_len > 7` check will fail as there
are only 4 bytes of payload.
In particular, parsing ANY replies for NAPTR records was broken
by that patch.
Fix that by moving the check into the case in which it is already
known that the record is a NAPTR record.
Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
Original commit message:
ares_parse_naptr_reply: make buffer length check more accurate
9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
for records parsed by `ares_parse_naptr_reply()`. However, that
function is designed to parse replies which also contain non-NAPTR
records; for A records, the `rr_len > 7` check will fail as there
are only 4 bytes of payload.
In particular, parsing ANY replies for NAPTR records was broken
by that patch.
Fix that by moving the check into the case in which it is already
known that the record is a NAPTR record.
Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
Original commit message:
ares_parse_naptr_reply: make buffer length check more accurate
9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
for records parsed by `ares_parse_naptr_reply()`. However, that
function is designed to parse replies which also contain non-NAPTR
records; for A records, the `rr_len > 7` check will fail as there
are only 4 bytes of payload.
In particular, parsing ANY replies for NAPTR records was broken
by that patch.
Fix that by moving the check into the case in which it is already
known that the record is a NAPTR record.
Ref: c-ares/c-ares@18ea996
PR-URL: nodejs#13883
Reviewed-By: James M Snell <[email protected]>
Original commit message:
ares_parse_naptr_reply: make buffer length check more accurate
9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
for records parsed by `ares_parse_naptr_reply()`. However, that
function is designed to parse replies which also contain non-NAPTR
records; for A records, the `rr_len > 7` check will fail as there
are only 4 bytes of payload.
In particular, parsing ANY replies for NAPTR records was broken
by that patch.
Fix that by moving the check into the case in which it is already
known that the record is a NAPTR record.
Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
Original commit message:
ares_parse_naptr_reply: make buffer length check more accurate
9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
for records parsed by `ares_parse_naptr_reply()`. However, that
function is designed to parse replies which also contain non-NAPTR
records; for A records, the `rr_len > 7` check will fail as there
are only 4 bytes of payload.
In particular, parsing ANY replies for NAPTR records was broken
by that patch.
Fix that by moving the check into the case in which it is already
known that the record is a NAPTR record.
Ref: c-ares/c-ares@18ea996
PR-URL: nodejs/node#13883
Reviewed-By: James M Snell <[email protected]>
Original commit message:
ares_parse_naptr_reply: make buffer length check more accurate
9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
for records parsed by `ares_parse_naptr_reply()`. However, that
function is designed to parse replies which also contain non-NAPTR
records; for A records, the `rr_len > 7` check will fail as there
are only 4 bytes of payload.
In particular, parsing ANY replies for NAPTR records was broken
by that patch.
Fix that by moving the check into the case in which it is already
known that the record is a NAPTR record.
Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.