Immutable.Map comparison with not the same key order

[davidfou]

Do you want to request a feature or report a bug?
bug

What is the current behavior?
Comparison between to identical Immutable.Map() but declared without the same key order fails.

If the current behavior is a bug, please provide the steps to reproduce and either a repl.it demo through https://repl.it/languages/jest or a minimal repository on GitHub that we can yarn install and yarn test.

What is the expected behavior?
All the specs on this example should pass.

describe('comparison with Immitable.Map', () => {
  const map1 = Immutable.Map({ a: 1, b: 2 });
  const map2 = Immutable.Map({ b: 2, a: 1 });
  
  it('should ignore key order with Immutable.is', () => {
    expect(Immutable.is(map1, map2)).toBe(true);
  });
  
  it('should ignore key order with Jest', () => {
    expect(map1).toEqual(map2);
  });
});

[SimenB]

The behavior is consistent with normal JS Map.

describe('comparison with Map', () => {
  const map1 = new Map();
  const map2 = new Map();
  
  map1.set('a', 1);
  map1.set('b', 2);
  map2.set('b', 2);
  map2.set('a', 1);
  
  it('should ignore key order with Jest', () => {
    expect(map1).toEqual(map2);
  });
});

(We could probably start rendering "visual differences" here)

Details

It's the same for plain arrays.

// Fails
expect(['foo', 'bar']).toEqual(['bar', 'foo']);

But objects don't care about insertion order (which is not really specced, so makes sense)

// Passes
expect({ a: 1, b: 2 }).toEqual({ b: 2, a: 1 });

I'm not sure what the correct solution is, but it feels like we should be more consistent between Maps and Objects?

Details

This feels weird:

  it('should treat objects, and arrays of their keys, consistently', () => {
    const obj1 = { a: 1, b: 2 };
    const obj2 = { b: 2, a: 1 };

    // Passes
    expect(obj1).toEqual(obj2);
    // Fails
    expect(Object.keys(obj1)).toEqual(Object.keys(obj2));
  });

_{Put the parts about arrays in detail blocks, not really related to the issue at hand}

[leebyron]

Map is not ordered

…

On Sat, Oct 7, 2017 at 7:45 AM Simen Bekkhus ***@***.***> wrote: The behavior is consistent with normal JS Map. describe('comparison with Map', () => { const map1 = new Map(); const map2 = new Map(); map1.set('a', 1); map1.set('b', 2); map2.set('b', 2); map2.set('a', 1); it('should ignore key order with Jest', () => { expect(map1).toEqual(map2); }); }); [image: image] <https://user-images.githubusercontent.com/1404810/31307491-5a10f014-ab65-11e7-8885-48c3031b011b.png> It's the same for plain arrays. // Failsexpect(['foo', 'bar']).toEqual(['bar', 'foo']); [image: image] <https://user-images.githubusercontent.com/1404810/31307425-9dec3614-ab64-11e7-838b-255f446031ce.png> But objects don't care about insertion order // Passesexpect({ a: 1, b: 2 }).toEqual({ b: 2, a: 1 }); I'm not sure what the correct solution is, but it feels like we should be more consistent? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4618 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADD0kT6Mqjyx7zsQU1aFHGyNgrm5m4bks5sp2RigaJpZM4Pw2vL> .

[SimenB]

Indeed, which makes me think that both Maps from Immutable and standard JS Maps should behave like Object when checking equality. That's up to @cpojer though.

(we can safely ignore what I said about arrays, it's not relevant here. I'll put it in details)

[cpojer]

This is tricky because Jest doesn't have knowledge on how to sort two iterables to make them comparable. I think I'd be in favor of making toEqual work though if it isn't too big of a deal in the equality matcher.

[davidfou]

Here my 2 cents: why not first use Immutable.is then fallback to current behaviour only if it's false?

[SimenB]

Feels icky to have a dependency on immutable js in a generic assertion library. You can create your own assertion for that (and plug it in with expect.extend)

[davidfou]

That's right. So toEqual only use the output from pretty-format? Maybe this package should reorder on all unordered types recursively?

const initialMap = Immutable.Map({b: 1, a: 2});
const orderedMap = initialMap.toSeq().sortBy((v, k) => k).toMap();
Immutable.Map({b: 1, a: 2}).toSeq().sortBy((v, k) => k).toMap()

POC

[SimenB]

we do a deep equality (which doesn't know about immutable js), pretty-format is used to try to show a pretty diff afterwards

Again, I'd suggest to do create a custom matcher. See e.g. https://github.com/jest-community/jest-extended for lots of examples on how to create them

[seanli3]

The JS Map test given by @SimenB passes on the new Jest.

describe('comparison with Map', () => {
  const map1 = new Map();
  const map2 = new Map();
  
  map1.set('a', 1);
  map1.set('b', 2);
  map2.set('b', 2);
  map2.set('a', 1);
  
  it('should ignore key order with Jest', () => {
    expect(map1).toEqual(map2);
  });
});

Jest: 21.2.1
Node: 8.9.1
Yarn: 1.3.2

Should we make it consistent for Immutable Map?

[cpojer]

Yeah, we should make it consistent if possible.

[enrique-ramirez]

Hi!

I encountered this issue today and wanted to upvote this so it has a bit more priority. So, thanks!

[SimenB]

PR welcome 🙂