Skip to content

Commit

Permalink
Fix flaky Windows exit unit test, remove error messages (#40945)
Browse files Browse the repository at this point in the history 
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 flutter/flutter#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].

<!-- Links -->
[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
  • Loading branch information
@yaakovschectman
yaakovschectman authored Apr 7, 2023
1 parent 0a81b0c commit 1f5433d
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 81 deletions.
18 changes: 0 additions & 18 deletions shell/platform/windows/fixtures/main.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ByteData?> exited = Completer<ByteData?>();
final String jsonString = json.encode(<String, dynamic>{
'method': 'System.exitApplication',
'args': <String, dynamic>{
'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')
Expand Down
105 changes: 42 additions & 63 deletions shell/platform/windows/flutter_windows_engine_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -639,13 +639,16 @@ TEST_F(FlutterWindowsEngineTest, TestExit) {
FlutterWindowsEngineBuilder builder{GetContext()};
builder.SetDartEntrypoint("exitTestExit");
bool finished = false;
bool did_call = false;

std::unique_ptr<FlutterWindowsEngine> engine = builder.Build();
auto window_binding_handler =
std::make_unique<::testing::NiceMock<MockWindowBindingHandler>>();
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<MockWindowsLifecycleManager>(engine.get());
auto handler = std::make_unique<MockWindowsLifecycleManager>(engine);
ON_CALL(*handler, Quit)
.WillByDefault(
[&finished](std::optional<HWND> hwnd, std::optional<WPARAM> wparam,
Expand All @@ -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<BinaryMessengerImpl>(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<uint8_t*>(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) {
Expand All @@ -685,11 +676,15 @@ TEST_F(FlutterWindowsEngineTest, TestExitCancel) {
bool finished = false;
bool did_call = false;

std::unique_ptr<FlutterWindowsEngine> engine = builder.Build();
auto window_binding_handler =
std::make_unique<::testing::NiceMock<MockWindowBindingHandler>>();
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<MockWindowsLifecycleManager>(engine.get());
auto handler = std::make_unique<MockWindowsLifecycleManager>(engine);
ON_CALL(*handler, Quit)
.WillByDefault([&finished](std::optional<HWND> hwnd,
std::optional<WPARAM> wparam,
Expand Down Expand Up @@ -729,39 +724,36 @@ 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<FlutterWindowsEngine> engine = builder.Build();
auto window_binding_handler =
std::make_unique<::testing::NiceMock<MockWindowBindingHandler>>();
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<MockWindowsLifecycleManager>(engine.get());
ON_CALL(*handler, IsLastWindowOfProcess).WillByDefault([]() { return true; });
ON_CALL(*handler, Quit)
.WillByDefault([&handler](std::optional<HWND> hwnd,
std::optional<WPARAM> wparam,
std::optional<LPARAM> lparam, UINT exit_code) {
handler->WindowsLifecycleManager::Quit(hwnd, wparam, lparam, exit_code);
});
ON_CALL(*handler, DispatchMessage)
auto handler = std::make_unique<MockWindowsLifecycleManager>(engine);
auto& handler_obj = *handler;
ON_CALL(handler_obj, IsLastWindowOfProcess).WillByDefault([]() {
return true;
});
ON_CALL(handler_obj, Quit)
.WillByDefault(
[&handler_obj](std::optional<HWND> hwnd, std::optional<WPARAM> wparam,
std::optional<LPARAM> 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(
hwnd, msg, wparam, lparam);
});
modifier.SetLifecycleManager(std::move(handler));

auto binary_messenger =
std::make_unique<BinaryMessengerImpl>(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<uint8_t*>(response), 0);
});

engine->Run();

// This delegate will be registered after the lifecycle manager, so it will be
Expand All @@ -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<FlutterWindowsEngine> engine = builder.Build();
auto window_binding_handler =
std::make_unique<::testing::NiceMock<MockWindowBindingHandler>>();
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<MockWindowsLifecycleManager>(engine.get());
auto handler = std::make_unique<MockWindowsLifecycleManager>(engine);
ON_CALL(*handler, IsLastWindowOfProcess).WillByDefault([&finished]() {
finished = true;
return false;
Expand All @@ -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<BinaryMessengerImpl>(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<uint8_t*>(response), 0);
});

engine->Run();

engine->window_proc_delegate_manager()->OnTopLevelWindowProc(0, WM_CLOSE, 0,
Expand All @@ -831,8 +812,6 @@ TEST_F(FlutterWindowsEngineTest, TestExitCloseMultiWindow) {
while (!finished) {
engine->task_runner()->ProcessTasks();
}

EXPECT_FALSE(did_call);
}

} // namespace testing
Expand Down

0 comments on commit 1f5433d

Please sign in to comment.