Skip to content
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

AllOf() matcher crashes if inner matchers throw #190

Open
bobjackman opened this issue Aug 13, 2022 · 1 comment
Open

AllOf() matcher crashes if inner matchers throw #190

bobjackman opened this issue Aug 13, 2022 · 1 comment

Comments

@bobjackman
Copy link

bobjackman commented Aug 13, 2022

I don't have time to create a full PR with tests, but here's a repro and a patch if someone has the time to integrate them.

I believe this is closely related to, but still distinct from #174

Repro

import 'package:test/test.dart';

void main() {
  test('foo', () {
    var myVal = 'somestring';
    expect(myVal, allOf(containsPair('foo', 'bar')), reason: 'oh noes!');
  });
}

Which outputs:

dart:core                                         Object.noSuchMethod
package:matcher/src/operator_matchers.dart 61:13  _AllOf.describeMismatch
package:test_api                                  expect
test\demo.dart 6:5                                main.<fn>

NoSuchMethodError: The method 'describeMismatch' was called on null.
Receiver: null
Tried calling: describeMismatch("somestring", Instance of 'StringDescription', null, false)

Fix

--- a/lib/src/operator_matchers.dart
+++ b/lib/src/operator_matchers.dart
@@ -46,10 +46,15 @@ class _AllOf extends Matcher {
   @override
   bool matches(dynamic item, Map matchState) {
     for (var matcher in _matchers) {
+      try {
         if (!matcher.matches(item, matchState)) {
           addStateInfo(matchState, {'matcher': matcher});
           return false;
         }
+      } catch(_) {
+        addStateInfo(matchState, {'matcher': matcher});
+        rethrow;
+      }
     }
     return true;
   }

Alternate Fix

Since the original code didn't only called addStateInfo() for a failed match, the above assumes that's important and as such, preserves the same logic (again, I didn't have time to explore in more depth). If this is not the case and it's indeed acceptable to call addStateInfo() in all cases, then the fix becomes simpler:

--- a/lib/src/operator_matchers.dart
+++ b/lib/src/operator_matchers.dart
@@ -46,10 +46,10 @@ class _AllOf extends Matcher {
   @override
   bool matches(dynamic item, Map matchState) {
     for (var matcher in _matchers) {
+        addStateInfo(matchState, {'matcher': matcher});
         if (!matcher.matches(item, matchState)) {
-          addStateInfo(matchState, {'matcher': matcher});
           return false;
         }
     }
     return true;
   }

Workaround

For the time being, this crash can be avoided by rewriting the test like so:

-expect(myVal, allOf(containsPair('foo', 'bar')), reason: 'oh noes!');
+expect(myVal, isA<Map>().having((x) => x, 'that', allOf(containsPair('foo', 'bar'))), reason: 'on noes!');
@bobjackman
Copy link
Author

In a similar vein:

var expectedValues = generateValues()
  ..renameKey('someKey', 'someOtherExistingKey'); // -- throws `Bad state: Existing element would be overwrittenexisting value`, but gets supressed due to below

expect(actualValues, expectedValues);

which outputs:

package:matcher/src/equals_matcher.dart 272:43  _DeepMatcher.describeMismatch
package:test_api                                expect
test\models\User_test.dart 59:7                 main.<fn>.<fn>

type 'Null' is not a subtype of type '_Mismatch' in type cast

Similarly, the workaround for this one is also:

-expect(actualValues, equals(expectedValues));
+expect(actualValues, isA<Map>().having((x) => x, 'that', equals(expectedValues)));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant