From a58f5cd56aff89f179d5b3b01ed49fd86e04733a Mon Sep 17 00:00:00 2001 From: Nate Bosch Date: Mon, 22 Jul 2024 23:05:52 +0000 Subject: [PATCH 1/3] Special case maps with only value keys Closes #2256 When checking deep equality on Map instances (including maps nested in other collection types) we allow keys which are instances of other collection (and don't have a useful `operator ==`) and keys which are `Condition` callbacks that could potentially match multiple keys of the actual value. This is maximally flexible, but it loses the nested path information we are otherwise able to keep for iterables compared by index. Check whether expectation maps have any keys which need this special treatment, and if not, special case to an algorithm close to the `Iterable` algorithm which can enqueue nested checks and maintain the direct known path into the collection. Update the doc comment to describe the new behavior. This does potentially introduce confusion since a change which adds a `Condition` key has to potential to make _other_ keys in the same expectation behaved differently. The benefit is a more actionable failure output for the much more common case of checking expectations with hardcoded keys. --- pkgs/checks/CHANGELOG.md | 6 +- pkgs/checks/lib/src/collection_equality.dart | 151 +++++++++++++----- .../extensions/collection_equality_test.dart | 21 ++- 3 files changed, 138 insertions(+), 40 deletions(-) diff --git a/pkgs/checks/CHANGELOG.md b/pkgs/checks/CHANGELOG.md index 91cc3e986..ec6fa4d3d 100644 --- a/pkgs/checks/CHANGELOG.md +++ b/pkgs/checks/CHANGELOG.md @@ -1,6 +1,10 @@ ## 0.3.1-wip -- Update min SDK constraint to 3.4.0. +- Directly compare keys across actual and expected `Map` instances when + checking deep collection equality and all the keys can be directly compared + for equality. This maintains the path into a nested collection for typical + cases of checking for equality against a purely value collection. +- Update min SDK constraint to 3.4.0. ## 0.3.0 diff --git a/pkgs/checks/lib/src/collection_equality.dart b/pkgs/checks/lib/src/collection_equality.dart index 93281270c..c52cebf52 100644 --- a/pkgs/checks/lib/src/collection_equality.dart +++ b/pkgs/checks/lib/src/collection_equality.dart @@ -12,10 +12,10 @@ import '../context.dart'; /// {@template deep_collection_equals} /// Elements, keys, or values within [expected] which are a collections are /// deeply compared for equality with a collection in the same position within -/// [actual]. Elements which are collection types are note compared with the +/// [actual]. Elements which are collection types are not compared with the /// native identity based equality or custom equality operator overrides. /// -/// Elements, keys, or values within [expected] which are `Condition` callbacks +/// Elements, keys, or values within [expected] which are [Condition] callbacks /// are run against the value in the same position within [actual]. /// Condition callbacks must take a `Subject` or `Subject` and /// may not use a more specific generic. @@ -24,14 +24,25 @@ import '../context.dart'; /// Note also that the argument type `Subject` cannot be inferred and /// must be explicit in the function definition. /// -/// Elements, keys, or values within [expected] which are any other type are -/// compared using `operator ==` equality. +/// Elements and values within [expected] which are any other type are compared +/// using `operator ==` equality. /// -/// Comparing sets or maps will have a runtime which is polynomial on the the -/// size of those collections. Does not use [Set.contains] or [Map.containsKey], -/// there will not be runtime benefits from hashing. Custom collection behavior -/// is ignored. For example, it is not possible to distinguish between a `Set` -/// and a `Set.identity`. +/// Deep equality checks for [Map] instances depend on the structure of the +/// expected map. +/// If all keys of the expectation are non-collection and non-condition values +/// the keys are compared through the map instances with [Map.containsKey]. +/// +/// If any key in the expectation is a `Condition` or a collection type the map +/// will be compared as a [Set] of entries where both the key and value must +/// match. +/// +/// Comparing sets or maps with complex keys has a runtime which is polynomial +/// on the the size of those collections. +/// These comparisons do not use [Set.contains] or [Map.containsKey], +/// and there will not be runtime benefits from hashing. +/// Custom collection behavior of `contains` is ignored. +/// For example, it is not possible to distinguish between a `Set` and a +/// `Set.identity`. /// /// Collections may be nested to a maximum depth of 1000. Recursive collections /// are not allowed. @@ -70,7 +81,7 @@ Iterable? _deepCollectionEquals( } else { currentExpected as Map; rejectionWhich = _findMapDifference( - currentActual, currentExpected, path, currentDepth); + currentActual, currentExpected, path, queue, currentDepth); } if (rejectionWhich != null) return rejectionWhich; } @@ -165,39 +176,107 @@ Iterable? _findSetDifference( } Iterable? _findMapDifference( - Object? actual, Map expected, _Path path, int depth) { + Object? actual, + Map expected, + _Path path, + Queue<_Search> queue, + int depth) { if (actual is! Map) { return ['${path}is not a Map']; } - Iterable describeEntry(MapEntry entry) { - final key = literal(entry.key); - final value = literal(entry.value); - return [ - ...key.take(key.length - 1), - '${key.last}: ${value.first}', - ...value.skip(1) - ]; + if (expected.keys + .any((key) => key is Condition || key is Iterable || key is Map)) { + return _findAmbiguousMapDifference(actual, expected, path, depth); + } else { + return _findUnambiguousMapDifference(actual, expected, path, queue, depth); } +} - return unorderedCompare( - actual.entries, - expected.entries, - (actual, expected) => - _elementMatches(actual.key, expected.key, depth) && - _elementMatches(actual.value, expected.value, depth), - (expectedEntry, _, count) => [ - ...prefixFirst( - '${path}has no entry to match ', describeEntry(expectedEntry)), - if (count > 1) 'or ${count - 1} other entries', - ], - (actualEntry, _, count) => [ - ...prefixFirst( - '${path}has unexpected entry ', describeEntry(actualEntry)), - if (count > 1) 'and ${count - 1} other unexpected entries', - ], - ); +Iterable _describeEntry(MapEntry entry) { + final key = literal(entry.key); + final value = literal(entry.value); + return [ + ...key.take(key.length - 1), + '${key.last}: ${value.first}', + ...value.skip(1) + ]; +} + +/// Returns a description of a difference found between [actual] and [expected] +/// when [expected] has only direct key values and there is a 1:1 mapping +/// between an expected value and a checked value in the map. +Iterable? _findUnambiguousMapDifference( + Map actual, + Map expected, + _Path path, + Queue<_Search> queue, + int depth) { + for (final entry in expected.entries) { + assert(entry.key is! Condition); + assert(entry.key is! Iterable); + assert(entry.key is! Map); + if (!actual.containsKey(entry.key)) { + return prefixFirst( + '${path}has no key matching expected entry ', _describeEntry(entry)); + } + final expectedValue = entry.value; + final actualValue = actual[entry.key]; + + if (expectedValue is Iterable || expectedValue is Map) { + if (depth + 1 > _maxDepth) throw _ExceededDepthError(); + queue.addLast(_Search( + path.append(entry.key), actualValue, expectedValue, depth + 1)); + } else if (expectedValue is Condition) { + final failure = softCheck(actualValue, expectedValue); + if (failure != null) { + final which = failure.rejection.which; + return [ + 'has an element ${path.append(entry.key)}that:', + ...indent(failure.detail.actual.skip(1)), + ...indent(prefixFirst('Actual: ', failure.rejection.actual), + failure.detail.depth + 1), + if (which != null) + ...indent(prefixFirst('which ', which), failure.detail.depth + 1) + ]; + } + } else { + if (actualValue != expectedValue) { + return [ + ...prefixFirst('${path.append(entry.key)}is ', literal(actualValue)), + ...prefixFirst('which does not equal ', literal(expectedValue)) + ]; + } + } + } + for (final entry in actual.entries) { + if (!expected.containsKey(entry.key)) { + return prefixFirst( + '${path}has an unexpected key for entry ', _describeEntry(entry)); + } + } + return null; } +Iterable? _findAmbiguousMapDifference(Map actual, + Map expected, _Path path, int depth) => + unorderedCompare( + actual.entries, + expected.entries, + (actual, expected) => + _elementMatches(actual.key, expected.key, depth) && + _elementMatches(actual.value, expected.value, depth), + (expectedEntry, _, count) => [ + ...prefixFirst( + '${path}has no entry to match ', _describeEntry(expectedEntry)), + if (count > 1) 'or ${count - 1} other entries', + ], + (actualEntry, _, count) => [ + ...prefixFirst( + '${path}has unexpected entry ', _describeEntry(actualEntry)), + if (count > 1) 'and ${count - 1} other unexpected entries', + ], + ); + class _Path { final _Path? parent; final Object? index; diff --git a/pkgs/checks/test/extensions/collection_equality_test.dart b/pkgs/checks/test/extensions/collection_equality_test.dart index 516357bbc..28a10dd98 100644 --- a/pkgs/checks/test/extensions/collection_equality_test.dart +++ b/pkgs/checks/test/extensions/collection_equality_test.dart @@ -129,9 +129,9 @@ void main() { }, { 'a': (Subject it) => it.isA().startsWith('a') })).isNotNull().deepEquals([ - "has no entry to match 'a': ", + "has an element at ['a'] that:", + " Actual: 'b'", + " which does not start with 'a'", ]); }); @@ -147,6 +147,21 @@ void main() { ]); }); + test('maintains paths through maps when the keys are all values', () { + check(deepCollectionEquals({ + 'a': [ + {'b': 'c'} + ] + }, { + 'a': [ + {'b': 'd'} + ] + })).isNotNull().deepEquals([ + "at ['a'][<0>]['b'] is 'c'", + "which does not equal 'd'", + ]); + }); + test('reports recursive lists', () { var l = []; l.add(l); From a9cdf6156f2c9003f124ed0c060faa5f20e07ef1 Mon Sep 17 00:00:00 2001 From: Nate Bosch Date: Mon, 22 Jul 2024 23:32:52 +0000 Subject: [PATCH 2/3] Pull out copy/paste chunk from iterable difference --- pkgs/checks/lib/src/collection_equality.dart | 89 ++++++++------------ 1 file changed, 35 insertions(+), 54 deletions(-) diff --git a/pkgs/checks/lib/src/collection_equality.dart b/pkgs/checks/lib/src/collection_equality.dart index c52cebf52..b87f499c2 100644 --- a/pkgs/checks/lib/src/collection_equality.dart +++ b/pkgs/checks/lib/src/collection_equality.dart @@ -111,32 +111,38 @@ List? _findIterableDifference(Object? actual, 'expected an iterable with at least ${index + 1} element(s)' ]; } - var actualValue = actualIterator.current; - var expectedValue = expectedIterator.current; - if (expectedValue is Iterable || expectedValue is Map) { - if (depth + 1 > _maxDepth) throw _ExceededDepthError(); - queue.addLast( - _Search(path.append(index), actualValue, expectedValue, depth + 1)); - } else if (expectedValue is Condition) { - final failure = softCheck(actualValue, expectedValue); - if (failure != null) { - final which = failure.rejection.which; - return [ - 'has an element ${path.append(index)}that:', - ...indent(failure.detail.actual.skip(1)), - ...indent(prefixFirst('Actual: ', failure.rejection.actual), - failure.detail.depth + 1), - if (which != null) - ...indent(prefixFirst('which ', which), failure.detail.depth + 1) - ]; - } - } else { - if (actualValue != expectedValue) { - return [ - ...prefixFirst('${path.append(index)}is ', literal(actualValue)), - ...prefixFirst('which does not equal ', literal(expectedValue)) - ]; - } + final difference = _compareValue(actualIterator.current, + expectedIterator.current, path, index, queue, depth); + if (difference != null) return difference; + } + return null; +} + +List? _compareValue(Object? expectedValue, Object? actualValue, + _Path path, Object? pathAppend, Queue<_Search> queue, int depth) { + if (expectedValue is Iterable || expectedValue is Map) { + if (depth + 1 > _maxDepth) throw _ExceededDepthError(); + queue.addLast(_Search( + path.append(pathAppend), actualValue, expectedValue, depth + 1)); + } else if (expectedValue is Condition) { + final failure = softCheck(actualValue, expectedValue); + if (failure != null) { + final which = failure.rejection.which; + return [ + 'has an element ${path.append(pathAppend)}that:', + ...indent(failure.detail.actual.skip(1)), + ...indent(prefixFirst('Actual: ', failure.rejection.actual), + failure.detail.depth + 1), + if (which != null) + ...indent(prefixFirst('which ', which), failure.detail.depth + 1) + ]; + } + } else { + if (actualValue != expectedValue) { + return [ + ...prefixFirst('${path.append(pathAppend)}is ', literal(actualValue)), + ...prefixFirst('which does not equal ', literal(expectedValue)) + ]; } } return null; @@ -219,34 +225,9 @@ Iterable? _findUnambiguousMapDifference( return prefixFirst( '${path}has no key matching expected entry ', _describeEntry(entry)); } - final expectedValue = entry.value; - final actualValue = actual[entry.key]; - - if (expectedValue is Iterable || expectedValue is Map) { - if (depth + 1 > _maxDepth) throw _ExceededDepthError(); - queue.addLast(_Search( - path.append(entry.key), actualValue, expectedValue, depth + 1)); - } else if (expectedValue is Condition) { - final failure = softCheck(actualValue, expectedValue); - if (failure != null) { - final which = failure.rejection.which; - return [ - 'has an element ${path.append(entry.key)}that:', - ...indent(failure.detail.actual.skip(1)), - ...indent(prefixFirst('Actual: ', failure.rejection.actual), - failure.detail.depth + 1), - if (which != null) - ...indent(prefixFirst('which ', which), failure.detail.depth + 1) - ]; - } - } else { - if (actualValue != expectedValue) { - return [ - ...prefixFirst('${path.append(entry.key)}is ', literal(actualValue)), - ...prefixFirst('which does not equal ', literal(expectedValue)) - ]; - } - } + final difference = _compareValue( + entry.value, actual[entry.key], path, entry.key, queue, depth); + if (difference != null) return difference; } for (final entry in actual.entries) { if (!expected.containsKey(entry.key)) { From d0b089ccb53b628720ac0afdb8a5fd9ba30aec10 Mon Sep 17 00:00:00 2001 From: Nate Bosch Date: Tue, 23 Jul 2024 00:19:15 +0000 Subject: [PATCH 3/3] Use consistent argument order, actual before expected --- pkgs/checks/lib/src/collection_equality.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkgs/checks/lib/src/collection_equality.dart b/pkgs/checks/lib/src/collection_equality.dart index b87f499c2..da6064a52 100644 --- a/pkgs/checks/lib/src/collection_equality.dart +++ b/pkgs/checks/lib/src/collection_equality.dart @@ -118,7 +118,7 @@ List? _findIterableDifference(Object? actual, return null; } -List? _compareValue(Object? expectedValue, Object? actualValue, +List? _compareValue(Object? actualValue, Object? expectedValue, _Path path, Object? pathAppend, Queue<_Search> queue, int depth) { if (expectedValue is Iterable || expectedValue is Map) { if (depth + 1 > _maxDepth) throw _ExceededDepthError(); @@ -226,7 +226,7 @@ Iterable? _findUnambiguousMapDifference( '${path}has no key matching expected entry ', _describeEntry(entry)); } final difference = _compareValue( - entry.value, actual[entry.key], path, entry.key, queue, depth); + actual[entry.key], entry.value, path, entry.key, queue, depth); if (difference != null) return difference; } for (final entry in actual.entries) {