Repository navigation
Add WidgetTester.runAsync() - #17029
Conversation
There was a problem hiding this comment.
this presumably wants a guard?
There was a problem hiding this comment.
i guess the assert serves that purpose. we should add a message to it to make that clearer.
This will allow callers to run real async code within a widget test. #16859
| return new Future<Null>.value(testBodyResult); | ||
|
|
||
| return new Future<Null>.microtask(() async { | ||
| while (_pendingAsyncTasks != null) { |
There was a problem hiding this comment.
you need to flush microtasks at least once even when it's null
There was a problem hiding this comment.
oh i see, you do that in the async barrier.
Yeah, that makes sense.
There was a problem hiding this comment.
oh i see, you do that in the async barrier.
I thought so too, which is why I didn't put it in here. But your first instinct was right - we do need it, because the work done in runAsync() may have just completed, meaning _pendingAsyncTasks will be null, yet the microtask queue is still backed up.
|
approach sgtm |
| /// | ||
| /// If [callback] completes with an error, the error will be caught by the | ||
| /// Flutter framework and made available via [takeException], and the future | ||
| /// returned by this method will complete will `null`. |
| throw new test_package.TestFailure( | ||
| 'runAsync() was called, then before its future completed, it\n' | ||
| 'was called again. You must wait for the first returned future\n' | ||
| 'to complete before calling runAsync() again.' |
There was a problem hiding this comment.
We don't normally break lines for FlutterErrors (see https://master-docs-flutter-io.firebaseapp.com/flutter/foundation/FlutterError/message.html for the conventions), but I don't know what the conventions are for TestFailure.
| return Zone.root.run(() { | ||
| _pendingAsyncTasks = new Completer<void>(); | ||
| return callback().then<T>((T value) { | ||
| return value; |
There was a problem hiding this comment.
this .then seems... trivial? Is it actually needed?
There was a problem hiding this comment.
It's totally not needed, but I thought it improved readability. I'm not married to it though - lemme know if you want me to remove it.
There was a problem hiding this comment.
I'm not a huge fan of it, without any commentary, it would make me wonder why it's there and whether I'm missing something.
There was a problem hiding this comment.
Yeah I'd just remove it. If there's a worry about code clarity, add a comment.
There was a problem hiding this comment.
I removed it before merging.
|
|
||
| @override | ||
| Future<T> runAsync<T>(Future<T> callback()) async { | ||
| try { |
There was a problem hiding this comment.
this should also prevent reentrancy
There was a problem hiding this comment.
Good catch - will do
| /// If [callback] completes with an error, the error will be caught by the | ||
| /// Flutter framework and made available via [takeException], and the future | ||
| /// returned by this method will complete will `null`. | ||
| Future<T> runAsync<T>(Future<T> callback()); |
There was a problem hiding this comment.
this should document that, for sanity, reentrancy isn't allowed
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
Thanks for the detailed comments! LGTM
This will allow callers to run real async code within a widget test. flutter#16859
4 participants
This will allow callers to run real async code within a widget test.
#16859