-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
test: skip whatwg url parse tests when icu is missing #9246
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
Conversation
|
It looks like we're using different methods throughout the test suite to figure out whether we have Intl support. Can you perhaps create a |
|
good idea |
61a0186 to
3dcd9f3
Compare
|
nointl specific CI: https://ci.nodejs.org/job/node-test-commit-linux-nointl/2/ |
test/common.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the other common.hasFoo properties are either simple data properties or getters, not functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. switched to a getter
3dcd9f3 to
3f871c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intl
the WHATWG url parser relies on ICU's punycode implementation. A handful of the standard tests fail when ICU is not present because of the additional checks that are not implemented. For now, skip the parse and setter tests if ICU is not present.
3f871c1 to
99cd46d
Compare
|
Regular CI: https://ci.nodejs.org/job/node-test-pull-request/4641/ |
|
Unrelated failures in CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Apropos the WHATWG URL tests, would it be possible to add a 'needs intl' heuristic? Something like |
|
Yeah, I was considering adding a check but making it more explicit by adding metadata to the test fixture to say whether or not ICU is required for that item. Takes the guess work out of it. |
|
that said, this commit is intended primarily to make sure the non-intl CI is running green until I can get that done. Since the new URL parser is an experimental feature still, it shouldn't be holding things up |
PR-URL: #9246 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
the WHATWG url parser relies on ICU's punycode implementation. A handful of the standard tests fail when ICU is not present because of the additional checks that are not implemented. For now, skip the parse and setter tests if ICU is not present. PR-URL: #9246 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
Landed in d7e4ae1...52670fc |
PR-URL: #9246 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
the WHATWG url parser relies on ICU's punycode implementation. A handful of the standard tests fail when ICU is not present because of the additional checks that are not implemented. For now, skip the parse and setter tests if ICU is not present. PR-URL: #9246 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: nodejs#9246 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test
Description of change
The WHATWG url parser relies on ICU's punycode implementation. A handful of the standard tests fail when ICU is not present because of the additional checks that are not implemented. For now, skip the parse tests if ICU is not present.