Closed humorless closed 1 year ago
Thanks @alysbrooks so much. After I tried to run the property test, I found out that there are a lot of edge cases which I did not think it throughly.
I can not make this property test pass until now.
However, I have considered more about the semantic of remove-unchanged
and I changed it a lot.
(defspec diff-itself 100
(prop/for-all [x diff-test/gen-any-except-NaN]
(if (coll? x)
(= (manipulate/remove-unchanged (ddiff/diff x x))
(empty x))
(= (manipulate/remove-unchanged (ddiff/diff x x))
nil))))
@plexus @alysbrooks
Do you think that I should dig it throughly to figure out why this property test can not work? Or the current remove-unchanged
you think is good enough?
What do the failing cases look like?
What do the failing cases look like?
FAIL in cljs:lambdaisland.deep-diff2.manipulate-test/diff-itself (cljs/test.js:429)
Expected:
{:result true}
Actual:
{:shrunk
{:total-nodes-visited 756,
:depth 51,
:pass? false,
:result false,
:result-data nil,
:time-shrinking-ms 168,
:smallest [#{0 1 false -6 -5 -4 -3 -2 -1}]},
:failed-after-ms 70,
:num-tests 59,
:seed 1680177300377,
:fail
[#{1.1197369622161879e-14 1.3019822841584656e-21 0.6875
#uuid "a907a7fe-d2eb-482d-b1cc-3acfc12daf55"
-30
:X/*!1:3
:u7*A/p?2IG5d*!Nl
:**d7ws
"ý"
"ÔB*àñS¬ÚûV¡ç¯±·á£H
û?'V$ëY;CLk-oOV"
!U-h_C*A7/x0_n1
A-*wn./o_?4w18-!
"ìêܼà4^¤mÐðktê1_ò· À4\n@J\"29)cd-\t®"
y3W-2
#uuid "6d507164-f8b9-401d-8c44-d6b0e310c248"
"M"
:cy7-3
:w4/R.-s?9V5
#uuid "1bcb00c9-88b9-4eae-9fea-60600dfaefa0"
-20
#uuid "269ab6f9-f19d-4c9d-a0cb-51150e52e9f7"
-235024979
:O:m_9.9+A/N+usPa6.HA*G
228944.657438457
:x/w?
:__+o+sut9!t/?0l
"â«"
false
#uuid "b6295f83-8176-47b5-946e-466f74226629"
e3zQ!E*5
:T5rb
:++y:2
-7364
zG/ex23
"¡"
-4318364480
:D+?2?!/Hrc!jA7z_2
:z-I/!8Uq+d?
-0.5588235294117647
-0.5925925925925926
-0.8108108108108109}],
:result false,
:result-data nil,
:failing-size 58,
:pass? false,
:test-var "diff-itself"}
The problem is not in remove-unchanged
, but in deep-diff itself. It seems for some sets we do some weird things:
(let [s #{false 5}]
(ddiff/diff s s))
;; => #{{:- 5} false {:+ 5}}
A little surprised that bug hasn't been discovered before. Maybe it's just sets with a mixture of types that have issues?
Logged a separate issue, seems it also happens for maps. I think the false
is part of the problem here.
https://github.com/lambdaisland/deep-diff2/issues/43
I renamed the function and namespace to mimize
, and added a top-level entrypoint.
@humorless seems you're all good now, the problem wasn't your code or the test, the problem was already there, but it's fixed now.
Two remarks,
diff-item?
where you check with instance?
but then later on you were checking for :-
or :+
, isn't that a problem if the user passes in a map with :+
or :-
key?diff-item?
or map keys, but map keys can also be arbitrary values, so we need to also check that none of their children have diff items.Actually, now that I write this I'm not sure if we descend into map keys when diffing...
Anyway, great work, happy to have this feature. Next we can think about how to expose this in kaocha.
The two remarks are totally true. I only thought of the most typical cases when implemented.
Try to solve the issue #13 with an API
remove-unchanged
, test, CHANGLOG