Windows unit tests no longer use the Windows SDK #2696
Conversation
MaikuB
left a comment
MaikuB
left a comment
There was a problem hiding this comment.
Thanks for looking into this. Could the issue have been addressed without having to change the main plugin the way it's been done in this PR? The issue that sticks is how the plugin required changes and makes use of a global variable to determine if the plugin should use a real implementation or a mock. Two antipatterns/code smells are happening as a result
|
Added a new constructor marked |
MaikuB
left a comment
MaikuB
left a comment
There was a problem hiding this comment.
Thanks for making the changes. I took a pass and believe there are some improvements that could be done
|
|
||
| /// Creates an instance of this plugin with the given (mocked) bindings. | ||
| @visibleForTesting | ||
| FlutterLocalNotificationsWindows.withBindings( |
There was a problem hiding this comment.
I was about to mention perhaps to rename it to FlutterLocalNotificationsWindows.private as it would be consistent with what you had on Linux. There was also an element of how the with<...> convention can lead to issues (repetition from readability perspective and could break if more arguments got introduced).
Rather than taking this approach, I noticed first-party plugins are taking an optional, nullable argument with the @visibleForTesting annotation applied to it. I think this is actually a good approach as implementation and test code would point to the same constructor. Would you be able to switch it so that the only constructor follows that pattern? An example of what I mean can be found here
| } | ||
|
|
||
| /// Does a basic syntax check on XML. | ||
| bool _checkXml(String xml) { |
There was a problem hiding this comment.
It's file private but look this could have been private within the FlutterLocalNotificationsWindows class. Would you be able to adjust this?
Note: on a related note and not that it has to be done in this PR but I believe I missed picking this up when it comes to _globalLaunchCallback as well
| import 'bindings.dart'; | ||
|
|
||
| /// Mocked FFI bindings. | ||
| class MockBindings implements NotificationsPluginBindings { |
There was a problem hiding this comment.
Can you place this in the test folder? It's purely test related and is the approach taken when creating mocks via mocktail or mockito or hand crafted ones. Note this is also currently a stub not a mock. A mock provides the ability to record the interactions that can then be used to verify what took place. Can you also rename the class and adjust the docs accordingly to reflect this?
| import 'base.dart'; | ||
|
|
||
| /// The Windows implementation of `package:flutter_local_notifications`. | ||
| class FlutterLocalNotificationsWindows extends WindowsNotificationsBase { |
There was a problem hiding this comment.
With the comment I left that allows the FFI implementation to take an argument for the bindings used behind the scenes, does this perhaps render the stub redundant now? Based on my own knowledge and testing, it would be redundant but let me know if I've missed anything
2 participants
Fixes flaky Windows unit tests on every PR, without losing too much. To actually test notifications, run the example app manually. These new tests probe the XML directly without actually passing it to the Windows SDK or even loading the DLL. To make sure we're still testing the Dart logic, I mocked out the FFI bindings, not the plugin itself, so that all the Dart validation logic still runs, but no C++ runs.
Fixes #2695