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: expand test coverage for url.js #8859

Closed
wants to merge 1 commit into from

Conversation

jun-oka
Copy link
Contributor

@jun-oka jun-oka commented Sep 30, 2016

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

test url
lib/url

Description of change

Currently Line 156 of lib/url.js is not reachable from test-url because there are no example URL which has white space in the front of url.
I added one example which can reach that line.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 30, 2016
@Trott
Copy link
Member

Trott commented Sep 30, 2016

The new test case looks good to me and causes line 156 to be exercised whereas current tests do not. 👍

Can you update the commit message so the first line is less than 50 characters? I'd recommend something like this:

test: increase test coverage for url.js

Currently Line 156 of lib/url.js is not reachable from test-url because there are no example URL which has white space in the front of url.
I added one example which can reach that line.
@jun-oka
Copy link
Contributor Author

jun-oka commented Sep 30, 2016

@Trott Thank you for the notice. I will pay more attention about the number of characters for the first line from next commit.

@mscdex mscdex added the url Issues and PRs related to the legacy built-in url module. label Sep 30, 2016
@jun-oka jun-oka changed the title test: add example in url test to reach line 156 in url.js test: expand test coverage for url.js Sep 30, 2016
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member

jasnell commented Sep 30, 2016

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@fhinkel
Copy link
Member

fhinkel commented Oct 3, 2016

Thank you 👍 . Landed in cad2a2f.

@fhinkel fhinkel closed this Oct 3, 2016
fhinkel pushed a commit that referenced this pull request Oct 3, 2016
Currently, line 156 of lib/url.js is not reachable from test-url because
there is no example URL which has a white space in the front of the url.
I added one example which can reach that line.

PR-URL: #8859
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 6, 2016
Currently, line 156 of lib/url.js is not reachable from test-url because
there is no example URL which has a white space in the front of the url.
I added one example which can reach that line.

PR-URL: #8859
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Currently, line 156 of lib/url.js is not reachable from test-url because
there is no example URL which has a white space in the front of the url.
I added one example which can reach that line.

PR-URL: #8859
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 18, 2016
Currently, line 156 of lib/url.js is not reachable from test-url because
there is no example URL which has a white space in the front of the url.
I added one example which can reach that line.

PR-URL: #8859
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
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. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.