-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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, url: break up test-url.js #11049
Conversation
415352d
to
54fbec1
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.
Rubber-stamp LGTM, this seems to make sense to me.
CI: https://ci.nodejs.org/job/node-test-pull-request/6098/, while waiting for more reviews. |
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.
Rubber stamp LGTM
PR-URL: #11049 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #11049 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This isn't landing cleanly on v7.x. Mind submitting a backport PR? |
@evanlucas Looks like there is a test depending on #9292, which is semver-major, so this one can be skipped. |
@joyeecheung Thanks for checking! |
Break up test-url.js into multiple files by the functionality they test against, so they would be easier to compare with their whatwg url counterparts.
Also remove some unecessary
eslint-disable max-len
and format the error messages using templates(the disabling should be apply to the test cases, which might be inevitably too long, but not to the actual testing code).Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test, url