Checking that argument given to expect is of the right type when using with include

[astorije]

Relates to #501.

[astorije]

Hi @keithamus,

I have a question related to this PR. I know the following is possible:

expect(10).to.be.above(5).and.below(15);

(I know that within exists but that's for the sake of the example)

but is there a way we could make use of or possible? Something like:

expect(10).to.be.above(5).or.below(0);

There would be other nice uses of or, such as the one I am actually seeking:

expect(myVar).to.be.a('string').or.an('array');

For that latter example, a/an could be modified to accept multiple inputs:

expect(myVar).to.be.a('string', 'array');

but that would be nice to have a nice or operator. Any thoughts?

[astorije]

@keithamus, although my previous question about or still holds for Chai's API, I went ahead and added support of multiple types in cba291e (the object's type must be one of those supplied). That should make a lot of sense when the PR is complete, plus it adds a nice feature to the BDD API.
Note that I'll extend assert.typeOf to have the same support for consistency, but in future PR as my focus is the original issue and I needed this for my PR.

[astorije]

Voilà @keithamus, I am done. A couple of things though:

• Assuming you are testing a function that returns null although you weren't expecting so:

  expect(myMysteriousFunction()).to.include(someValue);

  You would get the following message:

  AssertionError: expected null to be an array, an object, or a string
  

  I'm afraid it's not clear enough, no? If not, I'll wrap the statement in a this.expectedTypes(...) method which sets the custom message and I'll add the function to the utilities.

• With my additions on the a/an assertion, we can easily add this check to all assertions. How can we nicely and uniformly add that info to their docstrings?

Let me know if you have any comments :-)

[astorije]

@keithamus... friendly ping? :)

[keithamus]

Hi sorry @astorije this was another that slipped through the next.

I'm not sold on passing an array of types to an. I'd prefer to see an or but the problem is with deeply chained assertions an or operator becomes confusing as to where it is switching, e.g.

expect({ foo: { bar: 1 }}).to.have.deep.property('foo.bar').and.be.a('number').and.equal(1).or.equal(2);

Which of the following does the above assertion do?

 obj.foo.bar && typeof obj.foo.bar === 'number && (foo.bar === 1 || foo.bar === 2)

 obj.foo.bar && (typeof foo.bar === 'number' && foo.bar === 1) || foo.bar === 2

(obj.foo.bar && typeof foo.bar === 'number' && foo.bar === 1) || obj === 2)

We could potentially have something like oneOf which could have a set of assertions passed to it (expect('foo').to.be.oneOf(expect.to.be.a('number'), expect.to.be.a('boolean'))) - but obviously our assertions are imperative and so we'd need a fundamental redesign of how they work - or introduce matchers (which, to be honest, is something I've been thinking about anyway).

Case in point is, while #501 is definitely something I'd like to see Chai fix, I feel like this PR is going in the wrong direction for it. Make sense? Thoughts?

[astorije]

Hi sorry @astorije this was another that slipped through the next.

Hey @keithamus, no worries :)

I'd prefer to see an or but the problem is with deeply chained assertions an or operator becomes confusing as to where it is switching, e.g.

That is indeed the issue with an or operator, so I do not think this would be suitable as is.

We could potentially have something like oneOf which could have a set of assertions passed to it (expect('foo').to.be.oneOf(expect.to.be.a('number'), expect.to.be.a('boolean'))) - but obviously our assertions are imperative and so we'd need a fundamental redesign of how they work

I thought about a oneOf/either operator that could also be used like:

expect('foo').to.be.oneOf(function (either)
  either.a('number');
  either.a('boolean');
);

but considering how expect works around exceptions, I couldn't see a way of doing that easily without, indeed, redesigning Chai as its core, not a suitable solution either.

or introduce matchers (which, to be honest, is something I've been thinking about anyway).

What do you mean by that? What is a matcher?

Case in point is, while #501 is definitely something I'd like to see Chai fix, I feel like this PR is going in the wrong direction for it. Make sense? Thoughts?

I don't think this PR is going in the wrong direction (surprising? :D), but let me explain myself.

I'm not sold on passing an array of types to an.

I was not very happy with that at first as well, but a few things convinced me:

• It can actually be useful to test that a returned value is one of several types, and without this change, I do believe it is currently impossible to cleanly achieve using Chai (well, one can wrap multiple expect(...).to.be.a(...) in a try...catch and hack around that, but boy this is ugly and I sure hope to never find this in my tests!)

• The any.keys(key1, key2, ...) assertion already works similarly, as shown by the examples:

  expect({ foo: 1, bar: 2 }).to.have.any.keys('foo', 'baz');

I could add a third bullet point to say that I really need that feature for this check to work, but that wouldn't be a fair argument :-)

If you think this PR is going in the wrong direction, the best thing I can offer is to wrap this check in a helper as mentioned in my comment above. That way, whenever an alternative solution arises, we can just edit it there without breaking our API or plugins that would use that helper.
Furthermore, I can also move the whole one-of-types check from the a/an assertion to that helper if you really want to keep the assertion array-less, but my opinion is that it's a nice feature to expose to the API.

Overall, what I want is to be done with this PR. I think having a non-ideal solution to improve over time is better than having no solutions until we find the one true perfect solution. Again, with my proposals we can make this work without affecting the API (despite my comment on the first list item) if that's your main concern.

What do you think?

[keithamus]

What do you mean by that? What is a matcher?

Sorry, I should have clarified this. I mean matcher as in the same kind of device Sinon has, where they can be used as "generics" in combination with assertions. Done right I think it could give Chai a lot of additional power, but it needs a lot of thinking through and so it hasn't been on the short or medium term roadmap (per se).

The way I see a matcher is that it could be interspersed with other assertions, to enable things like: expect({ foo: 1, bar: { ... }}).to.deep.equal({ foo: 1, bar: chai.match.object }); or expect(myChaiSpy).to.have.been.calledWith(1, 2, chai.match.function.that.has.lengthOf(2));. In this would provide a generic interface to do these "rough" assertions, and could be used here; for example: expect(obj).to.match.oneOf(chai.match.number, chai.match.boolean).

This has been discussed a bit over in #324 and #97, but as I said nothing really firm yet.

I was not very happy with that at first as well, but a few things convinced me...

I agree with those points, but I think a good generic solution as described above could fix those - so Im hesitant to add functionality until we know its the right functionality to add.

Overall, what I want is to be done with this PR. I think having a non-ideal solution to improve over time is better than having no solutions until we find the one true perfect solution. Again, with my proposals we can make this work without affecting the API (despite my comment on the first list item) if that's your main concern.

I agree with this, but with the caveat that a public API is a difficult thing to change, and so "the non-ideal solution" thats put in front of people and people start to use becomes "the solution". If we're discussing the same thing - how .an is changing to take an array might not the be "ideal" solution - then I'd rather not have it, and have .includes have private behaviour which we can make public when we land on the right solution. Make sense?

[astorije]

OK, a few things here, and I'll try to be concise this time, it's usually my weakness :)

• Regarding the matchers, I have no experience with Sinon matchers and I'm not sure I see the whole concept and benefits at 1am, but what I see is that the discussions started in 2012 and I'm not sure there will be activity around it and/or something I can use before a few months (years?). I cannot rely on that for this existing PR or for What to expect when you're expecting #501.
• I think we misunderstood each other: whether it may or may not be the right tool for the job here, I do think having a/an accept an array (oh, just to be clearer, it's not breaking the existing a/an assertion, it's adding to it) is a really nice feature to have. I see where I would use it myself, and I'm sure others would too, same as keys(). Maybe it deserves its own PR, but I am in favor of extending the existing BDD API with this new feature.
• Now, about the "ideal" and "non-ideal" solutions: I do think having a utility function/helper, like util.expectedTypes() (name TDB), same as util.inspect() or util.flag() is the ideal solution. Chai's assertions could use it, as well as other plugins (mine would most likely!). It would provide a nice interface and a clear message/exception when the object tested against an assertion does not respect the accepted types. What I called "non ideal" (potentially non ideal actually) is how we would do the internal machinery. But even when this gets improved, that helper is still there.

So I do think having such helper, whether or not a/an gets augmented, would be a good thing. Or maybe that's just because I spent a lot of time thinking and working on that so I'm biased :)
Let me know if you like that helper function and I'll start working on that, to show you how it looks outside of the blueprints :) If you don't... I'm not sure there is a solution here.

(So much for trying to be concise!)

[astorije]

Hi @keithamus, friendly ping? I'd really like this one to get done... :-)

[keithamus]

Hey @astorije sorry for taking a while to reply.

Overloading the an assertion is a no go for me, within the scope of this PR - especially to get it wrapped up quickly. We can bikeshed this later, but for now if you move that functionality to something like util.expectedTypes as suggested then I'd be happy to merge 😄

[astorije]

Hi @keithamus, thanks for your comment, I am now done with changes.

I have moved things to a util called expectTypes. I am using infinitive tense to be consistent with all other utils. Of course, English is not my native language so I'll let you choose better wording to your liking.

Also, you will note that one must call the function with _.expectTypes(this, ['type1', ...]); instead of _.expectTypes(obj, ['type1', ...]);. There are several reasons to this, two of them being that it prevents users/plugins to misuse this helper (for example for general type checking instead of using a/an), and it also gives us more freedom if in the future we need something else from the object (like getting the negate flag, or any new future flags).

I have updated/added tests accordingly, and reverted the a/an assertion.

One last question: Do you think we should add support for single values (_.expectTypes(this, 'string');, instead of passing it an array)? I don't think we should necessarily, but it's up to you. If so, do we need to define an alias called expectType?

[astorije]

Oh, forgot to (re-)mention, but once this makes it into master, I will add the check to other assertions that need it (number for above, within, ...).

What would be nice would be to add a docstring for that. What do you think and suggest?

[astorije]

@keithamus meeeerge meeee :-)

[keithamus]

@astorije sorry was on holiday. All merged now, I'll cut a release soon 😄

[astorije]

No worries!
I'll try to include that bit to other assertions before you release though.
Do you think we should add the types supported in the docstring in any way?

[keithamus]

Hmmmm... yes I guess that'd be a good idea actually. Feel free to either raise a new PR or tuck it into your next one for the other assertions.