-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[url_launcher] Migrate unit tests to NNBD #3657
[url_launcher] Migrate unit tests to NNBD #3657
Conversation
Replaces the problematic Mockito mock with a manual mock that handles null and non-null types correctly. Removes the unit test of the example app, since it's not adding any actual coverage.
f09b300
to
8ccaca4
Compare
mock.canLaunch('http://example.com/foobar'), | ||
mock.launch( | ||
'http://example.com/foobar', | ||
mock |
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.
Why do we need to migrate away from "when...` in mockito and introduce our own mocks? mockito already migrated to stable we should be able to just use it with nnbd, right?
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.
Sadly it's nowhere near that straightforward :( See https://github.com/dart-lang/mockito/blob/master/NULL_SAFETY_README.md
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.
Do we prefer auto-gen or manual? The doc mentioned autogen is recommended and I see that you manually generated the mocks.
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.
The Mockito generation adds a bunch more moving pieces; it didn't seem worth adding that extra complexity for a simple class with a couple of methods. There may be cases where we want to do it, but if it's simple to just start from Fake
and build manually that seems easier.
This one is probably borderline since it's doing real mock validation rather than just faking; in a number of other cases we were using Mockito to make fakes, not actual mocks. We may want to give it a try for the remaining unit tests that need conversion and compare (or I can try it with this one if you'd like).
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.
I'll try with android_intent
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.
LGTM
This pull request is not suitable for automatic merging in its current state.
|
* master: Adopt Xcode 12 for podspec lints (flutter#3653) Run static analyzer during xctest (flutter#3667) [google_maps_flutter_web] update min flutter sdk version to 1.20.0 (flutter#3662) [image_picker] Run CocoaPods iOS tests in RunnerUITests target (flutter#3663) [webview_flutter] Run CocoaPods iOS tests in RunnerUITests target (flutter#3664) [device_info] Enable NNBD for unit test (flutter#3658) remove unused plugin (flutter#3661) [android_intent] move unit test to nullsafety (flutter#3659) [url_launcher] Migrate unit tests to NNBD (flutter#3657) [share] Migrate unit tests to null-safety. (flutter#3660) [connectivity_for_web] Migration to null-safety. (flutter#3652) [camera] Stable release for null safety. (flutter#3641) [in_app_purchase] fix plugin version (flutter#3654) Move plugin tool tests over (flutter#3606) [in_app_purchase] migrate playing billing library to v3 (flutter#3636) Update plugin_platform_interface min version (flutter#3650) # Conflicts: # packages/webview_flutter/CHANGELOG.md # packages/webview_flutter/example/ios/Runner.xcodeproj/project.pbxproj
Replaces the problematic Mockito mock with a manual mock that handles
null and non-null types correctly.
Pre-launch Checklist
[shared_preferences]
///
).