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

assert: handle class constructors in throws() #4168

Closed
wants to merge 1 commit into from
Closed

assert: handle class constructors in throws() #4168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,13 @@ function expectedException(actual, expected) {
// Ignore. The instanceof check doesn't work for arrow functions.
}

return expected.call({}, actual) === true;
try {
return expected.call({}, actual) === true;
} catch (e) {
// Ignore. If expected is a class constructor, an error will be thrown
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that assert.throws() will now ignore all errors thrown by the validation function? (And are there semver implications to such a change? Wouldn't we want things like ReferenceError to bubble up to the user?)

Copy link
Member

Choose a reason for hiding this comment

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

I agree, this seems dangerous. You could rethrow unless e instanceof TypeError && /Class constructors cannot be invoked without 'new'/.test(e.message) but that changes the throw site and can still swallow genuine TypeErrors from the expected callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are definitely semver implications. I don't have strong feelings about this PR. To be honest, I'd rather close it than special case and rethrow errors.

Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, and likely not telling anyone anything they don't already know, the "proper" fix is probably something like:

  • Unlock API
  • Change function signature to add an options object and the user can send a validation option and/or a constructor option for an object that the thrown error must match
  • Relock the API

That could be done in a semver-minor way if we leave the legacy behavior intact, bug and all.

I'm not necessarily recommending that route--the API was locked for a reason--but just saying: If we want to fix it "right", the solution probably looks something like that...


return false;
}

function _throws(shouldThrow, block, expected, message) {
Expand Down
16 changes: 16 additions & 0 deletions test/parallel/test-assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,22 @@ try {
}
assert.ok(threw, 'wrong constructor validation');

// Validate that a class constructor works with throws()
threw = false;

try {
const ClassError = class {};

assert.throws(() => {
throw new RangeError();
}, ClassError);
} catch (e) {
assert(e instanceof RangeError);
threw = true;
}

assert.ok(threw, 'wrong constructor validation');

// use a RegExp to validate error message
a.throws(makeBlock(thrower, TypeError), /test/);

Expand Down