-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Conversation
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! I left a couple of notes/"bookmarks" in case somebody else wants to take a look.
_ambiguate(TestDefaultBinaryMessengerBinding.instance)! | ||
.defaultBinaryMessenger | ||
.setMockMethodCallHandler( | ||
channel, |
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.
Slight deviation of the pattern here, expected:
_ambiguate(TestDefaultBinaryMessengerBinding.instance)!
.defaultBinaryMessenger
.setMockMethodCallhandler(
maps.ensureChannelInitialized(mapId), (MethodCall methodCall) {...
(I think it's better extracting the channel to its own local, tbh)
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.
weird, i wonder why that happened.
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.
oh, i think this was the file i ended up looking at first, before i automated it, so this was done by hand
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.
to clarify, did you want me to make the changes consistent, or should i just land it as-is?
(this code will all get refactored in a few months anyway when the _ambiguate is removed.)
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'm going to mark this autosubmit for now given the repo merge, happy to land a separate PR if you think it's worth making the call sites more consistent)
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.
@Hixie nah, I just left these comments in case others wanted to look at what was "different" from the more automatic/mechanic changes. Thanks for landing this!
_ambiguate(TestDefaultBinaryMessengerBinding.instance)! | ||
.defaultBinaryMessenger | ||
.setMockMessageHandler( | ||
'flutter.io/videoPlayer/videoEvents123', | ||
(ByteData? message) async { | ||
final MethodCall methodCall = | ||
const StandardMethodCodec().decodeMethodCall(message); | ||
if (methodCall.method == 'listen') { | ||
await _ambiguate(ServicesBinding.instance) | ||
?.defaultBinaryMessenger | ||
await _ambiguate(TestDefaultBinaryMessengerBinding.instance)! | ||
.defaultBinaryMessenger | ||
.handlePlatformMessage( | ||
'flutter.io/videoPlayer/videoEvents123', | ||
const StandardMethodCodec() |
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 wonder why these couple of tests are using .setMockMessageHandler
instead of .setMockMethodCallHandler
... Is it so this can use a custom codec to decode the method call line 243?
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.
no idea.
/// | ||
/// We use this so that APIs that have become non-nullable can still be used | ||
/// with `!` and `?` on the stable branch. | ||
T? _ambiguate<T>(T? value) => value; |
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.
_ambiguate
is back!
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.
yeah, and with a vengeance. :-/
it'll disappear again after the next stable, if this all goes according to plan.
_ambiguate(TestDefaultBinaryMessengerBinding.instance)! | ||
.defaultBinaryMessenger | ||
.setMockMethodCallHandler( | ||
plugin.channel, | ||
(MethodCall methodCall) async { | ||
log.add(methodCall); | ||
return null; | ||
}, | ||
); |
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 what most of the changes on this PR look like:
- x.y.setMockMethodCallHandler(handler);
+ _ambiguate(TestDefaultBinaryMessengerBinding.instance)!
+ .defaultBinaryMessenger
+ .setMockMethodCallHandler(x.y, handler)
* 3c7d54bba [camerax] Implement camera preview (flutter/plugins#7112) * 48f50b4f1 [image_picker] Update NSPhotoLibraryUsageDescription description in README (flutter/plugins#6589) * eea17c996 Migrate these tests to the "new" API. (flutter/plugins#7189) * 190c6d916 Roll Flutter from 298d8c7 to 0be7c3f (21 revisions) (flutter/plugins#7194) * c0d4e8041 [google_sign_in] Endorses next web package. (flutter/plugins#7191) * cc4eaba0f [google_maps]: Bump org.mockito:mockito-core (flutter/plugins#7099) * 717a8bfef [image_picker]: Bump org.mockito:mockito-core (flutter/plugins#7097) * 8a09d8c13 [lifecycle]: Bump org.mockito:mockito-core (flutter/plugins#7096) * 40377a12a [in_app_pur]: Bump org.mockito:mockito-core (flutter/plugins#7094) * 6a4bbf1df [url_launcher]: Bump org.mockito:mockito-core (flutter/plugins#7098) * 96ab5cd12 Update codeowners (flutter/plugins#7188) * 00d5855cc Add missing CODEOWNER (flutter/plugins#7016) * c3e9d1ba3 [webview_flutter] Adds examples of accessing platform-specific features for each class (flutter/plugins#7089) * 1f7b57917 Roll Flutter from 0be7c3f to 33e4d21 (5 revisions) (flutter/plugins#7196) * 1acaf55c2 [plugins] Disables the AndroidGradlePluginVersion issue ID in all android packages (flutter/plugins#7045) * 132d9c77d [espresso] Update some dependencies (flutter/plugins#7195)
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).