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.

Sign up for GitHub

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

Jump to bottom

test: add regex #11193

Closed
wants to merge 8 commits into from
Closed

test: add regex #11193

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3acabec
test: add regex
Feb 1, 2017
8870c7f
test: verify error message thrown from assert.ifError
Feb 6, 2017
ac29f57
test: make regex the second argument of assert.throws
Feb 6, 2017
691ce1c
test: add space before curly brace
Feb 6, 2017
25527ff
test: fix exceed maximum lenghth lint error
Feb 7, 2017
a7a57e3
test: fix exceed maximum line length lint error
Feb 7, 2017
ccab058
test: fix indentation lint error
Feb 8, 2017
48e3497
Merge branch 'fix-test' of github.com:jobala/node into fix-test
Feb 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/parallel/test-assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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+$/});
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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$/

Copy link
Author

Choose a reason for hiding this comment

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

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

assert.doesNotThrow(function() { assert.ifError(null); });
assert.doesNotThrow(function() { assert.ifError(); });

Expand Down