Repository navigation
Reject asynchronous matchers inside isNot and anyOf - #2792
Conversation
Asynchronous matchers report a pending result as a synchronous match and fail the test later. `isNot` inverts that placeholder into an immediate failure, and `anyOf` short-circuits on it, so neither operator can combine asynchronous results correctly. For example `anyOf([completes, throwsA(x)])` passes synchronously no matter how the future completes, and the test may fail later even when one of the matchers would have matched. `isNot` and `anyOf` now match their operands in a zone that marks asynchronous matchers as disallowed. `AsyncMatcher.matches` and `doesNotComplete` throw an `ArgumentError` in that zone before starting any asynchronous work, which also catches asynchronous matchers nested under other operators. `expect` and `expectLater` clear the restriction, so expectations made in callbacks run by an operator are unaffected. `AsyncMatcher` subclasses can override `mayMatchSynchronously` to defer the check until `matchAsync` returns a `Future`. The `throws` matchers opt in, so common patterns like `isNot(throwsA(...))` with a synchronous callback keep working, while a `Future` or a callback returning a `Future` is still rejected.
|
I found only 1 misleading test pattern that this catches copy/pasted 3 times in 1 repo. There may be more problems with this in external code which I haven't tried to survey. I think it's worth landing because there's really no reason to use the patterns this blocks. |
jakemac53
left a comment
jakemac53
left a comment
There was a problem hiding this comment.
Is this breaking, or are we fairly certain that all subtypes of AsyncMatcher extend the base type?
Since this would break those who are not, should we mark it as base now?
We've never exposed it through a public library. I think there are some
That's a good idea, it's (practically) equally breaking and would protect us in the future. |
|
Ah, I guess not quite equally breaking, adding |
|
There are uses of In practice the design of this class only makes sense for use as a base class, and I think it's OK to rely on existing code doing that. There's nothing to be gained with If more changes were planned I'd consider going through the migration to add |
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
|
Unfortunate about the transitive base/final requirement, but makes sense |
…18795) The `testExceptionHandling` helpers in the firebase_auth and firebase_performance platform interface tests have no callers. They also contain an assertion that doesn't do what it appears to: await expectLater( () async => testMethod(), anyOf([ completes, if (type == 'PLATFORM' || type == 'EXCEPTION') throwsA(isA<FirebaseException>()), ]), ); `completes` only matches a `Future`, and the value here is a closure, so that branch can never match. When `type` is `'PLATFORM'` or `'EXCEPTION'` the `anyOf` behaves like `throwsA(...)` alone, and for any other `type` the assertion always fails. In addition, `anyOf` can't combine asynchronous matchers: it treats the pending result from `throwsA` as a match, and `expectLater` doesn't wait for the real result. An upcoming change to `package:matcher` makes `anyOf` and `isNot` throw an `ArgumentError` when they are used with asynchronous matchers like these, which would break these helpers if they were ever called again: dart-lang/test#2792 Remove the helpers and the imports they used.
Revisions updated by `dart tools/rev_sdk_deps.dart`. native is rolled separately in https://dart-review.googlesource.com/c/sdk/+/563524. http (https://github.com/dart-lang/http/compare/23f8574..7d7aae2): 7d7aae2 Mon Oct 12 20:39:25 2026 -0700 Kevin Moore Clean up dependencies and expand Node and Wasm test coverage (dart-lang/http#1903) ab986ad Mon Oct 12 18:59:24 2026 -0700 Kevin Moore fix(http_parser): fix chunked size overflow and MediaType.toString quoting (dart-lang/http#2001) cb8113b Mon Oct 12 17:35:26 2026 -0700 Kevin Moore docs(http2): move post-3.1.0 security entries to 3.2.0-wip (dart-lang/http#2022) test (https://github.com/dart-lang/test/compare/30acb69..9bf69a8): 9bf69a88 Tue Oct 13 14:33:18 2026 -0700 Nate Bosch Reject asynchronous matchers inside isNot and anyOf (dart-lang/test#2792) tools (https://github.com/dart-lang/tools/compare/2b54c71..350bde5): 350bde54f Tue Oct 13 08:22:26 2026 -0700 Kevin Moore chore(coverage): prepare for v1.16.0 release (dart-lang/tools#2643) a55965844 Tue Oct 13 13:29:52 2026 +0200 Sigurd Meldgaard [markdown] Fix quadratic parsing of consecutive link reference definitions (dart-lang/tools#2662) [email protected] TEST=ci Change-Id: I1065854cfbaf71ebb7a097d89f28902394a80c2e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/563365 Commit-Queue: Nate Bosch <[email protected]> Reviewed-by: Brian Quinlan <[email protected]>
Closes #1637
Asynchronous matchers report a pending result as a synchronous match and
fail the test later.
isNotinverts that placeholder into an immediatefailure, and
anyOfshort-circuits on it, so neither operator can combineasynchronous results correctly. For example
anyOf([completes, throwsA(x)])passes synchronously no matter how the future completes, and the test may
fail later even when one of the matchers would have matched.
isNotandanyOfnow match their operands in a zone that marksasynchronous matchers as disallowed.
AsyncMatcher.matchesanddoesNotCompletethrow anArgumentErrorin that zone before starting anyasynchronous work, which also catches asynchronous matchers nested under
other operators.
expectandexpectLaterclear the restriction, soexpectations made in callbacks run by an operator are unaffected.
AsyncMatchersubclasses can overridemayMatchSynchronouslyto defer thecheck until
matchAsyncreturns aFuture. Thethrowsmatchers opt in, socommon patterns like
isNot(throwsA(...))with a synchronous callback keepworking, while a
Futureor a callback returning aFutureis stillrejected.