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

[helfer]

Expected behavior:
expect(null).to.be.within(0,1) should return false for any numbers a and b.

Actual behavior:
expect(null).to.be.within(0,1) returns true. Sam for other falsey values, I assume, and similar problem with true.

Proposed fix:
Within should check that the value is a number and reject if it isn't before comparing the range here.

[meeber]

Hi @helfer, thanks for the report!

As a workaround, I'd recommend the following form:

expect(null).to.be.a('number').within(0,1)

I would worry about the implications of rejecting non-numbers here. Your example returns true because null gets coerced into a number (specifically, +0) by the <= and >= operators, per the ECMAScript spec. If anyone out there has written tests that rely on this per-spec coercion, then backward compatibility will be broken. Which isn't necessarily a problem, if this is deemed to be a bug in the within logic.

But what about other types of coercion? These valid tests would start failing if only numbers were allowed through:

• expect("3").to.be.within(1,4)
• expect("b").to.be.within("a", "c")

So what we might actually be talking about here is allowing some types of per-spec coercions to occur, but not others. I dunno. It feels like a tough sell, but at the same time I agree it's disorienting for null to return true in the specific example you provided. However, I also think the already-available workaround is an expressive solution.

Thoughts?

[helfer]

@meeber Thanks, the workaround makes sense, I'll use that.

The main reason I was surprised by this behavior is that all the examples in the documentation use numbers, and do not suggest that within coerces other types. I know Javascript is quite loose when it comes to comparators, but I think most people would expect a testing library to be much less tolerant.

To keep backwards compatibility, it's probably best not to change the behavior, but the documentation should be updated to mention that this works with other types and that a('number') should be used if a number is expected.

[meeber]

Agreed. If we move forward with the current functionality, then the documentation should indicate so and include examples of coercion. Additionally, tests should be added demonstrating the coercion.

Going to leave this issue open for awhile for further feedback and possible pull-request-wanted for docs/tests update.

[keithamus]

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?

[helfer]

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

[meeber]

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.

[meeber]

Anyone interested in working the PR for this? @helfer? :D

[helfer]

@meeber Not sure I'm familiar enough with the conventions and internals of chai, but with the proper guidance I could take a stab at it.

[lucasfcosta]

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 😄

[meeber]

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!

[meeber]

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?

[keithamus]

@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.

[vieiralucas]

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?

[helfer]

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

[meeber]

@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! :D

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]

@meeber hahaha
@lucasfcosta lets take over the world!

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

[meeber]

@vieiralucas Correct!

[meeber]

Leaving this open as a reminder that @vieiralucas's first PR resolved half of this issue, and that he'll submit a PR to resolve the other half after 4.0.0 lands.