-
Notifications
You must be signed in to change notification settings - Fork 35
Should Expect.equal
fail on floats?
#230
Comments
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 In other words, (And assuming that |
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. |
I assume this was supposed to say |
I agree, and I also consider it a positive to spread awareness of the pitfalls of comparing floats for equality in general! |
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 :) |
In 0.19 we should be getting However, I don't think this should be blocked on that!
Love it! 💯 |
Sounds like a plan for the next major release. |
We have
Expect.within
for comparing floats, but people don't necessarily know it's there. They may useExpect.equal
on floats, not realizing that they should useExpect.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 toExpect.within
instead.Here's how we could implement it:
Expect.equal
start by callingtoString
on both argumentsString.toFloat
truncate
both floats and compare them with==
. If the truncation changed either of them, they were non-integers and should have been compared withExpect.within
.Thoughts?
The text was updated successfully, but these errors were encountered: