test: update test for url.parse() to match the exact error through re… by andreicioromila · Pull Request #12879 · nodejs/node

andreicioromila · 2017-05-07T11:30:01Z

When called with invalid values, url.parse() throws a TypeError with a specific message. Update test to check that the message matches the error thrown.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

…gexp check

addaleax · 2017-05-07T11:49:36Z

test/parallel/test-url-parse-invalid-input.js

-].forEach(function(val) {
-  assert.throws(function() { url.parse(val); }, TypeError);
+].forEach((val) => {
+  assert.throws(() => { url.parse(val); }, /^TypeError: Parameter "url" must be a string, not undefined|boolean|number|object$/);


I think you want to group undefined|boolean|number|object in parentheses, right?
And could you also wrap at the , (align the function and the regexp vertically)? We try to keep lines under 80 characters – for regexps that’s not always possible, but it would still be nice to try and keep lines short :)

I have included the suggested changes

lpinca · 2017-05-07T12:42:14Z

Minor request that can be ignored: can you please update the commit message?
Something like this should work.

test: add regex check in test-url-parse-invalid-input

Use a regex to validate the error message.

vsemozhetbyt · 2017-05-07T14:31:04Z

FWIW: I've posted an ESlint issue concerning this case:

assert.throws(() => { url.parse('http://%E0%A4%A@fail'); }, /^URIError: URI malformed$/);

DavidCai1111 · 2017-05-07T14:55:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7931/

TimothyGu

Otherwise LGTM.

TimothyGu · 2017-05-07T18:58:34Z

test/parallel/test-url-parse-invalid-input.js

@@ -13,8 +13,9 @@ const url = require('url');
  0,
  [],
  {}


Add symbols while at it?

thefourtheye · 2017-05-08T15:38:34Z

test/parallel/test-url-parse-invalid-input.js

-  assert.throws(function() { url.parse(val); }, TypeError);
+].forEach((val) => {
+  assert.throws(() => { url.parse(val); },
+                /^TypeError: Parameter "url" must be a string, not (undefined|boolean|number|object)$/);


Our linter will not like this line, as it has more than 80 characters... :(

@thefourtheye this is no longer true. Inline regex are now whitelisted.

@lpinca Oops, sorry then. Thanks for letting me know... Checking it now.

jasnell · 2017-05-09T19:41:32Z

test/parallel/test-url-parse-invalid-input.js

 });

-assert.throws(function() { url.parse('http://%E0%A4%A@fail'); }, /^URIError: URI malformed$/);
+assert.throws(() => { url.parse('http://%E0%A4%A@fail'); }, /^URIError: URI malformed$/);


I believe this one is still long. I'm fine with the inline regex being whitelisted, but I think if there are multiple arguments we should still try to split them by line to avoid the long lines if possible.

jasnell · 2017-05-10T19:17:58Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/7997/

andreicioromila · 2017-05-12T10:17:20Z

Since there was an update on this PR, can everyone please review it again so it can be merged?

jasnell

Still LGTM

jasnell · 2017-05-12T15:18:29Z

test/parallel/test-url-parse-invalid-input.js

 });

-assert.throws(function() { url.parse('http://%E0%A4%A@fail'); }, /^URIError: URI malformed$/);
+assert.throws(() => { url.parse('http://%E0%A4%A@fail'); },


very minor nit: the { and } around the body of the arrow function here can be omitted but isn't necessary.

I would say the braces should not be omitted. That would change it from merely running url.parse() to returning a value. If it was { return url.parse(...); } then omitting the braces (and the return) would make sense. It's mostly a semantic difference, but I think it's important.

lpinca · 2017-05-12T15:20:44Z

Still LGTM.

andreicioromila · 2017-05-16T07:57:44Z

What can I do to improve this PR?
Or can this be merged?

lpinca · 2017-05-16T08:02:56Z

@andreicioromila it can be merged. I'll run a new CI and if there are no surprises I'll land it.

lpinca · 2017-05-16T08:04:04Z

CI 3: https://ci.nodejs.org/job/node-test-pull-request/8105/

lpinca · 2017-05-16T10:15:26Z

Landed in 7906ed5.

Use a regex to validate the error message. PR-URL: #12879 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: David Cai <davidcai1993@yahoo.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Use a regex to validate the error message. PR-URL: nodejs#12879 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: David Cai <davidcai1993@yahoo.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

test: update test for url.parse() to match the exact error through re…

411cf56

…gexp check

nodejs-github-bot added the test Issues and PRs related to the tests. label May 7, 2017

addaleax reviewed May 7, 2017

View reviewed changes

test: allign the function and regexp vertically to keep lines short

5b86bd5

addaleax approved these changes May 7, 2017

View reviewed changes

lpinca approved these changes May 7, 2017

View reviewed changes

hiroppy approved these changes May 7, 2017

View reviewed changes

DavidCai1111 approved these changes May 7, 2017

View reviewed changes

mscdex added the url Issues and PRs related to the legacy built-in url module. label May 7, 2017

TimothyGu approved these changes May 7, 2017

View reviewed changes

thefourtheye reviewed May 8, 2017

View reviewed changes

watilde approved these changes May 8, 2017

View reviewed changes

jasnell reviewed May 9, 2017

View reviewed changes

test: add function and symbol validation for url.parse()

ef07ba4

jasnell approved these changes May 12, 2017

View reviewed changes

lpinca closed this May 16, 2017

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jun 22, 2017

@@ @@ -13,8 +13,9 @@ const url = require('url'); @@
 ,
                 [],
                 {}

Uh oh!

Conversation

andreicioromila commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented May 7, 2017

Uh oh!

vsemozhetbyt commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavidCai1111 commented May 7, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 10, 2017

Uh oh!

andreicioromila commented May 12, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented May 12, 2017

Uh oh!

andreicioromila commented May 16, 2017

Uh oh!

lpinca commented May 16, 2017

Uh oh!

lpinca commented May 16, 2017

Uh oh!

lpinca commented May 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

andreicioromila commented May 7, 2017 •

edited

Loading

vsemozhetbyt commented May 7, 2017 •

edited

Loading