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, url: break up test-url.js #11049

Closed
wants to merge 2 commits into from

Conversation

@joyeecheung
Copy link
Member

commented Jan 28, 2017

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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, url

@joyeecheung joyeecheung force-pushed the joyeecheung:refactor-url-test branch to 54fbec1 Jan 28, 2017

@addaleax
Copy link
Member

left a comment

Rubber-stamp LGTM, this seems to make sense to me.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/6098/, while waiting for more reviews.

@cjihrig
Copy link
Contributor

left a comment

Rubber stamp LGTM

jasnell added a commit that referenced this pull request Jan 30, 2017
url, test: break up test-url.js
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>
jasnell added a commit that referenced this pull request Jan 30, 2017
test: remove unnecessary eslint-disable max-len
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>
@jasnell

This comment has been minimized.

Copy link
Member

commented Jan 30, 2017

Landed in 147d2a6 and 7854503

@evanlucas

This comment has been minimized.

Copy link
Member

commented Jan 31, 2017

This isn't landing cleanly on v7.x. Mind submitting a backport PR?

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

@evanlucas Looks like there is a test depending on #9292, which is semver-major, so this one can be skipped.

@evanlucas

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

@joyeecheung Thanks for checking!

@joyeecheung joyeecheung deleted the joyeecheung:refactor-url-test branch Feb 19, 2017

@jasnell jasnell referenced this pull request Apr 4, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.