-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Edge case weirdness with new deepEqual setup #1431
Comments
Wow, nice find. I'll take a look at that! |
Well this is a tricky one! Note that though both arrays look the same, there is one major difference. In the first array, items The underlying comparison library describes each value it encounters. Comparisons are done on these descriptor objects. It also detects when object trees contain the same objects (by identity), describing these as pointers. Pointers are expected to be equal.
This trips up the When the When the Fixing that leads to another problem: a diff is shown for
That's pretty confusing! Perhaps it could be improved by including an "address" after the A question for you @tquetano-r7 and also @avajs/core: would you expect If we decide not to care then the pointer stuff can be removed entirely, allowing your test to pass without even having to diff anything. Unfortunately pointers also play a role in circular reference detection so removing it isn't straight-forward either. |
Personally I would not expect |
Before this commit, Concordance attempted to determine whether a complex value was reused in the same location on either side of a comparison. It did this by detecting value reuse and emitting a Pointer descriptor instead. This required lookups to be maintained, to map pointers back to previously seen descriptors. It turned out that maintaining these lookups is hard, and there were various code paths where Concordance should have added descriptors to the lookup tables but didn't. Furthermore, pointer indexes are simple integer increments, which means that the same value in either side of the comparison may actually have a different index. This breaks the comparison logic. avajs/ava#1431 shows some of the issues that arose from this approach. This commit simplifies the implementation. When describing, if the same value is encountered then the first descriptor is emitted. The serialization logic tracks complex descriptors by their pointer index and serializes a pointer descriptor if reuse is detected. Deserialization ensures that the first descriptor is emitted, rather than the pointer. Circular references are now tracked for each side of the comparison. A stack is maintained, and if at any stage in the comparison a circular reference is detected, then the result of that comparison is only equal if both sides have the same circular reference. Which is a long way of saying that some circular references are considered equal, and others are not. This shouldn't matter much for real-life uses of Concordance, though it is a breaking change.
Yea, anything else is too dificult. @tquetano-r7 in your project, could you run |
Before this commit, Concordance attempted to determine whether a complex value was reused in the same location on either side of a comparison. It did this by detecting value reuse and emitting a Pointer descriptor instead. This required lookups to be maintained, to map pointers back to previously seen descriptors. It turned out that maintaining these lookups is hard, and there were various code paths where Concordance should have added descriptors to the lookup tables but didn't. Furthermore, pointer indexes are simple integer increments, which means that the same value in either side of the comparison may actually have a different index. This breaks the comparison logic. avajs/ava#1431 shows some of the issues that arose from this approach. This commit simplifies the implementation. When describing, if the same value is encountered then the first descriptor is emitted. The serialization logic tracks complex descriptors by their pointer index and serializes a pointer descriptor if reuse is detected. Deserialization ensures that the first descriptor is emitted, rather than the pointer. Circular references are now tracked for each side of the comparison. A stack is maintained, and if at any stage in the comparison a circular reference is detected, then the result of that comparison is only equal if both sides have the same circular reference. Which is a long way of saying that some circular references are considered equal, and others are not. This shouldn't matter much for real-life uses of Concordance, though it is a breaking change.
Before this commit, Concordance attempted to determine whether a complex value was reused in the same location on either side of a comparison. It did this by detecting value reuse and emitting a Pointer descriptor instead. This required lookups to be maintained, to map pointers back to previously seen descriptors. It turned out that maintaining these lookups is hard, and there were various code paths where Concordance should have added descriptors to the lookup tables but didn't. Furthermore, pointer indexes are simple integer increments, which means that the same value in either side of the comparison may actually have a different index. This breaks the comparison logic. avajs/ava#1431 shows some of the issues that arose from this approach. This commit simplifies the implementation. When describing, if the same value is encountered then the first descriptor is emitted. The serialization logic tracks complex descriptors by their pointer index and serializes a pointer descriptor if reuse is detected. Deserialization ensures that the first descriptor is emitted, rather than the pointer. Circular references are now tracked for each side of the comparison. A stack is maintained, and if at any stage in the comparison a circular reference is detected, then the result of that comparison is only equal if both sides have the same circular reference. Which is a long way of saying that some circular references are considered equal, and others are not. This shouldn't matter much for real-life uses of Concordance, though it is a breaking change.
Description
It appears with the new diff mechanism, some very edge-case weirdness is happening. I encountered it a scenario (sorry, this is seriously the least involved example I could reproduce this with) where:
js
Errors out:
But if you comment out any of the items in the arrays, it passes. Also, if you comment out the
'foo.bar': fakeStuff
line from each, it also passes!Finally ...
Also passes. It is literally just the
deepEqual
of the main objects that doesn't. This is a workaround for now, but this is hinky.Error Message & Stack Trace
In some cases, the error mentioned above fails with:
However in rare cases, i actually get a diff ... where the
dot.separated
property value is deemed inequal, but it is clearly equal in value. The worst one is when I have a larger array of this issue, where the output I get in the console is:Very unhelpful.
Config
Copy the relevant section from
package.json
:Environment
Node.js = 6.10.2
OS = Manjaro Linux
AVA = 0.20.0
npm = 3.10.0
yarn = 0.24.6
The text was updated successfully, but these errors were encountered: