From 1f5433d4e4ae0a1a589a70655f371dd5f218e4a7 Mon Sep 17 00:00:00 2001 From: yaakovschectman <109111084+yaakovschectman@users.noreply.github.com> Date: Fri, 7 Apr 2023 17:11:15 -0400 Subject: [PATCH] Fix flaky Windows exit unit test, remove error messages (#40945) Shutdown is async, so we can not guarantee the order of execution and, by extension, ignorance of execution used as an auxiliary part of a unit test. Also, mock the windows view/binding handler for the engine in order to avoid error messages. Addresses https://github.com/flutter/flutter/issues/124162 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --- shell/platform/windows/fixtures/main.dart | 18 --- .../flutter_windows_engine_unittests.cc | 105 +++++++----------- 2 files changed, 42 insertions(+), 81 deletions(-) diff --git a/shell/platform/windows/fixtures/main.dart b/shell/platform/windows/fixtures/main.dart index c9794f68d89ab..dd88b7eab2f3a 100644 --- a/shell/platform/windows/fixtures/main.dart +++ b/shell/platform/windows/fixtures/main.dart @@ -97,24 +97,6 @@ void exitTestExit() async { closed.complete(data); }); await closed.future; - - // From here down, nothing should be called, because the application will have already been closed. - final Completer exited = Completer(); - final String jsonString = json.encode({ - 'method': 'System.exitApplication', - 'args': { - 'type': 'required', 'exitCode': 0 - } - }); - ui.PlatformDispatcher.instance.sendPlatformMessage( - 'flutter/platform', - ByteData.sublistView( - Uint8List.fromList(utf8.encode(jsonString)) - ), - (ByteData? reply) { - exited.complete(reply); - }); - await exited.future; } @pragma('vm:entry-point') diff --git a/shell/platform/windows/flutter_windows_engine_unittests.cc b/shell/platform/windows/flutter_windows_engine_unittests.cc index f21e3aa00df64..161181cd11fda 100644 --- a/shell/platform/windows/flutter_windows_engine_unittests.cc +++ b/shell/platform/windows/flutter_windows_engine_unittests.cc @@ -639,13 +639,16 @@ TEST_F(FlutterWindowsEngineTest, TestExit) { FlutterWindowsEngineBuilder builder{GetContext()}; builder.SetDartEntrypoint("exitTestExit"); bool finished = false; - bool did_call = false; - std::unique_ptr engine = builder.Build(); + auto window_binding_handler = + std::make_unique<::testing::NiceMock>(); + MockFlutterWindowsView view(std::move(window_binding_handler)); + view.SetEngine(builder.Build()); + FlutterWindowsEngine* engine = view.GetEngine(); - EngineModifier modifier(engine.get()); + EngineModifier modifier(engine); modifier.embedder_api().RunsAOTCompiledDartCode = []() { return false; }; - auto handler = std::make_unique(engine.get()); + auto handler = std::make_unique(engine); ON_CALL(*handler, Quit) .WillByDefault( [&finished](std::optional hwnd, std::optional wparam, @@ -655,28 +658,16 @@ TEST_F(FlutterWindowsEngineTest, TestExit) { EXPECT_CALL(*handler, Quit).Times(1); modifier.SetLifecycleManager(std::move(handler)); - auto binary_messenger = - std::make_unique(engine->messenger()); - // If this callback is triggered, the test fails. The exit procedure should be - // called without checking with the framework. - binary_messenger->SetMessageHandler( - "flutter/platform", [&did_call](const uint8_t* message, - size_t message_size, BinaryReply reply) { - did_call = true; - char response[] = ""; - reply(reinterpret_cast(response), 0); - }); - engine->Run(); engine->window_proc_delegate_manager()->OnTopLevelWindowProc(0, WM_CLOSE, 0, 0); + // The test will only succeed when this while loop exits. Otherwise it will + // timeout. while (!finished) { engine->task_runner()->ProcessTasks(); } - - EXPECT_FALSE(did_call); } TEST_F(FlutterWindowsEngineTest, TestExitCancel) { @@ -685,11 +676,15 @@ TEST_F(FlutterWindowsEngineTest, TestExitCancel) { bool finished = false; bool did_call = false; - std::unique_ptr engine = builder.Build(); + auto window_binding_handler = + std::make_unique<::testing::NiceMock>(); + MockFlutterWindowsView view(std::move(window_binding_handler)); + view.SetEngine(builder.Build()); + FlutterWindowsEngine* engine = view.GetEngine(); - EngineModifier modifier(engine.get()); + EngineModifier modifier(engine); modifier.embedder_api().RunsAOTCompiledDartCode = []() { return false; }; - auto handler = std::make_unique(engine.get()); + auto handler = std::make_unique(engine); ON_CALL(*handler, Quit) .WillByDefault([&finished](std::optional hwnd, std::optional wparam, @@ -729,22 +724,29 @@ TEST_F(FlutterWindowsEngineTest, TestExitCancel) { TEST_F(FlutterWindowsEngineTest, TestExitSecondCloseMessage) { FlutterWindowsEngineBuilder builder{GetContext()}; builder.SetDartEntrypoint("exitTestExit"); - bool did_call = false; bool second_close = false; - std::unique_ptr engine = builder.Build(); + auto window_binding_handler = + std::make_unique<::testing::NiceMock>(); + MockFlutterWindowsView view(std::move(window_binding_handler)); + view.SetEngine(builder.Build()); + FlutterWindowsEngine* engine = view.GetEngine(); - EngineModifier modifier(engine.get()); + EngineModifier modifier(engine); modifier.embedder_api().RunsAOTCompiledDartCode = []() { return false; }; - auto handler = std::make_unique(engine.get()); - ON_CALL(*handler, IsLastWindowOfProcess).WillByDefault([]() { return true; }); - ON_CALL(*handler, Quit) - .WillByDefault([&handler](std::optional hwnd, - std::optional wparam, - std::optional lparam, UINT exit_code) { - handler->WindowsLifecycleManager::Quit(hwnd, wparam, lparam, exit_code); - }); - ON_CALL(*handler, DispatchMessage) + auto handler = std::make_unique(engine); + auto& handler_obj = *handler; + ON_CALL(handler_obj, IsLastWindowOfProcess).WillByDefault([]() { + return true; + }); + ON_CALL(handler_obj, Quit) + .WillByDefault( + [&handler_obj](std::optional hwnd, std::optional wparam, + std::optional lparam, UINT exit_code) { + handler_obj.WindowsLifecycleManager::Quit(hwnd, wparam, lparam, + exit_code); + }); + ON_CALL(handler_obj, DispatchMessage) .WillByDefault( [&engine](HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) { engine->window_proc_delegate_manager()->OnTopLevelWindowProc( @@ -752,16 +754,6 @@ TEST_F(FlutterWindowsEngineTest, TestExitSecondCloseMessage) { }); modifier.SetLifecycleManager(std::move(handler)); - auto binary_messenger = - std::make_unique(engine->messenger()); - binary_messenger->SetMessageHandler( - "flutter/platform", [&did_call](const uint8_t* message, - size_t message_size, BinaryReply reply) { - did_call = true; - char response[] = ""; - reply(reinterpret_cast(response), 0); - }); - engine->Run(); // This delegate will be registered after the lifecycle manager, so it will be @@ -788,21 +780,22 @@ TEST_F(FlutterWindowsEngineTest, TestExitSecondCloseMessage) { while (!second_close) { engine->task_runner()->ProcessTasks(); } - - EXPECT_FALSE(did_call); } TEST_F(FlutterWindowsEngineTest, TestExitCloseMultiWindow) { FlutterWindowsEngineBuilder builder{GetContext()}; builder.SetDartEntrypoint("exitTestExit"); bool finished = false; - bool did_call = false; - std::unique_ptr engine = builder.Build(); + auto window_binding_handler = + std::make_unique<::testing::NiceMock>(); + MockFlutterWindowsView view(std::move(window_binding_handler)); + view.SetEngine(builder.Build()); + FlutterWindowsEngine* engine = view.GetEngine(); - EngineModifier modifier(engine.get()); + EngineModifier modifier(engine); modifier.embedder_api().RunsAOTCompiledDartCode = []() { return false; }; - auto handler = std::make_unique(engine.get()); + auto handler = std::make_unique(engine); ON_CALL(*handler, IsLastWindowOfProcess).WillByDefault([&finished]() { finished = true; return false; @@ -811,18 +804,6 @@ TEST_F(FlutterWindowsEngineTest, TestExitCloseMultiWindow) { EXPECT_CALL(*handler, Quit).Times(0); modifier.SetLifecycleManager(std::move(handler)); - auto binary_messenger = - std::make_unique(engine->messenger()); - // If this callback is triggered, the test fails. The exit procedure should be - // called without checking with the framework. - binary_messenger->SetMessageHandler( - "flutter/platform", [&did_call](const uint8_t* message, - size_t message_size, BinaryReply reply) { - did_call = true; - char response[] = ""; - reply(reinterpret_cast(response), 0); - }); - engine->Run(); engine->window_proc_delegate_manager()->OnTopLevelWindowProc(0, WM_CLOSE, 0, @@ -831,8 +812,6 @@ TEST_F(FlutterWindowsEngineTest, TestExitCloseMultiWindow) { while (!finished) { engine->task_runner()->ProcessTasks(); } - - EXPECT_FALSE(did_call); } } // namespace testing