chai: expect(null).to.be.within(0,10) does not fail assertion

@vieiralucas Absolutely. In fact, it’s on Chai’s roadmap for 7.x.x to claim all of the world’s Lucases as contributors. With your contribution, we’ll be up to two! 😄

If you have any questions, doesn’t hesitate to ask. Also, please keep the PR in scope of what was agreed upon earlier in the thread (i.e., numbers-only).

@vieiralucas It’s totally fine with me, and I don’ think anyone else would mind, so go ahead!

Hello guys, I’ve been following chai’s project for quite some time, and I think that I might be capable of making a pull request addressing this issue. I actually already started coding it 😄 So, is it ok for you if I submit a PR for this?

I think it’s reasonable for an assertion library to avoid coercion games and demand specific types.

I’d throw my support behind this as a breaking change for 4.0.0 assuming the following:

• Related assertions receive the same type detection treatment (some might already have this): above, least, below, most, closeTo, increase, decrease
• Documentation updated to indicate type requirements
• Tests added to prove type requirements

I also wonder if it’d be useful to have an assertion that behaved similarly to sort’s default compareFunction, using Unicode code points.

@vieiralucas Correct!

@meeber hahaha @lucasfcosta lets take over the world!

Just to make sure, I should submit this to the 4.x.x branch right?

@meeber while that would be a cool idea, I think its perhaps a draft for another issue. For now, let’s do type checking to make sure the expectation is a number, and we can extend to strings later on.

Was thinking about this earlier. One possibility would be to allow above, below, and within to accept both numbers as well as strings, as long as the actual and expected were consistent. And strings would be tested based on Unicode code points.

Examples:

expect(3).to.be.within(2, 4); // Pass
expect("b").to.be.within("a", "c"); // Pass
expect(3).to.be.within("a", "c"); // Type error
expect("b").to.be.within(2, 4); // Type error

Possible aliases:

expect("z").to.be.above("a"); // Original
expect("z").to.come.after("a"); // Alias?
expect("z").to.follow("a"); // Alias?

expect("a").to.be.below("z"); // Original
expect("a").to.come.before("z"); // Alias?
expect("a").to.precede("z"); // Alias?

expect("m").to.be.within("a", "z"); // Original
expect("m").to.come.between("a", "z"); // Alias?

It looks like the closeTo assertion already has a type check here

And it already has tests for the type check here, here, and here

Therefore, one approach is to use the existing code for closeTo as guidance for type-checking the other numeric assertions. Although we’re open to suggestions for improvements!

Hi @helfer, Basically, all of the assertion’s logic is here, at assertions.js, there you will be able to find all of the related assertions you will need to make changes into, as @meeber said. I also recommend that you take a look into assertion.js, because that’s the object we use to do assertions themselves, it includes messages, expected and actual values and the expression which should be checked.

Whenever you run into something from utils you can take a look at this file to check what that utility function does.

Basically you will have to run some checks for types inside the expressions @meeber said above, but you may tackle this the way you think it’s better.

If you need to know more about the contribution process we’ve got this file too.

Please let me know if you need anything, I would certainly be eager to help you contribute and earn a spot into our Hall of Fame 😄

@keithamus I think changing within to accept only numbers would be best, because that way the usage of within would be very clear.

We could definitely make within fail when given a non-number. I think we should even consider doing so, as a lot of our existing assertions are quite strict. But as @meeber suggests, to do so would be a breaking change. Luckily we have a 4.x.x branch which we can put this breaking change into - and release 4.0.0 with that breaking change. Thoughts @helfer and @meeber?