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

Set equality is unexpectedly ordered #93

Open
rnewman opened this issue Apr 25, 2016 · 11 comments
Open

Set equality is unexpectedly ordered #93

rnewman opened this issue Apr 25, 2016 · 11 comments

Comments

@rnewman
Copy link

rnewman commented Apr 25, 2016

  it('Compares sets correctly.', (done) => {
    expect(new Set(['a', 'b'])).toEqual(new Set(['b', 'a']));
    done();
  });
      Error: Expected Set (2) {'a', 'b'} to equal Set (2) {'b', 'a'}
      + expected - actual

      -["a","b"]
      +["b","a"]

This is with 1.18.0. I expect set equality to be independent of order; the order of elements is an implementation detail.

@ljharb
Copy link
Collaborator

ljharb commented Apr 25, 2016

Your expectation is incorrect - Set in JavaScript has a defined and consistent implementation order, and is a spec detail, not an implementation detail.

@rnewman
Copy link
Author

rnewman commented Apr 25, 2016

I wouldn't expect the preserved insertion order of the set — which is noted in the spec — to influence its equality properties, but hey, JS is full of surprises. Time to switch to Immutable.js for this, I guess.

@rnewman rnewman closed this as completed Apr 25, 2016
@ljharb
Copy link
Collaborator

ljharb commented Apr 25, 2016

I think it'd be reasonable to have a method that explicitly checked that two Sets/Maps/arrays had the same contents, irrespective of order.

@mjackson
Copy link
Owner

mjackson commented May 5, 2016

@ljharb What would you call the to method?

@ljharb
Copy link
Collaborator

ljharb commented May 5, 2016

hmm - naming is hard :-) something like expect(foo).toBeTheSameSetAs(bar) would work but that uses the math definition of Set, not the JS one. expect(foo).toMatch(bar) maybe?

@mjackson
Copy link
Owner

mjackson commented May 5, 2016

How about expect(a).toEqualSet(b)? expect-jsx already uses toEqualJSX.

@mjackson
Copy link
Owner

mjackson commented May 5, 2016

Actually, just found this:

Two sets A and B are said to be equal if every element of A is an element of B and every element of B is an element of A. The equality of sets A and B is denoted by A = B. Inequality of two sets A and B is denoted by A ≠ B.

Seems like we should just do some type detection in toEqual and toNotEqual and compare sets by their contents, irrespective of order.

@ljharb
Copy link
Collaborator

ljharb commented May 5, 2016

@mjackson if it's generic, that would break tests that currently compare array orderings. If it's not - and only for Set/Map - then you only have a slow try/catch way of brand-checking that it's a Set or a Map, and, Sets and Maps in JS are implicitly ordered.

@mjackson
Copy link
Owner

mjackson commented May 5, 2016

Even though they are implicitly ordered, conceptually a Set is not. toEqual is the right method IMO.

@ljharb
Copy link
Collaborator

ljharb commented May 6, 2016

I don't agree in the case of JS - but it wouldn't be a tragedy imo to have toEqual compare Sets and Maps in an unordered fashion - if i want ordering, I can [...foo] and compare those.

@mjackson
Copy link
Owner

mjackson commented May 6, 2016

I'd be open to a PR that does this, @rnewman, if you're up for it. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants