ModalBarrier's gesture recognizer does not call invokeCallback#125386
ModalBarrier's gesture recognizer does not call invokeCallback#125386auto-submit[bot] merged 7 commits intoflutter:masterfrom
ModalBarrier's gesture recognizer does not call invokeCallback#125386Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
ModalBarrier has imperfect error messageModalBarrier's gesture recognizer does not call invokeCallback
| expect(tester.binding.takeException(), 'TODO_assert_the_error_context'); | ||
| }); | ||
|
|
||
|
|
There was a problem hiding this comment.
(Will do it in batch with my other PRs)
| )); | ||
| await tester.tap(find.byKey(barrierKey)); | ||
| await tester.pump(); | ||
| expect(tester.binding.takeException(), 'TODO_assert_the_error_context'); |
There was a problem hiding this comment.
I am not sure how to test this: This PR fixes the FlutterErrorDetails.context field, but here, takeException does not expose the FlutterErrorDetails (but only FlutterErrorDetails.exception).
Thus, I wonder how should I test it? e.g.
Method 1: Add a new method in parallel to takeException and expose the whole FlutterErrorDetails.
Method 2: Do not test it (I think this is bad)
There was a problem hiding this comment.
Wouldn't the toString of the exception contain something that was added by invokeCallback that you can look for in the test here?
There was a problem hiding this comment.
Hmm,
print(tester.binding.takeException().toString());
yields:
Exception: deliberate
So I guess it does not add anything.
There was a problem hiding this comment.
The proposal is that, in addition to the existing code:
dynamic takeException() {
assert(inTest);
final dynamic result = _pendingExceptionDetails?.exception;
_pendingExceptionDetails = null;
return result;
}
FlutterErrorDetails? _pendingExceptionDetails;We can add:
FlutterErrorDetails takeErrorDetails() {
assert(inTest);
final FlutterErrorDetails result = _pendingExceptionDetails; // <-- NOTE
_pendingExceptionDetails = null;
return result;
}There was a problem hiding this comment.
I can create a separate PR for that (given that Flutter guide seems to prefer one PR to do one thing)
There was a problem hiding this comment.
This shouldn't need new API. Can you get to the error with a custom onError handler? https://master-api.flutter.dev/flutter/foundation/FlutterError/onError.html
There was a problem hiding this comment.
I will try that. Thanks!
|
Looks like a lot of tests are failing. Can you take a look? |
Co-authored-by: Michael Goderbauer <[email protected]>
|
Sure! See my comments above |
|
@fzyzcjy Can you rebase to tip of tree to make the google testing check happy? |
|
Sure! |
Close #125385 - please see explanations there :)
If you think this PR looks OK, I will polish it (e.g. copy-and-paste-and-modify the tests shown in #125385 into here and make CI pass)
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.