recover after missing/extra element in sequence #187
Conversation
before: ``` expected: (match? [1 2 3 4] [1 3 4]) actual: [1 (mismatch (expected 2) (actual 3)) (mismatch (expected 3) (actual 4)) (missing 4)] ``` after: ``` expected: (match? [1 2 3 4] [1 3 4]) actual: [1 (expected 2) 3 4] ```
| (map vector | ||
| (repeat matchers) | ||
| (insert-all-combinations actuals | ||
| (repeat (- matchers-count (count actuals)) | ||
| ::missing))) |
There was a problem hiding this comment.
This might make computing mismatches for sequences a slower.
One way to deal with it would be to create a dedicated matcher that enables this logic. I don't like this because it creates more options for users and this new mismatch behavior is probably something people want by default.
Another way to limit how much it could potentially slow things down would be to cap how many combinations are considered. Something like a (take 30 ..) around the (insert-all-combinations ..) thing here. (probably bound to a dynamic var for configuration)
There was a problem hiding this comment.
thoughts on this @dchelimsky and also @aredington, given your work on #166 ?
|
Super cool @philomates! I've played with this PR in a scratch project to get a feel for it when used in tests. The terminology of Here's my little playground: (ns mc-recover.mc-recover-test
(:require [clojure.test :as t :refer [deftest is testing]]
[matcher-combinators.test]))
(deftest mc-recover
;; playing around with matcher-combinators seq recovery
;; very cool!
;; all assertions will fail.
(is (match? [1 2] [1 2 3 4]) "extra trailing")
(is (match? [1 2] [9 0 1 2]) "extra leading")
(is (match? [1 2] [1 98 99 2]) "extra internal")
(is (match? [1 2] [9 8 1 98 99 2 3 4]) "extra leading, internal, trailing")
(is (match? [] [1 2 3]) "all extra")
(is (match? [1 2 3] [1 2]) "missing trailing")
(is (match? [1 2 3] [2 3]) "mising leading")
(is (match? [1 99 2] [1 2]) "missing internal")
(is (match? [0 1 2 99 98 97 3 4 5] [2 99 3]) "missing leading, internal, trailing")
(is (match? [1 2 3] []) "all missing")
(is (match? [1 82 3 4 2 7 9 10] [1 2 10 11]) "recover multiple mismatches")
(is (match? [1 [2 3 4] 5] [1 [2 3] 5]) "nested mismatch")
(is (match? [1 2 3] (list 1 3)) "mismatch on different seq types")
(is (match? [:x :y [:a :b :c] :z] [:x [:a :b :c]]) "recovery works on nested element")
(is (match? [#"some.*" :y 3 #"other.*" 5] ["somestring" :y 4 "nope" 5]) "other matchers still work")
(is (match? {:a {:b [3 4 5]}} {:a {:b [3 4 5 6]}}) "nested mismatch in a map"))
And the results it produced with the cognitect test runner: The only case that surprised me slightly was this one: I was thinking that maybe we'd get a missing for 9, a match for 10, and an unexpected for 11. The formatting of actual, when actual is multiple lines is, I'm pretty sure, a pre-existing and separate issue. |
|
For comparison, that last example when diffed with deep-diff2: Here's a screenshot for the colouring: It seems to behave the way I would have expected, maybe we can do the same after all. |
|
Thanks for the comprehensive testing @lread for (is (match? [1 82 3 4 2 7 9 10] [1 2 10 11]) "recover multiple mismatches")the current behavior gives 6 mismatches and 2 matches, for a total length of 8 elements the deepdiff version gives which has 3 matches and 6 mismatches, for a total length of 9 elements. |
|
Yeah, good point, there there is no absolute right or wrong. I prefer a diff which makes some attempt to re-sync after a mismatch. But that might be my subjective preference. It could be interesting to learn more about deep-diff2's rationale and intent with regards to re-sync. Deep-diff2 uses clj-diff, which describes its diffing algorithm use here. I don't know much about diff algorithms, but I think an "optimal" one produces the smallest number of diffs, which seems related. |
|
I think that
We could
I'm happy with any of this, as eve without the deepdiff alignment, this is a lot better as/is. I think the best option is the last one, if you're good with that. |
|
I'm fine with merging as is and doing a follow-up to improve the results to better match deepdiff. |
addresses #177
before:
after: