tapjs/tsame

Map<Set> vs Map<Set>

Closed this issue · 12 comments

krainboltgreene commented

I'm trying to compare two maps, that in console look the same, but tsame is telling me they are different.

Map {
  'sandwich' => Set {
  Map { 'name' => 'Hotdog', 'category' => 'sandwich' },
  Map { 'name' => 'Club Sandwich', 'category' => 'sandwich' } } }
Map {
  'sandwich' => Set {
  Map { 'name' => 'Hotdog', 'category' => 'sandwich' },
  Map { 'name' => 'Club Sandwich', 'category' => 'sandwich' } } }

./test.js ............................................. 2/3 858ms
  Multiple Map Set
  not ok should be equivalent
    +++ found
    --- wanted

    at:
      line: 130
      column: 3
      file: test.js
      type: Test
    stack: >
      Test.<anonymous> (test.js:130:3)

      Object.<anonymous> (test.js:65:1)

      Module.replacementCompile
      (node_modules/nyc/node_modules/append-transform/index.js:58:13)

      loader (node_modules/babel-register/lib/node.js:144:5)

      require.extensions.(anonymous function)
      (node_modules/babel-register/lib/node.js:154:7)

      Object.<anonymous>
      (node_modules/nyc/node_modules/append-transform/index.js:62:4)

      Object.<anonymous> (node_modules/nyc/bin/wrap.js:23:4)
krainboltgreene commented

Hey @isaacs, what can I do to help solve this problem?

isaacs commented

The problem is actually Set<Map> not Map<Set>. The simplest example would be new Set([{}])

The setMatch method is using obj.has(value), so it's tripping up on object identity in the set contents.

Since sets are not indexed, one possible fix is to walk the pattern set, and if an object is encountered, check to see if any objects are found that match the object in the pattern set.

However, this could lead to some weird situations. Consider this:

t.match(new Set([{a: 1}]), new Set([{a:1},{a:1}]))

If we did that, then we'd also have to track which objects had already been matched, so that a pattern of "a set of two foos" is not satisfied by an object which has only one foo.

isaacs commented

I guess t.same also checks that the lengths match, but you'd still have to track objects to be mindful of this problem:

t.same(new Set([{a: 1}, {b: 2}]), new Set([{a:1}, {a:1}]))
krainboltgreene commented

For what it's worth, I was noticing issues with same showing diffs for Map as well, as seen above.

krainboltgreene commented 
t.match(new Set([{a: 1}]), new Set([{a:1},{a:1}]))

Wouldn't that reduce to t.match(new Set([{a: 1}]), new Set([{a: 1}])) and therefor be correct?

isaacs commented

Nope. Object identity.

> {a:1} === {a:1}
false

Now, if you did:

const a = {a:1}
const s = new Set([a, a])
console.log(s.size) // 1
krainboltgreene commented

Oh crap.

krainboltgreene commented

Alright, so for Set what if we pulled everything to an array ([...new Set([{a: 1}, {a: 1}])]) then sort and then compare?

isaacs commented

Landed a semver-major fix in tsame and tmatch. will push to tap this evening or tomorrow.

Thanks!

krainboltgreene commented
isaacs commented

Sure, especially if you've got a repro case.

krainboltgreene commented

@isaacs Thank you so much for this. I was ready and willing to make a PR and you jumped on this without hesitation. Let me know if I can ever buy you a coffee!