Should `Expect.equal` fail on floats?

[rtfeldman]

We have Expect.within for comparing floats, but people don't necessarily know it's there. They may use Expect.equal on floats, not realizing that they should use Expect.within instead.

It occurs to me that we could have Expect.equal fail the test when given two floats, with an error message pointing the caller to Expect.within instead.

Here's how we could implement it:

1. Have Expect.equal start by calling toString on both arguments
2. Convert them both to floats using String.toFloat
3. If that worked, truncate both floats and compare them with ==. If the truncation changed either of them, they were non-integers and should have been compared with Expect.within.

Thoughts?

[mgold]

If I have a test that's passing because my floating point arithmetic happens to work out perfectly, is it bad or wrong to use Expect.equal? Is it better to specify my tolerances now, or do so lazily if and when my test breaks because of a 1e-13 difference?

In other words, Expect.within is useful for making making tests pass when we want them to. But I don't think Expect.equal will make tests pass when we don't want them to. So what's wrong with it then?

(And assuming that Expect.within Expect.equal for floats is always a bad idea, this is a PATCH change that will break a lot of tests.)

[drathier]

I'm leaning slightly towards implementing this check. It's more likely that the dev has that part of the code base in their working memory right when they write the tests, than when they go in to do a "quick fix".

On the other hand, you might not have to fix it ever if it happens to work the first time, and you never need to change that part of the code base.

Maybe we could argue it's a smell? That the tests are bad, even if they work? Comparing floats for exact equality is a bad thing in 99.999% of cases, after all.

[rtfeldman]

(And assuming that Expect.within for floats is always a bad idea, this is a PATCH change that will break a lot of tests.)

I assume this was supposed to say Expect.equal - and I agree; I'd want to include this with the MAJOR release that removes Fuzz.andThen.

[rtfeldman]

It's more likely that the dev has that part of the code base in their working memory right when they write the tests, than when they go in to do a "quick fix".

I agree, and I also consider it a positive to spread awareness of the pitfalls of comparing floats for equality in general!

[drathier]

spread awareness of the pitfalls of comparing floats for equality

That's a really good reason! I'm all for this change in the next major bump! We should consider the performance hit as well, though.

Also, does this float/int check always work? Let's write a fuzz test for the is-this-a-float function :)

[rtfeldman]

We should consider the performance hit as well, though.

In 0.19 we should be getting Kernel access (for try / catch so we can translate exceptions into test failures, among other things) so we can speed this up with typeof and arithmetic then.

However, I don't think this should be blocked on that!

Also, does this float/int check always work? Let's write a fuzz test for the is-this-a-float function :)

Love it! 💯

[mgold]

Sounds like a plan for the next major release.