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

Fix asymmetric equal for Number #7948

Merged
merged 7 commits into from Feb 25, 2019
Merged

Fix asymmetric equal for Number #7948

merged 7 commits into from Feb 25, 2019

Conversation

dubzzz
Copy link
Contributor

@dubzzz dubzzz commented Feb 21, 2019

Summary

Fixes #7941: toStrictEqual fails to distinguish 0 from 5e-324

The real problem is that the equal operation defined into jasmineUtils is not symmetric.

Test plan

Before that change, the following assertion was failing:

expect(0).not.toStrictEqual(5e-324);

Now it passes as expected.

@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 21, 2019

I have not added tests as I did not know where to add them. Today the jasmineUtils file does not seem to be covered by direct tests - it is indirectly covered by tests on toStrictEqual and other expectations offered by Jest.

@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 21, 2019

I would be interested into having your opinion on #7938 (comment). It suggests to add automatic test suites to detect such bugs more easily in the future.

@pedrottimark
Copy link
Contributor

If a recent commit broke CI, then you might need to merge the fix from master branch tomorrow.

To answer your question about tests, I just added two cases after the first 3 in the packages/expect/src/__tests__/matchers.test.js file:

describe('.toEqual()', () => {
  [
    [true, false],
    [1, 2],
    [0, -0],
    [0, Number.MIN_VALUE], // issues/7941
    [Number.MIN_VALUE, 0],
    // and so on

Look down in the test file to see that the tuples consist of [received, expected]

For me, the first new case throws because of the error and the second writes a new snapshot.

On your branch, you can expect two new snapshots in packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap file.

@pedrottimark
Copy link
Contributor

Considering the potential untested regression that you saved us (the we who is me :) from, here are 3 suggested test cases following the first 2 existing cases farther down:

  [
    [true, true],
    [1, 1],
    [NaN, NaN],
    [0, new Number(0)],
    [new Number(0), 0],
    // and so on

The add 3 new snapshots from the expected-to-fail .not.toEqual assertions.

@pedrottimark
Copy link
Contributor

Off topic of this pull request, serialization in this report confused me, even if I should have known:

Expected: <green>0</>
Received: <red>{}</>

Good thing that this edge case might only even happen in Jest test suite:

expect(new Number(0)).toEqual(0);

Copy link
Contributor

@pedrottimark pedrottimark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Sometimes how you don’t fix it (and why not) is as important as how you do fix it :)

@SimenB
Copy link
Member

SimenB commented Feb 22, 2019

I didn't know you could call Number with new... Should toEqual handle that and unbox the value? Calling .valueOf() seems to do the trick

@SimenB
Copy link
Member

SimenB commented Feb 22, 2019

Looking at the MDN, this seems to be a case that toEqual should handle that toBe would fail on

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number#Syntax

Probably off topic

@pedrottimark
Copy link
Contributor

pedrottimark commented Feb 22, 2019

YDKJS :) Nicolas reminded me new Number instantiates object but Number alone is type cast:

var a = new Number('123'); // a === 123 is false
var b = Number('123'); // b === 123 is true
a instanceof Number; // is true
b instanceof Number; // is false

Yes, this is an off-the-edge case in which deep equality differs from referential equality:

describe('toBe', () => {
  test('2 instances', () => {
    expect(new Number(0)).not.toBe(new Number(0));
  });
  test('instance and primitive', () => {
    expect(new Number(0)).not.toBe(0);
  });
});

EDIT: so that is reason for return Object.is(Number(a), Number(b)); in code called by toEqual

@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 24, 2019

I do not reproduce the AppVeyor build failed locally. I get other issues - even on master - but not the one reported by AppVeyor.

I'll rebase the PR to keep on track with master

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@5d48312). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #7948   +/-   ##
========================================
  Coverage          ?   64.8%           
========================================
  Files             ?     256           
  Lines             ?   10047           
  Branches          ?    1464           
========================================
  Hits              ?    6511           
  Misses            ?    3218           
  Partials          ?     318
Impacted Files Coverage Δ
packages/expect/src/jasmineUtils.ts 76.03% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d48312...e9a1873. Read the comment docs.

@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 25, 2019

Is there something blocking the merge of this PR?

@SimenB
Copy link
Member

SimenB commented Feb 25, 2019

@pedrottimark feel free to merge these PRs to expect (unless you really want a second/third pair of eyes - and if so please say so) - you know the package best 🙂

// `NaN`s are equivalent, but non-reflexive. An `egal` comparison is performed for
// other numeric values.
return a != +a ? b != +b : a === 0 ? 1 / a == 1 / b : a == +b;
return Object.is(Number(a), Number(b));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

such a beautiful fix 😀

@pedrottimark pedrottimark merged commit bcaa146 into jestjs:master Feb 25, 2019
@pedrottimark
Copy link
Contributor

@dubzzz Thank you for your contribution and your patience.

@SimenB Yes, my friend. I need to pull my weight :)

@SimenB
Copy link
Member

SimenB commented Feb 25, 2019

I need to pull my weight :)

No danger of not doing that 😛

@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 25, 2019

@pedrottimark No worry ;) Thanks a lot for the quick reviews. You gave me really useful hints concerning where the bug comes from so that I was able to fix it

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

toStrictEqual fails to distinguish 0 from 5e-324
6 participants