-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[url_launcher] Add native unit tests for Windows #4156
Changes from all commits
6b6d723
99b2ab1
bc94f32
e239b3d
ec3b3e2
a6a1e8b
ce8bb54
eb432df
8542f52
3032865
cef2f07
c5c0a8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
| #include "system_apis.h" | ||
|
|
||
| #include <windows.h> | ||
|
|
||
| namespace url_launcher_plugin { | ||
|
|
||
| SystemApis::SystemApis() {} | ||
|
|
||
| SystemApis::~SystemApis() {} | ||
|
|
||
| SystemApisImpl::SystemApisImpl() {} | ||
|
|
||
| SystemApisImpl::~SystemApisImpl() {} | ||
|
|
||
| LSTATUS SystemApisImpl::RegCloseKey(HKEY key) { return ::RegCloseKey(key); } | ||
|
|
||
| LSTATUS SystemApisImpl::RegOpenKeyExW(HKEY key, LPCWSTR sub_key, DWORD options, | ||
| REGSAM desired, PHKEY result) { | ||
| return ::RegOpenKeyExW(key, sub_key, options, desired, result); | ||
| } | ||
|
|
||
| LSTATUS SystemApisImpl::RegQueryValueExW(HKEY key, LPCWSTR value_name, | ||
| LPDWORD type, LPBYTE data, | ||
| LPDWORD data_size) { | ||
| return ::RegQueryValueExW(key, value_name, nullptr, type, data, data_size); | ||
| } | ||
|
|
||
| HINSTANCE SystemApisImpl::ShellExecuteW(HWND hwnd, LPCWSTR operation, | ||
| LPCWSTR file, LPCWSTR parameters, | ||
| LPCWSTR directory, int show_flags) { | ||
| return ::ShellExecuteW(hwnd, operation, file, parameters, directory, | ||
| show_flags); | ||
| } | ||
|
|
||
| } // namespace url_launcher_plugin |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
| #include <windows.h> | ||
|
|
||
| namespace url_launcher_plugin { | ||
|
|
||
| // An interface wrapping system APIs used by the plugin, for mocking. | ||
| class SystemApis { | ||
| public: | ||
| SystemApis(); | ||
| virtual ~SystemApis(); | ||
|
|
||
| // Disallow copy and move. | ||
| SystemApis(const SystemApis&) = delete; | ||
| SystemApis& operator=(const SystemApis&) = delete; | ||
|
|
||
| // Wrapper for RegCloseKey. | ||
| virtual LSTATUS RegCloseKey(HKEY key) = 0; | ||
|
|
||
| // Wrapper for RegQueryValueEx. | ||
| virtual LSTATUS RegQueryValueExW(HKEY key, LPCWSTR value_name, LPDWORD type, | ||
| LPBYTE data, LPDWORD data_size) = 0; | ||
|
|
||
| // Wrapper for RegOpenKeyEx. | ||
| virtual LSTATUS RegOpenKeyExW(HKEY key, LPCWSTR sub_key, DWORD options, | ||
| REGSAM desired, PHKEY result) = 0; | ||
|
|
||
| // Wrapper for ShellExecute. | ||
| virtual HINSTANCE ShellExecuteW(HWND hwnd, LPCWSTR operation, LPCWSTR file, | ||
| LPCWSTR parameters, LPCWSTR directory, | ||
| int show_flags) = 0; | ||
| }; | ||
|
|
||
| // Implementation of SystemApis using the Win32 APIs. | ||
| class SystemApisImpl : public SystemApis { | ||
| public: | ||
| SystemApisImpl(); | ||
| virtual ~SystemApisImpl(); | ||
|
|
||
| // Disallow copy and move. | ||
| SystemApisImpl(const SystemApisImpl&) = delete; | ||
| SystemApisImpl& operator=(const SystemApisImpl&) = delete; | ||
|
|
||
| // SystemApis Implementation: | ||
| virtual LSTATUS RegCloseKey(HKEY key); | ||
| virtual LSTATUS RegOpenKeyExW(HKEY key, LPCWSTR sub_key, DWORD options, | ||
| REGSAM desired, PHKEY result); | ||
| virtual LSTATUS RegQueryValueExW(HKEY key, LPCWSTR value_name, LPDWORD type, | ||
| LPBYTE data, LPDWORD data_size); | ||
| virtual HINSTANCE ShellExecuteW(HWND hwnd, LPCWSTR operation, LPCWSTR file, | ||
| LPCWSTR parameters, LPCWSTR directory, | ||
| int show_flags); | ||
| }; | ||
|
|
||
| } // namespace url_launcher_plugin |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,162 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
| #include <flutter/method_call.h> | ||
| #include <flutter/method_result_functions.h> | ||
| #include <flutter/standard_method_codec.h> | ||
| #include <gmock/gmock.h> | ||
| #include <gtest/gtest.h> | ||
| #include <windows.h> | ||
|
|
||
| #include <memory> | ||
| #include <string> | ||
|
|
||
| #include "url_launcher_plugin.h" | ||
|
|
||
| namespace url_launcher_plugin { | ||
| namespace test { | ||
|
|
||
| namespace { | ||
|
|
||
| using flutter::EncodableMap; | ||
| using flutter::EncodableValue; | ||
| using ::testing::DoAll; | ||
| using ::testing::Pointee; | ||
| using ::testing::Return; | ||
| using ::testing::SetArgPointee; | ||
|
|
||
| class MockSystemApis : public SystemApis { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpicky and up to you -- I wonder if a simple fake here that returns hardcoded/deterministic values and maybe a bool for open/close state would be enough, and make the tests themselves a bit more readable.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The results can't be hard-coded, because I wanted to do full failure path testing, which means I would need a variable and a setter for the return value of each method. By the time I did that, the helper would be a lot more complicated than the mock declaration, so I came down on the side of gmock. (This also lets me check a few arguments to the calls trivially, which is a nice bit of added test robustness.) |
||
| public: | ||
| MOCK_METHOD(LSTATUS, RegCloseKey, (HKEY key), (override)); | ||
| MOCK_METHOD(LSTATUS, RegQueryValueExW, | ||
| (HKEY key, LPCWSTR value_name, LPDWORD type, LPBYTE data, | ||
| LPDWORD data_size), | ||
| (override)); | ||
| MOCK_METHOD(LSTATUS, RegOpenKeyExW, | ||
| (HKEY key, LPCWSTR sub_key, DWORD options, REGSAM desired, | ||
| PHKEY result), | ||
| (override)); | ||
| MOCK_METHOD(HINSTANCE, ShellExecuteW, | ||
| (HWND hwnd, LPCWSTR operation, LPCWSTR file, LPCWSTR parameters, | ||
| LPCWSTR directory, int show_flags), | ||
| (override)); | ||
| }; | ||
|
|
||
| class MockMethodResult : public flutter::MethodResult<> { | ||
| public: | ||
| MOCK_METHOD(void, SuccessInternal, (const EncodableValue* result), | ||
| (override)); | ||
| MOCK_METHOD(void, ErrorInternal, | ||
| (const std::string& error_code, const std::string& error_message, | ||
| const EncodableValue* details), | ||
| (override)); | ||
| MOCK_METHOD(void, NotImplementedInternal, (), (override)); | ||
| }; | ||
|
|
||
| std::unique_ptr<EncodableValue> CreateArgumentsWithUrl(const std::string& url) { | ||
| EncodableMap args = { | ||
| {EncodableValue("url"), EncodableValue(url)}, | ||
| }; | ||
| return std::make_unique<EncodableValue>(args); | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| TEST(UrlLauncherPlugin, CanLaunchSuccessTrue) { | ||
| std::unique_ptr<MockSystemApis> system = std::make_unique<MockSystemApis>(); | ||
| std::unique_ptr<MockMethodResult> result = | ||
| std::make_unique<MockMethodResult>(); | ||
|
|
||
| // Return success values from the registery commands. | ||
| HKEY fake_key = reinterpret_cast<HKEY>(1); | ||
| EXPECT_CALL(*system, RegOpenKeyExW) | ||
| .WillOnce(DoAll(SetArgPointee<4>(fake_key), Return(ERROR_SUCCESS))); | ||
| EXPECT_CALL(*system, RegQueryValueExW).WillOnce(Return(ERROR_SUCCESS)); | ||
| EXPECT_CALL(*system, RegCloseKey(fake_key)).WillOnce(Return(ERROR_SUCCESS)); | ||
| // Expect a success response. | ||
| EXPECT_CALL(*result, SuccessInternal(Pointee(EncodableValue(true)))); | ||
|
|
||
| UrlLauncherPlugin plugin(std::move(system)); | ||
| plugin.HandleMethodCall( | ||
| flutter::MethodCall("canLaunch", | ||
| CreateArgumentsWithUrl("https://some.url.com")), | ||
| std::move(result)); | ||
| } | ||
|
|
||
| TEST(UrlLauncherPlugin, CanLaunchQueryFailure) { | ||
| std::unique_ptr<MockSystemApis> system = std::make_unique<MockSystemApis>(); | ||
| std::unique_ptr<MockMethodResult> result = | ||
| std::make_unique<MockMethodResult>(); | ||
|
|
||
| // Return success values from the registery commands, except for the query, | ||
| // to simulate a scheme that is in the registry, but has no URL handler. | ||
| HKEY fake_key = reinterpret_cast<HKEY>(1); | ||
| EXPECT_CALL(*system, RegOpenKeyExW) | ||
| .WillOnce(DoAll(SetArgPointee<4>(fake_key), Return(ERROR_SUCCESS))); | ||
| EXPECT_CALL(*system, RegQueryValueExW).WillOnce(Return(ERROR_FILE_NOT_FOUND)); | ||
| EXPECT_CALL(*system, RegCloseKey(fake_key)).WillOnce(Return(ERROR_SUCCESS)); | ||
| // Expect a success response. | ||
| EXPECT_CALL(*result, SuccessInternal(Pointee(EncodableValue(false)))); | ||
|
|
||
| UrlLauncherPlugin plugin(std::move(system)); | ||
| plugin.HandleMethodCall( | ||
| flutter::MethodCall("canLaunch", | ||
| CreateArgumentsWithUrl("https://some.url.com")), | ||
| std::move(result)); | ||
| } | ||
|
|
||
| TEST(UrlLauncherPlugin, CanLaunchHandlesOpenFailure) { | ||
| std::unique_ptr<MockSystemApis> system = std::make_unique<MockSystemApis>(); | ||
| std::unique_ptr<MockMethodResult> result = | ||
| std::make_unique<MockMethodResult>(); | ||
|
|
||
| // Return failure for opening. | ||
| EXPECT_CALL(*system, RegOpenKeyExW).WillOnce(Return(ERROR_BAD_PATHNAME)); | ||
| // Expect a success response. | ||
| EXPECT_CALL(*result, SuccessInternal(Pointee(EncodableValue(false)))); | ||
|
|
||
| UrlLauncherPlugin plugin(std::move(system)); | ||
| plugin.HandleMethodCall( | ||
| flutter::MethodCall("canLaunch", | ||
| CreateArgumentsWithUrl("https://some.url.com")), | ||
| std::move(result)); | ||
| } | ||
|
|
||
| TEST(UrlLauncherPlugin, LaunchSuccess) { | ||
| std::unique_ptr<MockSystemApis> system = std::make_unique<MockSystemApis>(); | ||
| std::unique_ptr<MockMethodResult> result = | ||
| std::make_unique<MockMethodResult>(); | ||
|
|
||
| // Return a success value (>32) from launching. | ||
| EXPECT_CALL(*system, ShellExecuteW) | ||
| .WillOnce(Return(reinterpret_cast<HINSTANCE>(33))); | ||
| // Expect a success response. | ||
| EXPECT_CALL(*result, SuccessInternal(Pointee(EncodableValue(true)))); | ||
|
|
||
| UrlLauncherPlugin plugin(std::move(system)); | ||
| plugin.HandleMethodCall( | ||
| flutter::MethodCall("launch", | ||
| CreateArgumentsWithUrl("https://some.url.com")), | ||
| std::move(result)); | ||
| } | ||
|
|
||
| TEST(UrlLauncherPlugin, LaunchReportsFailure) { | ||
| std::unique_ptr<MockSystemApis> system = std::make_unique<MockSystemApis>(); | ||
| std::unique_ptr<MockMethodResult> result = | ||
| std::make_unique<MockMethodResult>(); | ||
|
|
||
| // Return a faile value (<=32) from launching. | ||
| EXPECT_CALL(*system, ShellExecuteW) | ||
| .WillOnce(Return(reinterpret_cast<HINSTANCE>(32))); | ||
| // Expect an error response. | ||
| EXPECT_CALL(*result, ErrorInternal); | ||
|
|
||
| UrlLauncherPlugin plugin(std::move(system)); | ||
| plugin.HandleMethodCall( | ||
| flutter::MethodCall("launch", | ||
| CreateArgumentsWithUrl("https://some.url.com")), | ||
| std::move(result)); | ||
| } | ||
|
|
||
| } // namespace test | ||
| } // namespace url_launcher_plugin | ||
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 not great, but I don't have a solution I like better so far. The issue is that the
fluttertool generates both an#includepath and a call to the C API based on the value ofpluginClass, so if it stays as FooPlugin, then the public header has to be calledfoo_plugin.h. But the plugin class's file should match its name, so should be calledfoo_plugin.h/cpp, and it's ugly to have two headers with the same name even if they are in different directories (and the implementation file of the C function would need to move, or have a name that doesn't match its header).So the best option I can see is to have the
pluginClassvalue not actually be the plugin class's name, but instead the plugin's name without thePluginpart (which isn't quite what I did here since there was already a mismatch with the federated name; in the template it would beFoofor a plugin whose implementation isFooPlugin). That should be harmless, but it's kind of confusing. We could deprecatedpluginClassand replace it with a new key (continuing to honor pluginClass) to make it less odd, but I'm not sure it's worth the ecosystem migration (and/or inconsistency, depending on how many plugins migrate).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.
Hrm. Also not in love with this. Will book a quick sync to discuss, then we can summarise here.
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.
Summary: we agree that there's no obvious good option 😐 We're going to do this for now, and revisit our overall strategy when we update the plugin template to have the class in separate files. (And if we go a different route there, we can always update our plugins here.)