-
Notifications
You must be signed in to change notification settings - Fork 6k
Fix multiple issues affecting HooksTest.HooksUnitTests #36741
Fix multiple issues affecting HooksTest.HooksUnitTests #36741
Conversation
jason-simmons
commented
Oct 13, 2022
- Return errors from the HandlePlatformMessage call in PlatformConfigurationNativeApi::SendPortPlatformMessage
- Clear the PlatformDispatcher onError handler installed by one of the tests. This handler can cause an infinite loop if a later test throws an exception.
- Wait for completion of async tests.
gaaclarke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the change to the runtime logic should have a test. Let me know if you need help adding the test.
lib/ui/fixtures/ui_test.dart
Outdated
| void hooksTests() { | ||
| void test(String name, VoidCallback testFunction) { | ||
| void hooksTests() async { | ||
| Future<void> test(String name, FutureOr<void> testFunction()) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I didn't know Dart could do this, is this idiomatic as opposed to having FutureOr<void> Function() testFunction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the Function syntax is now preferred
(see https://dart.dev/guides/language/effective-dart/design#prefer-using-function-type-syntax-for-parameters)
| HandlePlatformMessage(dart_state, name, data_handle, response); | ||
|
|
||
| return Dart_Null(); | ||
| return HandlePlatformMessage(dart_state, name, data_handle, response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already tested by https://github.com/flutter/engine/blob/main/lib/ui/fixtures/ui_test.dart#L929
That test was actually failing. But the failure was not being reported properly due to the other issues addressed by this PR.
* Return errors from the HandlePlatformMessage call in PlatformConfigurationNativeApi::SendPortPlatformMessage * Clear the PlatformDispatcher onError handler installed by one of the tests. This handler can cause an infinite loop if a later test throws an exception. * Wait for completion of async tests.
130df1c to
917338e
Compare