Error or not updated docs on assert.isAbove and isBelow

[v1adko]

Hi.

assert.isAbove and assert.isBelow used to take Dates as possible inputs and worked fine, but since v4.0.0 it errors with expected *date* to be a number resulting in numerous failed test-cases throughout my codebase after an update from 3.5.

So my guess is either the documentation was not updated (as it currently lists {Mixed} type for an input) or it's a bug - either way there seems to be a problem. Just wanted to ask which one is it.

It's reproducible both in Mocha (Node) and in the browser (dev console on the official website with 4.0.0-canary.2 at the moment of this issue)

Is there is indeed a problem or have I misunderstood something?
And how can I help?

[meeber]

Hi @v1adko, thanks for the issue. Per #691 and #692, these assertions were changed to only accept numbers, but only the bdd documentation got updated. However, I think it's reasonable that these assertions should also accept Date objects.

One thing I'm not sure about is if the assertion should allow a number to be compared to a Date object. While technically valid (since the Date object will be converted to number of milliseconds since the Unix epoch), these kinds of silent type conversions scare me, especially in the context of an assertion library. I'm inclined to say that the types of the target and the passed value must match.

An alternative would be to keep the current behavior of above and below to only accept numbers, but then add new assertions after and before that only accept dates. Or to go with the first solution but then add after and before as aliases.

[keithamus]

I agree with matching values. I think we should ensure something like assert.isBefore(new Date('2017-01-01'), new Date('2017-02-01')) works, but that assert.isBefore(new Date('2017-01-01'), 1485907200000) should fail due to the type mismatch.

[v1adko]

I agree with @keithamus. I think there should be a type comparison and Number and Date should not be mixed.
@meeber, another possible approach could be separate assert functions like isBeforeDate/isEarlier, to keep it concise and specific, even self-explanatory. But as a downside it's gonna bloat the assert API and I'm not sure what's the policy and opinions are on that.

[keithamus]

To further the argument for expanding isBefore/isAfter; I think polymorphism is a fine trait even for testing libraries. Type coercion is problematic, especially for testing frameworks (=== is a best practice, right?). I hope these two sentences are something we can all agree on. I would also agree to @v1adko's point about API bloat.

[meeber]

@keithamus Agreed but important to note that the current assertions are isAbove and isBelow. The isBefore and isAfter assertions were suggested alternatives if polymorphism was undesirable in this case. Or they could just be added as aliases. But that's discussion for another issue. Sounds like we agree that the existing isAbove and isBelow should be expanded to support Date objects as long as both values are Dates (and continue supporting numbers as long as both values are numbers). This will involve small update of both bdd and assert docs in addition to code change.

[keithamus]

Oops, yes, I meant to say we could expand isAbove and isBelow.