Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assertEquals conflicts with the JavaScript's meaning of "equality" #3513

Closed
Tracked by #4717
rotu opened this issue Aug 3, 2023 · 5 comments
Closed
Tracked by #4717

assertEquals conflicts with the JavaScript's meaning of "equality" #3513

rotu opened this issue Aug 3, 2023 · 5 comments
Labels
bug Something isn't working needs triage

Comments

@rotu
Copy link
Contributor

rotu commented Aug 3, 2023

Describe the bug

assertEquals incorrectly treats NaN which as equal to itself.

For reference, JavaScript has 3 similar operations for equivalence:

Steps to Reproduce

import * as asserts from 'https://deno.land/std/assert/mod.ts'

asserts.assertEquals(NaN, NaN)
asserts.assertStrictEquals(NaN, NaN)
asserts.assertAlmostEquals(NaN, NaN)

Expected behavior

As written, all three of the above assertions should fail. If the current NaN behavior is intended, it should be documented and/or renamed to something like assertAlike or assertSameValue which doesn't conflict with Javascript's definitions of "equal".

Environment

  • OS: MacOS
  • deno version: 1.35.3
  • std version: 0.196.0
@rotu rotu added bug Something isn't working needs triage labels Aug 3, 2023
@kt3k
Copy link
Member

kt3k commented Aug 3, 2023

In Node assert.equal and assert.strictEqual don't throw with the below:

assert.equal(NaN, NaN)
assert.strictEqual(NaN, NaN)

Also in Jest, the below doesn't throw:

expect(NaN).toEqual(NaN);
expect(NaN).toBe(NaN);

I think assertion libs treating NaN equals itself is standard in JS ecosystem

@rotu
Copy link
Contributor Author

rotu commented Aug 3, 2023

@kt3k good call in comparing to other frameworks. It's a bit of a rabbit hole.

Here's what I found:

Node

assert.equal and assert.strictEqual both treat NaN as equal to itself.

  1. assert.strictEqual uses Object.is.
  2. assert.equal uses the == operator, with a special NaN handling to match assert.strictEqual.
  3. The equality operators for objects are by-reference (the recursive value semantics are under a function deepStrictEqual)

Jest

The documentation is rather sparse, but both toEqual and toStrictEqual treat NaN as equal to itself.

  1. toEqual checks value equality https://jestjs.io/docs/expect#toequalvalue
  2. toStrictEqual checks value equality, but stricter https://jestjs.io/docs/expect#tostrictequalvalue
  3. The equality operators for objects are by-value. toBe checks reference equality.

Chai

The equal method treats NaN as NOT equal to itself. the eql treats NaN as equal to itself.

  1. equal compares with ===
  2. eql uses a deep equality operator which uses Object.is for primitive comparisons.

@lino-levan
Copy link
Contributor

My gut feeling is that if someone cares about NaN semantics in JS, they could just do

assert(maybeNaN() === anotherMaybeNaN());

Most developers do not know or care about Object.is vs ===. assertEqual already does everything by value, so I think this is consistent within this model.

I feel like this is more of an issue of documentation than of behavior. We should make sure that developers that DO care know that assertEquals does equality by value in case of NaN.

@rotu
Copy link
Contributor Author

rotu commented Aug 7, 2023

Most developers do not know or care about Object.is vs ===. assertEqual already does everything by value, so I think this is consistent within this model.

Unfortunately, I don't think that's the case. NaN is a pretty common object to compare especially where you might want assertions.

I do think Object.is is the most sane comparison that most people will need most of the time. And that the non-reflexive semantics of IEEE 754's compareQuietEqual do not deserved to be called "equal" nor reflected in a language's most handy equality operator.

I feel like this is more of an issue of documentation than of behavior. We should make sure that developers that DO care know that assertEquals does equality by value in case of NaN.

It's documentation AND naming. I think JavaScript as a language is at fault here. It screwed up with "equality" twice, and now the word is broken.

For some reason, assertEqual doesn't even use "same value" for numbers, which is IMO the wrong choice.


I'd maybe rename:

  • assertStrictEquals -> assertIs
  • assertEquals -> assertAlike.

@rotu rotu changed the title assertEquals(NaN, NaN) doesn't throw assertEquals conflicts with the JavaScript's meaning of "equality" Aug 7, 2023
@kt3k
Copy link
Member

kt3k commented May 30, 2024

Closing due to the inactivity.

@kt3k kt3k closed this as not planned Won't fix, can't repro, duplicate, stale May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage
Projects
None yet
Development

No branches or pull requests

3 participants