-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[pigeon] Add Flutter API integration tests #3066
[pigeon] Add Flutter API integration tests #3066
Conversation
@@ -280,7 +340,7 @@ abstract class FlutterIntegrationCoreApi { | |||
|
|||
/// Returns the passed map, to test serialization and deserialization. | |||
@ObjCSelector('echoNullableMap:') | |||
Map<String?, Object?> echoNullableMap(Map<String?, Object?> aMap); | |||
Map<String?, Object?>? echoNullableMap(Map<String?, Object?>? aMap); |
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 fixes a mistake I made when first adding it, and noticed while implementing the tests.
// echoObject.mapWithAnnotations, genericAllTypes.mapWithAnnotations), | ||
// true); | ||
// expect( | ||
// mapEquals(echoObject.mapWithObject, genericAllTypes.mapWithObject), true); |
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 was outdated; it only applies to AllNullableTypes
now.
}, | ||
// TODO(stuartmorgan): Fix and re-enable. | ||
// See https://github.com/flutter/flutter/issues/118726 | ||
skip: targetGenerator == TargetGenerator.kotlin); |
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 not sure what happened here; I didn't change anything that should have affected this, but this test was failing for me.
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.
Oops, it looks like we're not running Kotlin tests in CI by mistake; the only _integration_test
hit in the CI output is Swift. I'll look into that.
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 right, that's flutter/flutter#111505 🤦🏻
@@ -270,7 +270,22 @@ interface HostIntegrationCoreApi { | |||
/** Returns the passed string asynchronously. */ | |||
fun echoAsyncString(aString: String, callback: (String) -> Unit) | |||
fun callFlutterNoop(callback: () -> Unit) | |||
fun callFlutterEchoAllTypes(everything: AllTypes, callback: (AllTypes) -> Unit) | |||
fun callFlutterSendMultipleNullableTypes(aNullableBool: Boolean?, aNullableInt: Long?, aNullableString: String?, callback: (AllNullableTypes) -> Unit) |
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 have a linter/formatter for kotlin? Is this something we can roll in to our tools?
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.
We don't have one; filed flutter/flutter#118756
@@ -281,7 +281,22 @@ protocol HostIntegrationCoreApi { | |||
/// Returns the passed string asynchronously. | |||
func echoAsyncString(aString: String, completion: @escaping (String) -> Void) | |||
func callFlutterNoop(completion: @escaping () -> Void) | |||
func callFlutterEchoAllTypes(everything: AllTypes, completion: @escaping (AllTypes) -> Void) | |||
func callFlutterSendMultipleNullableTypes(aNullableBool: Bool?, aNullableInt: Int32?, aNullableString: String?, completion: @escaping (AllNullableTypes) -> Void) |
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.
Same with swift
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.
Tracked by flutter/flutter#41129; hopefully we'll have that fairly soon.
Per offline discussion the Java implementation could potentially be a little less verbose by using a shared generic result class. I'm going to go ahead and land it as-is since that's a minor detail in test-only code that shouldn't need to change, but I'll see if it's easy and if so do a follow-up later with it. |
* Add the new wrapper APIs * Regenerate files * Add the new integration tests * macOS Swift implementation * iOS Swift implementation * Android Kotlin implementation; some tests disabled * Android Java implementation * iOS Obj-C implementation * Windows C++ implementation
Adds
callFlutter*
wrappers for all of the recently-add Flutter APIecho*
methods incore_tests.dart
, and adds integration tests driving them all.This doesn't fix any of the exposed issues; they are all runtime rather than compile time, so I opted to file issues for each of them instead so they can be fixed on a per-generator basis (and so fixing them won't block getting the tests in place).
Part of flutter/flutter#116374
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.///
).