tape-testing / tape

tap-producing test harness for node and browsers
MIT License
5.77k stars 307 forks source link

deepEqual may be broken? #528

Closed mixmix closed 4 years ago

mixmix commented 4 years ago

The following fails in tape@5, and passes in tape@4 ?!

const test = require('tape')

test('deepEqual', t => {
  const actual = [
    { id: '%x7tx05ZDJlZyhbFQKEMIah3i9puOE43PwuNvX4d/3rE=.sha256' },
    { id: '%/KZzVNBS97oqDtMNqaqu0mDYJT1LlXIfRn7cMcEjPLk=.sha256' }
  ]

  const expecte = [
    { id: '%x7tx05ZDJlZyhbFQKEMIah3i9puOE43PwuNvX4d/3rE=.sha256' }, 
    { id: '%/KZzVNBS97oqDtMNqaqu0mDYJT1LlXIfRn7cMcEjPLk=.sha256' }
  ]

  t.deepEqual(actual, expected)
  t.end()
})
ORESoftware commented 4 years ago

always include the error

ljharb commented 4 years ago

Seems to pass in tape 5 for me (with the proper variable spelling).

Happy to reopen if I can reproduce the issue (note that "works in v4, not in v5" isn't necessarily a bug, breaking changes across major versions are the reason for a major version)

mixmix commented 4 years ago

Thanks for comparing. Strangely when I ran the code I posted in the tape repo (@5.0.1) it passes for me. In my project though I see:

      operator: deepEqual
      expected: |-
        [ { id: '%BFLDq7Upme0Q4Jgqz+hM4eP/TQ0/S4UOTnTKwsB+tDU=.cloaked' } ]
      actual: |-
        [ { id: '%BFLDq7Upme0Q4Jgqz+hM4eP/TQ0/S4UOTnTKwsB+tDU=.cloaked' } ]
      at: Test.test (/home/mix/projects/SSBC/ssb-graphql/tribes/test/groups.test.js:51:5)
      stack: |-
  Error: should be deeply equivalent
  at Test.assert [as _assert] (/home/mix/projects/SSBC/ssb-graphql/tribes/node_modules/tape/lib/test.js:260:54)
  at Test.bound [as _assert] (/home/mix/projects/SSBC/ssb-graphql/tribes/node_modules/tape/lib/test.js:84:32)
  at Test.tapeDeepEqual (/home/mix/projects/SSBC/ssb-graphql/tribes/node_modules/tape/lib/test.js:501:10)
  at Test.bound [as deepEqual] (/home/mix/projects/SSBC/ssb-graphql/tribes/node_modules/tape/lib/test.js:84:32)
  at Test.test (/home/mix/projects/SSBC/ssb-graphql/tribes/test/groups.test.js:51:5)

and

      operator: deepEqual
      expected: |-
        [ { id: '%1ORk8YoGTK0A0iJ+9Vk48gAWah/fk8S8lCCmEYm5JgU=.sha256', feedId: '@geT0tPv9enmqM2mXVvcmG9FkM+pWyS56VTZmdKAHYIU=.ed25519', preferredName: 'alice' }, { id: '%hXCPRjk5TUe60OXrBUVL4EjdIvOOpJ9snQ+4UzFq52M=.sha256', feedId: '@YIY1NbjRPBl9vHrc6rNIgOnDvONto0ivjooCiIAlpw4=.ed25519', preferredName: 'bob' } ]
      actual: |-
        [ { id: '%1ORk8YoGTK0A0iJ+9Vk48gAWah/fk8S8lCCmEYm5JgU=.sha256', feedId: '@geT0tPv9enmqM2mXVvcmG9FkM+pWyS56VTZmdKAHYIU=.ed25519', preferredName: 'alice' }, { id: '%hXCPRjk5TUe60OXrBUVL4EjdIvOOpJ9snQ+4UzFq52M=.sha256', feedId: '@YIY1NbjRPBl9vHrc6rNIgOnDvONto0ivjooCiIAlpw4=.ed25519', preferredName: 'bob' } ]
      at: Test.test (/home/mix/projects/SSBC/ssb-graphql/tribes/test/authors.test.js:49:5)
      stack: |-
  Error: gets group authors
  at Test.assert [as _assert] (/home/mix/projects/SSBC/ssb-graphql/tribes/node_modules/tape/lib/test.js:260:54)
  at Test.bound [as _assert] (/home/mix/projects/SSBC/ssb-graphql/tribes/node_modules/tape/lib/test.js:84:32)
  at Test.tapeDeepEqual (/home/mix/projects/SSBC/ssb-graphql/tribes/node_modules/tape/lib/test.js:501:10)
  at Test.bound [as deepEqual] (/home/mix/projects/SSBC/ssb-graphql/tribes/node_modules/tape/lib/test.js:84:32)
  at Test.test (/home/mix/projects/SSBC/ssb-graphql/tribes/test/authors.test.js:49:5)

I'm running node v10.15.3

mixmix commented 4 years ago

also fails for me in node v12.18.2

ljharb commented 4 years ago

If you can create a repo that reproduces the problem, I will be able to address it very quickly.

Note that there might be things that aren't shown in the output that differentiate those objects - for example, one of them might have a null [[Prototype]] while the other does not.

mixmix commented 4 years ago

https://gitlab.com/ahau/ssb-graphql-tribes/-/merge_requests/5

On Sep 5 2020, at 5:36 pm, Jordan Harband notifications@github.com wrote:

If you can create a repo that reproduces the problem, I will be able to address it very quickly. Note that there might be things that aren't shown in the output that differentiate those objects - for example, one of them might have a null [[Prototype]] while the other does not. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub (https://github.com/substack/tape/issues/528#issuecomment-687554914), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAUK3HWK54DQ2ZC7TSYDM73SEHE4NANCNFSM4QXEUVSA).

ljharb commented 4 years ago

Thanks! I cloned it, and added a console log of the results of running is-equal/why, which reported "arguments have a different [[Prototype]]", as I suspected. When I then log out Object.getPrototypeOf() on both objects, the first gives null and expected gives Object.prototype.

After adding __proto__: null to the expected object (note: in every nested object literal too, not just the top) fixes the assertion. Alternatively, you can use t.deepLooseEqual instead of t.deepEqual - the default strictness of t.deepEqual changed in v5.

Your tests have one additional error, which is that you're using an async function, which returns a promise to Tape, but you're calling t.end asynchronously which causes a conflict. Change ssb.close(t.end) to return new Promise((resolve) => ssb.close(resolve)) and that'll be resolved too (this is a fix in v5; mixing promises and callbacks in this way was always incorrect, but tape v4 ignored the returned Promise)

mixmix commented 4 years ago

Thank you for the detailed breakdown @ljharb , really appreciate you taking the time to describe what was happening. I'm confused about how the prototype thing happened ... will have to read up on that.

One strange thing about the async end problem is I see different behaviour based on how I run the tests :

ljharb commented 4 years ago

That is very strange; but I suspect that might be some kind of test pollution due to the "sodium" stuff that pops up? I would definitely expect both of those to behave the same.