jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.25k stars 6.46k forks source link

[Bug]: 0 is not considered equal to -0? #12221

Open Pomax opened 2 years ago

Pomax commented 2 years ago

Version

27.4.7

Steps to reproduce

it(`these are the same. No really `, () => {
  expect(0 === -0).toBe(true); 
  expect(0).toBe(-0);
})

First test passes, because it should. Second test fails. It very much should not.

Expected behavior

By both maths and JS spec definition, 0 is equal to -0

Actual behavior

    Expected: -0
    Received: 0

       7 |   it(`these are the same. No really `, () => {
       8 |     expect(0 === -0).toBe(true);
    >  9 |     expect(0).toBe(-0);
         |               ^
      10 |   })

Additional context

Floating point numbers result in hilarious edge cases.

Environment

Win 10 Pro x64, Node v17.3.0, Jest v27.4.7
SimenB commented 2 years ago

This is by design, we use Object.is. That said, I might be convinced that toEqual should report them as the same, but we have an explicit test today that they're not: https://github.com/facebook/jest/blob/5cd75f4e0b9f8b678fedee268676864013b12d81/packages/expect/src/__tests__/matchers.test.js#L452

@thymikee @cpojer thoughts?

Pomax commented 2 years ago

Given that I'm running into this as part of a maths library tests, 0 and -0 kind of need to be the same thing. The direction by which we arrive at zero is not part of the numerical equality. It's why 0 === -0 is true as per the JS language spec (ref: https://262.ecma-international.org/6.0/#sec-strict-equality-comparison, 7.2.13 step 4, points d and e) even if in the IEEE 754 spec the values +0 and -0 are provisioned as different bit patterns.

(Jest is a test framework for JS, after all; not the IEEE =)

Using Object.is in this case gives you the wrong result because while they're different internal values to JS (because otherwise it wouldn't be able to show 0 vs. -0) the JS spec says they're the same value in actual code. As such, treating them as not equal is quite literally a spec violation.

Of course, if a toStrictEqual or the like goes "hey these are different IEEE values" that makes a lot of sense, and even a maths library might benefit from that (e.g. to determine limiting behaviour) but the plain equals/toBe should most definitely say "yes, these are the same value".

mrazauskas commented 2 years ago

What about a custom .toBeMathematicallyEqual matcher? If .toBe matcher does not suite your project, why not to create a custom one?

Just an idea. I am not tying to argue (;

danny-does-stuff commented 2 years ago

@Pomax What you're saying does seem to make sense.. Are you implying then that the implementation of Object.is is incorrect? Or just saying that Object.is is not the correct function to use for the .toBe matcher?

Pomax commented 2 years ago

Good clarification question: using Object.is is in this case incorrect. For primitives, JS's strict equality (===) should be the authority on what is, and is not, "the same thing". (Except for values that the spec clearly indicates as never yielding true under === comparison, like NaN, Infinity, etc). Object.is is a low level mechanism for checking whether things are effectively pointers to the same data in memory (in this case, the standard library value table), which is too low level for testing user code (rather than testing whether an implementation of the JS interpreter itself is doing the right thing).

I haven't dug into the code to see how many things rely on Object.is but I would say that toBe should be based on ===, since it's for use with primitives, with equals based on arbitrarily deep object comparison. The Object.is comparison should only be used as part of the code for toStrictEqual.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

Pomax commented 2 years ago

This issue should probably not go stale.

harshbarger commented 2 years ago

I have no problem keeping the Object.is behavior with its 0 vs -0 inequality fortoBe. Since 5/-0 is -Infinity while 5/0 is Infinity, the difference has implications that should be respected.

That being said, I think there very much needs to be some option to use === in toEqual, whether an additional parameter or an additional version of the matcher. Right now I'm testing a function that creates an array of numbers in which 0 and -0 need to be treated as equal, and the function could produce either depending on circumstances.

It's really pretty awkward to have to figure out case by case whether I need to expect, for example, .toEqual([-3, -2, -1, 0, 1, 2]) or .toEqual([-3, -2, -1, -0, 1, 2]) when the two are functionally equivalent for my use. And a subtle change to the function or inputs could start breaking tests that would be perfectly robust if I could just allow 0 and -0 to be compared with ===.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

Pomax commented 2 years ago

obligatory "it would be nice to solve this" comment to prevent the stalebot from sniping this issue.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

Pomax commented 2 years ago

And again.

danielfarah54 commented 2 years ago

Any updates on this?

brunolnetto commented 1 year ago

Have you tried expect(0).toBeCloseTo(-0);? It may work.

Pomax commented 1 year ago

It may, but "very close to zero" and "zero" are wildly different things.

Also as a remark on an earlier comment that asserted that 5/0 yields Infinity and 5/-0 yields -Infinity: while true, the language itself specifies that 0 is strictly equal to -0 and running 0 === -0 (not just 0 == -0) must evaluate to true, or you're not implementing JS (or JS testing).

So: if you need to know whether the effect of a statement involving a possibly negative zero is some specific value, you run the statement and test the result. That's not an argument for what tests on zero itself do.

In this case using Object.is does something so close to what it's being used for that it feels like it does the thing it's being used for, but it's not. It checks whether two things point to the same items on the heap. The problem with that is that Number is special, and there are different bit patterns, with different heap values, that by spec-definition must be considered the same value in isolation. Zero and negative zero are the same value, even if statements involving them yield different results depending on that sign.

brunolnetto commented 1 year ago

A solution may be:

const isZero = (result) => {
    if(typeof result !== 'number') {
        throw NaNError('Provided argument is not a number. A possible zero result is a number!);
} else return Number(result) === 0;

}

Jest offers matcher extension. It seems very incircunstancial, although expected to twist nose in such situation. Why is not expect(expectation === result).toBe(true) a solution? :-/

Does it make sense?

t1anchen commented 9 months ago

Is it possible to make it optional toggle and allowed user to set it? Something could be like JEST_NEGATIVE_ZERO_COMPARISON_ENABLED=1 (or disable-ish similar).

wchargin commented 4 months ago

+1 especially in the case of expect(xs).toEqual(ys), where xs and ys are arrays or other deep structures, so the structural matching behavior of toEqual is still necessary but we still want standard equality semantics on -0. We're currently working around this with expect(xs.every((_, i) => xs[i] === ys[i])).toBe(true), but that has worse error messages and does not scale easily to more complicated structures. I agree that it makes sense for toBe to distinguish between the two, but it's surprising and unhelpful for toEqual to do so.

aayla-secura commented 1 month ago

It's kind of crazy that jest doesn't have a sane way to compare 0 and -0 in the same way that JavaScript internally can. I totally agree there needs to be a matcher that can consider 0 and -0 to eb the same.

In the mean time, one workaround is to use

expect(myNegZeroVal + 0).toBe(0);

This works because -0 + 0 gives 0.