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

addAssertion-weirdness #339

Closed
msiebuhr opened this issue Sep 29, 2016 · 11 comments
Closed

addAssertion-weirdness #339

msiebuhr opened this issue Sep 29, 2016 · 11 comments

Comments

@msiebuhr
Copy link
Contributor

msiebuhr commented Sep 29, 2016

All calls to addAssertion() is on the form of <type> [not] to ...:

expect.addAssertion('<any> to be one of', ...)

And running it gives:

  'log-level':
    'trace', // expected 'trace' to be one of [ 'info', 'trace' ]
             //   No matching assertion, did you mean:
             //   <any> [not] to be one of

Except when using the any-type; then it's addAssertion('[not] to ...'), which took me a while to figure out, given the error seem to tell me I'm doing things right...

@msiebuhr
Copy link
Contributor Author

msiebuhr commented Sep 29, 2016

(Also marginally related to #255 - set-like things are really nice when doing integration-style testing)

@sunesimonsen
Copy link
Member

Yes that can definitely be improved.
On tor. 29. sep. 2016 at 11.34, Morten Siebuhr [email protected]
wrote:

(Also marginally related to #255
#255)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#339 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFisrStRP8i9BGZ8diMOte3HVSfybXlks5qu4YqgaJpZM4KJxic
.

@sunesimonsen
Copy link
Member

sunesimonsen commented Nov 24, 2016

It might be helpful to show the signature of what you entered when there is a type miss match.

expected 'trace' to be one of [ 'info', 'trace' ]
  The assertion does not have a matching signature for:
     <string> to be one of <array>
  did you mean:
     <any> [not] to be one of

@papandreou
Copy link
Member

Implemented @sunesimonsen's suggestion here: #364

@msiebuhr, do you think the problem you originally reported gets solved by this?

@msiebuhr
Copy link
Contributor Author

Not really. The issue is that addAssertion() has a special fight-club syntax when defining assertions on <any> (namely, not to mention <any>).

expect('<string> to contain,... ');
expect('to contain', ...);          // = `<any> to contain ...`

Adhering to the element of least surprise, I should be able to:

expect('<any> to contain', ...);

@sunesimonsen
Copy link
Member

We could chose to write out a warning if you use that syntax and remove it later.

@papandreou
Copy link
Member

papandreou commented Jan 24, 2017

@msiebuhr,

Not really. The issue is that addAssertion() has a special fight-club syntax when defining assertions
on <any> (namely, not to mention <any>).

expect('<string> to contain,... ');
expect('to contain', ...);          // = `<any> to contain ...`

Oh, now I understand :)

Yeah, omitting the subject type and value types is allowed for compatibility with Unexpected 9 and below. It's essentially (but not 100%) an alias for expect.addAssertion('<any> to contain <any*>') -- indeed a bit of an oddball case, so I agree that we should deprecate it soon, probably for v11 (@sunesimonsen?).

Adhering to the element of least surprise, I should be able to:

expect('<any> to contain', ...);

That does work, doesn't it? It should declare an assertion that doesn't take any parameters (except the subject):

expect.addAssertion('<any> to foo', function (expect, subject) {
    expect(String(subject), 'to equal', 'foo')
});
expect('foo', 'to foo');
expect({toString: () => 'foo'}, 'to foo');

What doesn't work is omitting the types of the parameters when omitting the type of the subject, like this:

expect.addAssertion('to foo <number>', ...);
// Doesn't work, you have to use the full Unexpected 10 syntax and include the subject type as well.
// Throws: SyntaxError: Missing subject type in to foo <number>

// Doesn't do what you'd expect, since specifying a subject type but no parameter types
// means that the assertion doesn't take any parameters:
expect.addAssertion('<any> to do something with parameters', (expect, subject, param1) => {
    expect(param1, ...);
});

expect('foo', 'to do something with parameters', 123);
// Throws:
// expected 'foo' to do something with parameters 123
//   The assertion does not have a matching signature for:
//     <string> to do something with parameters <number>
//   did you mean:
//     <any> to do something with parameters

In the latter case the type system complains because to do something with parameters is only defined for the case where there's nothing following the assertion string.

I'm guessing that's what you've run into because expect('<any> to contain', ...); wouldn't make sense without parameters? Have you tried:

expect('<any> to contain <any+>', (expect, subject, param1, param2...) => {
    // ...
});

@papandreou
Copy link
Member

Adhering to the element of least surprise, I should be able to:

expect('<any> to contain', ...);

That does work, doesn't it? It should declare an assertion that doesn't take any parameters (except the subject):

Hmm, maybe we should warn about this case when addAssertion receives a function with an arity greater than 2 + the number of argument types? Then someone has fallen into the trap:

expect.addAssertion('<any> to foo', (expect, subject, value) => {
    // ...
});
// Error: The provided function takes more params than are defined by the type signature

@sunesimonsen
Copy link
Member

@papandreou that sounds like a nice idea, as long as we are only warning about more than the defined number of params.

@papandreou
Copy link
Member

Yeah, warning for less would be too daring, especially considering how ...rest params affect the length property:

> ((...rest) => {}).length
0
> ((a, ...rest) => {}).length
1

@papandreou
Copy link
Member

Implemented the idea here: #372

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

No branches or pull requests

3 participants