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

Document that expected must be a RegExp #101

Merged
1 commit merged into from
Aug 28, 2014
Merged

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Aug 27, 2014

You get silent false positives if you use e.g. TypeError instead of /TypeError/.

You get silent false positives if you use e.g. TypeError instead of /TypeError/.
ghost pushed a commit that referenced this pull request Aug 28, 2014
Document that expected must be a RegExp
@ghost ghost merged commit 593e7c9 into tape-testing:master Aug 28, 2014
@ghost
Copy link

ghost commented Aug 28, 2014

Should it also accept a string?

@domenic
Copy link
Collaborator Author

domenic commented Aug 28, 2014

I don't think so. Maybe it should accept constructors, but strings are not that great, especially given the message string adjacent to it in the parameter list.

@ljharb
Copy link
Collaborator

ljharb commented Sep 17, 2014

I'm currently using this by passing in TypeError, the constructor function. When I change it to use /^TypeError$/, I get a failure with expected: '/^TypeError$/', actual: { message: '…' }

Using /TypeError/ also passes, but that seems like it's abusing string coercion. Is this argument not expected to be the Error constructor function?

@domenic
Copy link
Collaborator Author

domenic commented Sep 17, 2014

Look at the source code. /^TypeError$/ of course fails because the message is nonempty.


From: Jordan Harbandmailto:notifications@github.com
Sent: ý2014-ý09-ý16 23:48
To: substack/tapemailto:tape@noreply.github.com
Cc: Domenic Denicolamailto:domenic@domenicdenicola.com
Subject: Re: [tape] Document that expected must be a RegExp (#101)

I'm currently using this by passing in TypeError, the constructor function. When I change it to use /^TypeError$/, I get a failure with expected: '/^TypeError$/', actual: { message: '…' }

Using /TypeError/ also passes, but that seems like it's abusing string coercion. Is this argument not expected to be the Error constructor function?


Reply to this email directly or view it on GitHubhttps://github.com//pull/101#issuecomment-55855323.

@ljharb
Copy link
Collaborator

ljharb commented Sep 17, 2014

So it seems like expected is totally unused unless it's a regex - that doesn't seem very useful to me :-/

@substack would you be willing to accept a PR that has an additional check for if expected is a function, and another if it's an Error object?

@domenic
Copy link
Collaborator Author

domenic commented Sep 17, 2014

Seems fine to me. Overloading is an anti pattern.

On Sep 17, 2014, at 9:31, "Jordan Harband" <notifications@github.mirror.nvdadr.commailto:notifications@github.com> wrote:

So it seems like expected is totally unused unless it's a regex - that doesn't seem very useful to me :-/

@substackhttps://github.com/substack would you be willing to accept a PR that has an additional check for if expected is a function, and another if it's an Error object?

Reply to this email directly or view it on GitHubhttps://github.com//pull/101#issuecomment-55921246.

@Raynos
Copy link
Collaborator

Raynos commented Sep 17, 2014

@ljharb just open a PR.

Making test.throws() have the same semantics as require('assert').throws() is always a welcome change.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants