Fix unorderedMatches for poorly ordered input #74

natebosch · 2018-04-11T00:48:15Z

Reimplements the unordered matcher using a recursive search assuming
that matchers can match multiple values in the input rather than a
greedy algorithm which allows a matcher to "consume" a value that is
needed for some other matcher.

User visible differences:

May match inputs that would have previously been (incorrectly)
rejected.
The failure description no longer includes the index of the matcher
which is unmatched, and all unmatched matchers are printed rather than
the first.

Fixes #73 Reimplements the unordered matcher using a recursive search assuming that matchers can match multiple values in the input rather than a greedy algorithm which allows a matcher to "consume" a value that is needed for some other matcher. User visible differences: - May match inputs that would have previously been (incorrectly) rejected. - The failure description no longer includes the index of the matcher which is unmatched, and all unmatched matchers are printed rather than the first.

jakemac53 · 2018-04-11T13:35:10Z

CHANGELOG.md

@@ -1,3 +1,9 @@
+## 0.12.2
+
+* Fixed `unorderedMatches` in cases where the matchers may match more than one


I am a little bit concerned that this is actually a breaking change... have you tried it internally? My guess is there would be only a small handful of breakages so it would be reasonable to roll but it would be good to get some idea.

I could not find any breakages related to this change in a global presubmit. I scanned through the few failures and they all looked like flakes.

We do have other options though. I am fixing this because I want the correct semantics for containsAll - https://github.com/dart-lang/matcher/issues/72, but we could also implement the correct semantics there and deprecate this broken one.

Once we have containsAll then unorderedMatches (and by extension unorderedEquals) become pretty much unnecessary because these are equivalent:

expect(something, unorderedEquals(expected); // can also be written as: expect(something.length, expected.length); expect(something, containsAll(expected)));

And unorderedEquals could always be substituted with unorderedMatches as long as none of the values were already matchers.

Synced offline, since we didn't find any broken tests with a large corpus we are comfortable moving forward with this change.

If we don't see large numbers of breakages internally then I say its safe to land 👍

jakemac53 · 2018-04-11T13:38:37Z

lib/src/iterable_matchers.dart

@@ -145,7 +145,7 @@ abstract class _IterableMatcher extends Matcher {
 /// Returns a matcher which matches [Iterable]s whose elements match the
 /// matchers in [expected], but not necessarily in the same order.
 ///
-///  Note that this is `O(n^2)` and so should only be used on small objects.
+///  Note that this is `O(n^2)` and so should only be used on small iterables.


Maybe note that its also now n^2 in memory usage

jakemac53 · 2018-04-11T13:41:26Z

lib/src/iterable_matchers.dart

-              break;
-            }
+      var adjacency = list
+          .map((_) => new List.filled(_expected.length, false, growable: false))


Note that you could instead start with empty List<int>s and then insert indexes of matches to save on memory in typical cases (worst case would still be n^2, but should be quite rare). Given that this method already comes with a warning on it I don't think its particularly important if it overcomplicates things.

Done. In the small value case it probably makes memory usage a bit worse since I suspect the default empty list allocates more memory than these fixed lists - but I think it make the code more clear and should be an improvement for large (sparsely matching) inputs.

jakemac53 · 2018-04-11T13:53:27Z

lib/src/iterable_matchers.dart

+      }
+      // The index into `values` matched with each matcher
+      var matched = new List<int>.filled(_expected.length, -1, growable: false);
+      for (int valueIndex = 0; valueIndex < list.length; valueIndex++) {


nit: Can we rename list to something more descriptive (not your code I know but it would help readability here).

Changed to values.

jakemac53 · 2018-04-11T14:22:18Z

lib/src/iterable_matchers.dart

+  ///
+  /// Recursively looks for new pairings whenever there is a conflict. [seen]
+  /// tracks the matchers that have already been consumed within this search.
+  bool _findPairing(List<List<bool>> adjacency, int valueIndex, Set<int> seen,


nit: consider making seen optional and then doing seen ??= new Set<int>(); so you don't have to pass in a new one when you call this method originally

jakemac53 · 2018-04-11T14:24:22Z

lib/src/iterable_matchers.dart

+  bool _findPairing(List<List<bool>> adjacency, int valueIndex, Set<int> seen,
+      List<int> matched) {
+    for (int i = 0; i < matched.length; i++) {
+      if (adjacency[valueIndex][i] && !seen.contains(i)) {


Note that my suggestion above (a list of ints instead of bools for adjacency) would make this loop easier and more efficient also (just iterate adjacency[valueIndex]).

Yeah I like they way that looks. Also lets me pull the seen.contains check out of the loop into an Iterable.where which I think helps.

jakemac53 · 2018-04-11T14:25:49Z

lib/src/iterable_matchers.dart

-        if (!gotMatch) {
+      }
+      // The index into `values` matched with each matcher
+      var matched = new List<int>.filled(_expected.length, -1, growable: false);


nit: I would slightly prefer null to -1.

jakemac53 · 2018-04-11T14:29:59Z

lib/src/iterable_matchers.dart

+      if (adjacency[valueIndex][i] && !seen.contains(i)) {
+        seen.add(i);
+        if (matched[i] < 0 ||
+            _findPairing(adjacency, matched[i], seen, matched)) {


This is really hard to follow, please add a comment describing the logic or simplify it if possible.

If we didn't have a previous match, use this as the match and return. Otherwise < describe logic for recursive call to _findPairing>.

Specifically understanding interactions of seen and matched through repeated recursive calls here is really difficult to understand.

Updated the doc comment, added some intermediate variables with descriptive names, and added a block comment at the point where we recurse. WDUT?

jakemac53 · 2018-04-11T14:40:01Z

test/iterable_matchers_test.dart

@@ -167,14 +176,14 @@ void main() {
        unorderedMatches([3, 1]),
        "Expected: matches [<3>, <1>] unordered "
        "Actual: [1, 2] "
-        "Which: has no match for <3> at index 0");
+        "Which: has no match for <3>");


fwiw, the index was actually useful here previously, especially in the case of objects that don't have good string representations. Imagine a List<Foo> where Foo has no custom toString method (or a not useful one), how do I know what one didn't match?

I think I can add back the index. Do we think it's helpful to list out multiple matchers that were unmatched or should I stop at the first one like the old implementation?

Added back the index and shortened the rest of the message to "along with n other unmatched"

jakemac53 · 2018-04-11T19:30:03Z

lib/src/iterable_matchers.dart

-              .addDescriptionOf(expectedMatcher)
-              .add(' at index $expectedPosition')
-              .toString();
+    var edges = values.map((_) => <int>[]).toList(growable: false);


nit: you could use List.filled

I don't think I can because that would give us the same instance of a list at every index.

Sorry meant List.generate

Sure, it's a few extra characters for now but once we can remove new it'll fit on one line again and I think the intent is more clear that the .toList() call.

jakemac53 · 2018-04-11T19:35:52Z

lib/src/iterable_matchers.dart

+  bool _findPairing(List<List<int>> edges, int valueIndex, List<int> matched,
+      [Set<int> seen]) {
+    seen ??= new Set<int>();
+    final possiblePairings = edges[valueIndex].where((m) => !seen.contains(m));


I was going to say this doesn't allow you to bail early, but then I remembered Iterables are lazy :D. Dart ftw!

jakemac53 · 2018-04-11T19:38:07Z

lib/src/iterable_matchers.dart

+    for (final matcherIndex in possiblePairings) {
+      seen.add(matcherIndex);
+      final previouslyMatched = matched[matcherIndex];
+      final canPlaceWithoutConflict = previouslyMatched == null;


nit: I don't think this local var is necessary, or you could eliminate previouslyMatched and move the null check up

Removed canPlaceWithoutConflict

jakemac53 · 2018-04-11T19:43:24Z

test/iterable_matchers_test.dart

    var d = [1, 2];
    shouldPass(d, unorderedMatches([2, 1]));
    shouldPass(d, unorderedMatches([greaterThan(1), greaterThan(0)]));
+    shouldPass(d, unorderedMatches([greaterThan(0), greaterThan(1)]));
+    shouldPass([2, 1], unorderedMatches([greaterThan(1), greaterThan(0)]));
+    shouldPass([2, 1], unorderedMatches([greaterThan(0), greaterThan(1)]));


Due to the complexity of how this algorithm actually works, it would be good to add a more exhaustive test as well.

what did you have in mind? I think n > 2 and checking every ordering of expected and the values should be exhaustive?

More items, some of which overlap with other ones, some of which are only one. Maybe something like this...

shouldPass( [1, 2, 3, 4, 5, 6], unorderedMatches([ greaterThan(1), // 6 equals(2), // 2 allOf([lessThan(3), isNot(0)]), // 1 equals(0), // 0 predicate((v) => v % 2 == 1), // 3 equals(5), // 5 ]));

I think the commented numbers are the only possible solution so that should be complicated enough ;).

bonus points if you go through all possible orderings of those matchers and test each of them haha

Fixes dart-lang/matcher#73 Reimplements the unordered matcher using a recursive search assuming that matchers can match multiple values in the input rather than a greedy algorithm which allows a matcher to "consume" a value that is needed for some other matcher. User visible differences: - May match inputs that would have previously been (incorrectly) rejected. - The failure description may include a number of unmatched expectations if more than 1 is unmatched.

natebosch requested review from jakemac53 and grouma April 11, 2018 00:48

googlebot added the cla: yes label Apr 11, 2018

jakemac53 requested changes Apr 11, 2018

View reviewed changes

natebosch added 2 commits April 11, 2018 12:12

Initial round of review comments

9d1e626

Bring back unmatched index

a4235f4

jakemac53 reviewed Apr 11, 2018

View reviewed changes

natebosch added 5 commits April 11, 2018 12:52

Unnecessary local

c160ccc

Dependency override on test to allow version solve

e74a8bc

complicated test

9fd338c

List.generate

a474768

Rename 'seen' as 'reserved'

c0e4224

jakemac53 approved these changes Apr 11, 2018

View reviewed changes

natebosch merged commit 03c582f into master Apr 11, 2018

natebosch deleted the fix-unordered-matches branch April 11, 2018 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unorderedMatches for poorly ordered input #74

Fix unorderedMatches for poorly ordered input #74

natebosch commented Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

jakemac53 Apr 11, 2018 •

edited

Loading

jakemac53 Apr 11, 2018

natebosch Apr 11, 2018

		@@ -1,3 +1,9 @@
		## 0.12.2

		* Fixed `unorderedMatches` in cases where the matchers may match more than one

Fix unorderedMatches for poorly ordered input #74

Fix unorderedMatches for poorly ordered input #74

Conversation

natebosch commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakemac53 Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakemac53 Apr 11, 2018 •

edited

Loading