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: add regex #11193

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
8 participants
@jobala
Contributor

jobala commented Feb 6, 2017

Description of task to be completed

Add a regular expression for assert.throws to validate the error message thrown.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@hiroppy hiroppy added the assert label Feb 6, 2017

@joyeecheung

joyeecheung requested changes Feb 6, 2017 edited

Can you change the commit message to be more informative, for example, test: verify error message from assert.ifError? Thanks!

Show outdated Hide outdated test/parallel/test-assert.js
@@ -378,7 +378,7 @@ try {
assert.strictEqual(true, threw,
'a.doesNotThrow is not catching type matching errors');
assert.throws(function() { assert.ifError(new Error('test error')); });
assert.throws(function() { assert.ifError(new Error('test error')), /^([A-Za-z])\w+$/});

This comment has been minimized.

@joyeecheung

joyeecheung Feb 6, 2017

Member

This regexp is probably too general. Since the error being thrown here is known, can you change that to /^Error: test error$/?

@joyeecheung

joyeecheung Feb 6, 2017

Member

This regexp is probably too general. Since the error being thrown here is known, can you change that to /^Error: test error$/?

This comment has been minimized.

@jobala

jobala Feb 6, 2017

Contributor

@joyeecheung I do not understand your request, can you please expound.

@jobala

jobala Feb 6, 2017

Contributor

@joyeecheung I do not understand your request, can you please expound.

This comment has been minimized.

@joyeecheung

joyeecheung Feb 6, 2017

Member

In this repo we try to make the validation of the error message to be as concrete as possible. For example:

assert.throws(
  () => {
    throw new Error('Wrong value');
  },
  /^Error: Wrong value$/ // Instead of something like /Wrong value/
);

There is an ongoing PR to update our guide on this: #11150

@joyeecheung

joyeecheung Feb 6, 2017

Member

In this repo we try to make the validation of the error message to be as concrete as possible. For example:

assert.throws(
  () => {
    throw new Error('Wrong value');
  },
  /^Error: Wrong value$/ // Instead of something like /Wrong value/
);

There is an ongoing PR to update our guide on this: #11150

This comment has been minimized.

@joyeecheung

joyeecheung Feb 6, 2017

Member

In this line, since according to the documentation of assert.ifError(value):

Throws value if value is truthy.

We are checking that the truthy new Error('test error') passed to assert.ifError() should be rethrown, hence the error message from the block should be /^Error: test error$/

@joyeecheung

joyeecheung Feb 6, 2017

Member

In this line, since according to the documentation of assert.ifError(value):

Throws value if value is truthy.

We are checking that the truthy new Error('test error') passed to assert.ifError() should be rethrown, hence the error message from the block should be /^Error: test error$/

This comment has been minimized.

@jobala

jobala Feb 6, 2017

Contributor

@joyeecheung thanks for taking your time, I now get your point.

@jobala

jobala Feb 6, 2017

Contributor

@joyeecheung thanks for taking your time, I now get your point.

Show outdated Hide outdated test/parallel/test-assert.js
@@ -378,7 +378,7 @@ try {
assert.strictEqual(true, threw,
'a.doesNotThrow is not catching type matching errors');
assert.throws(function() { assert.ifError(new Error('test error')); });
assert.throws(function() { assert.ifError(new Error('test error')), /^Error: test error/});

This comment has been minimized.

@cjihrig

cjihrig Feb 6, 2017

Contributor

I think the regular expression should come after the }. Also, please add a $ to the end of the regex.

@cjihrig

cjihrig Feb 6, 2017

Contributor

I think the regular expression should come after the }. Also, please add a $ to the end of the regex.

This comment has been minimized.

@jobala

jobala Feb 6, 2017

Contributor

@cjihrig thanks for pointing that out, I had missed the part that the regex is the optional argument to assert.throws()

@jobala

jobala Feb 6, 2017

Contributor

@cjihrig thanks for pointing that out, I had missed the part that the regex is the optional argument to assert.throws()

Show outdated Hide outdated test/parallel/test-assert.js
@@ -378,7 +378,7 @@ try {
assert.strictEqual(true, threw,
'a.doesNotThrow is not catching type matching errors');
assert.throws(function() { assert.ifError(new Error('test error')); });
assert.throws(function() { assert.ifError(new Error('test error'))}, /^Error: test error$/);

This comment has been minimized.

@cjihrig

cjihrig Feb 6, 2017

Contributor

You're going to need to add the semicolon back in.

@cjihrig

cjihrig Feb 6, 2017

Contributor

You're going to need to add the semicolon back in.

This comment has been minimized.

@cjihrig

cjihrig Feb 6, 2017

Contributor

Also, please add a space before the closing curly brace for symmetry.

@cjihrig

cjihrig Feb 6, 2017

Contributor

Also, please add a space before the closing curly brace for symmetry.

@cjihrig

cjihrig approved these changes Feb 6, 2017

@jasnell

jasnell approved these changes Feb 6, 2017

@hiroppy

hiroppy approved these changes Feb 7, 2017

@hiroppy

This comment has been minimized.

Show comment
Hide comment
@jobala

This comment has been minimized.

Show comment
Hide comment
@jobala

jobala Feb 7, 2017

Contributor

@abouthiroppy could you know why the tests are failing?

Contributor

jobala commented Feb 7, 2017

@abouthiroppy could you know why the tests are failing?

@hiroppy

This comment has been minimized.

Show comment
Hide comment
@hiroppy

hiroppy Feb 7, 2017

Member

Please fix this lint error.
381:1 error Line 381 exceeds the maximum line length of 80 max-len

Member

hiroppy commented Feb 7, 2017

Please fix this lint error.
381:1 error Line 381 exceeds the maximum line length of 80 max-len

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 7, 2017

Member

The long line has been fixed, but this is still returning a lint error due to indentation problems introduced in the fix for the log line.

@jobala Please run make jslint or vcbuild lint before pushing commits. (If you run make test or vcbuild test, the linting should occur as part of that already.)

Member

Trott commented Feb 7, 2017

The long line has been fixed, but this is still returning a lint error due to indentation problems introduced in the fix for the log line.

@jobala Please run make jslint or vcbuild lint before pushing commits. (If you run make test or vcbuild test, the linting should occur as part of that already.)

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Feb 8, 2017

Contributor

@Trott Did you mean make lint or vcbuild jslint? :D

Contributor

thefourtheye commented Feb 8, 2017

@Trott Did you mean make lint or vcbuild jslint? :D

@thefourtheye

LGTM if CI is green.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 8, 2017

Member

@Trott Did you mean make lint or vcbuild jslint? :D

Argh! Yeah, vcbuild jslint. Sorry.

Member

Trott commented Feb 8, 2017

@Trott Did you mean make lint or vcbuild jslint? :D

Argh! Yeah, vcbuild jslint. Sorry.

@jobala

This comment has been minimized.

Show comment
Hide comment
@jobala

jobala Feb 8, 2017

Contributor

@Trott thanks for the pointer.

Contributor

jobala commented Feb 8, 2017

@Trott thanks for the pointer.

@Trott

This comment has been minimized.

Show comment
Hide comment

jasnell added a commit that referenced this pull request Feb 11, 2017

test: improve test-assert.js
Verify error message thrown from assert.ifError

PR-URL: #11193
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 11, 2017

Member

landed in 5f20d62

Member

jasnell commented Feb 11, 2017

landed in 5f20d62

@jasnell jasnell closed this Feb 11, 2017

italoacasas added a commit that referenced this pull request Feb 13, 2017

test: improve test-assert.js
Verify error message thrown from assert.ifError

PR-URL: #11193
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: improve test-assert.js
Verify error message thrown from assert.ifError

PR-URL: nodejs#11193
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

test: improve test-assert.js
Verify error message thrown from assert.ifError

PR-URL: nodejs#11193
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

jasnell added a commit that referenced this pull request Mar 7, 2017

test: improve test-assert.js
Verify error message thrown from assert.ifError

PR-URL: #11193
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

Needs a backport PR to land in v4

Member

jasnell commented Mar 7, 2017

Needs a backport PR to land in v4

MylesBorins added a commit that referenced this pull request Mar 9, 2017

test: improve test-assert.js
Verify error message thrown from assert.ifError

PR-URL: #11193
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v6.10.1 proposal #11759

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment