[url_launcher] Add native unit tests for Windows #4156
Conversation
| platforms: | ||
| windows: | ||
| pluginClass: UrlLauncherPlugin | ||
| pluginClass: UrlLauncherWindows |
There was a problem hiding this comment.
This is not great, but I don't have a solution I like better so far. The issue is that the flutter tool generates both an #include path and a call to the C API based on the value of pluginClass, so if it stays as FooPlugin, then the public header has to be called foo_plugin.h. But the plugin class's file should match its name, so should be called foo_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 pluginClass value not actually be the plugin class's name, but instead the plugin's name without the Plugin part (which isn't quite what I did here since there was already a mismatch with the federated name; in the template it would be Foo for a plugin whose implementation is FooPlugin). That should be harmless, but it's kind of confusing. We could deprecated pluginClass and 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.
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.
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.)
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
lgtm with an optional nit and comment that's off-topic for this PR.
| using ::testing::Return; | ||
| using ::testing::SetArgPointee; | ||
|
|
||
| class MockSystemApis : public SystemApis { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
|
|
||
| } // namespace | ||
| bool UrlLauncherPlugin::CanLaunchUrl(const std::string& url) { | ||
| size_t separator_location = url.find(":"); |
There was a problem hiding this comment.
Off-topic for this patch, but I bet there's some Win32 API for parsing URLs out there. If the Dart side of this is already using the Url class it's much less of an issue; otherwise, : can appear as the port separator etc., or just as part of some garbage string the person passed in here. I suppse the net result would mostly be them shooting themselves in their own foot regardless unless they're writing an app to poke at a very specific and maybe not-particularly-valuable to know subset of users' registry keys.
There was a problem hiding this comment.
If the Dart side of this is already using the
Urlclass it's much less of an issue
The Dart side of the API is kind of a hot mess (flutter/flutter#79043), and one of the things I dislike most about it is that it takes a String. Someday I want to get back to that proposal and do the overhaul.
I suppse the net result would mostly be them shooting themselves in their own foot regardless
Yes, and at least unlike some of the other issues (like including unencoded spaces, which people do all the time) trying to pass a "URL" that doesn't actually have a scheme should completely fail on every platform, so be very easy to notice during development.
| platforms: | ||
| windows: | ||
| pluginClass: UrlLauncherPlugin | ||
| pluginClass: UrlLauncherWindows |
There was a problem hiding this comment.
Hrm. Also not in love with this. Will book a quick sync to discuss, then we can summarise here.
Adds a unit test target based on googletest. This is intended to be both a set of unit tests for this plugin, and also a model of changes that can be made to the `flutter create` template for Windows plugins to include better testing out of the box (flutter/flutter#82458). In addition to the test binary being directly runnable, the integration between CMake, VS, and googletest means that these tests are visible—and runnable—in the VS Test Explorer UI after opening the generated .sln file. Changes for testing in general: - Moved the plugin class declaration to a header. - Moved the C registration API implementation to a separate file. - Added (opt-in, so it won't affect plugin client builds) plugin CMake rules to download googletest and build a new executable target that builds all the plugin sources, plus gtest and gmock. - Added a line to the example app CMake rules to enable the unit tests. - Added a unit test file. url_launcher-specific changes: - Wrapped all Win32 calls in a thin class for mockability in unit tests. - Factored some logic into helpers for better maintainability while I was refactoring anyway. Note: This unit test is not yet being run by CI. A tools command to run Windows plugin unit tests will be a separate PR. Part of flutter/flutter#82445
Adds a unit test target based on googletest. This is intended to be both a set of unit tests for this plugin, and also a model of changes that can be made to the `flutter create` template for Windows plugins to include better testing out of the box (flutter/flutter#82458). In addition to the test binary being directly runnable, the integration between CMake, VS, and googletest means that these tests are visible—and runnable—in the VS Test Explorer UI after opening the generated .sln file. Changes for testing in general: - Moved the plugin class declaration to a header. - Moved the C registration API implementation to a separate file. - Added (opt-in, so it won't affect plugin client builds) plugin CMake rules to download googletest and build a new executable target that builds all the plugin sources, plus gtest and gmock. - Added a line to the example app CMake rules to enable the unit tests. - Added a unit test file. url_launcher-specific changes: - Wrapped all Win32 calls in a thin class for mockability in unit tests. - Factored some logic into helpers for better maintainability while I was refactoring anyway. Note: This unit test is not yet being run by CI. A tools command to run Windows plugin unit tests will be a separate PR. Part of flutter/flutter#82445
2 participants
Adds a unit test target based on googletest. This is intended to be both a set of unit tests for this plugin, and also a model of changes that can be made to the
flutter createtemplate for Windows plugins to include better testing out of the box (flutter/flutter#82458).In addition to the test binary being directly runnable, the integration between CMake, VS, and googletest means that these tests are visible—and runnable—in the VS Test Explorer UI after opening the generated .sln file.
Changes for testing in general:
url_launcher-specific changes:
Note: This unit test is not yet being run by CI. A tools command to run Windows plugin unit tests will be a separate PR.
Part of flutter/flutter#82445
Pre-launch Checklist
dart format.)[shared_preferences]///).