Skip to content

Conversation

@stuartmorgan-g
Copy link
Collaborator

Replaces all of the individual platform test harnesses with two plugins—one for most languages, one for alternate languages (Java and Obj-C)—to consolidate everything into a multi-platform test harness that uses exactly the same structure as all of the native unit tests in flutter/plugins. This has several benefits:

  • Knowledge of how our plugin tests work (including our docs) transfers directly to Pigeon.
  • There's a clear path to adding integration tests that also follow the existing plugin test pattern.
  • There's less boilerplate, since we don't have a separate harness for each plugin.
  • This moves us closer to the possibility of replacing a lot of run_test.* logic with the existing repo tooling that knows how to run tests that look like this (although currently not when the plugin is a sub-package).

In most cases the test files moved with essentially no changes. The primary exception was macOS, which was using a completely different (Cocoapod-based) test setup, so that test was restructured.

(No changelog or version changes since these are dev-only changes, but the check doesn't know that since the structure is unusual here.)

Part of flutter/flutter#111505

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@stuartmorgan-g stuartmorgan-g added override: no versioning needed Override the check requiring version bumps for most changes override: no changelog needed Override the check requiring CHANGELOG updates for most changes labels Nov 11, 2022
@stuartmorgan-g
Copy link
Collaborator Author

Notes for review:

  • This will be much easier to review by looking at the individual commits.
    • The second and third are just directly checking in flutter create output, so can probably just be skipped entirely.
    • The fourth is some cleanup to the template to get rid of parts we don't need.
    • Each language is its own commit; most are straightforward, and most of the file count is just deleting the old harness for each language.
  • I kept structural improvements to run_tests.* (migrating things from Bash to Dart, extracting sharable code, etc.) out of scope because I didn't want this to balloon.

If you'd like me to cut this into an initial PR for the scaffolding and then one for each language, let me know. That was my original plan, but as I went through it, it seemed like it might be easier (and faster) to review as a series of incremental commits rather than a flurry of PRs, but since the commits are already separated it would be pretty easy to change to multi-PR if you would prefer.

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this have any impact on our ability to run platform specific tests?

Does calling Java and ObjC 'alternate' make sense when the Kotlin and Swift generators are still in experimental status?

Seems like a lot of work, thank you for doing this!

@@ -0,0 +1,3 @@
## 0.0.1

* TODO: Describe initial release.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Todo before this merge, or after?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this is boilerplate from create; I need to delete these files.

@tarrinneal
Copy link
Contributor

Also, it appears that macos pigeon tests are failing

FAILURE: Build failed with an exception.
* Where:
Build file '/private/var/folders/bf/0cr6_1t525j9r585tdpgnv_m0000gn/T/cirrus-ci-build/packages/pigeon/platform_tests/alternate_language_test_plugin/example/android/app/build.gradle' line: 24
* What went wrong:
A problem occurred evaluating project ':app'.
> Failed to apply plugin 'com.android.internal.application'.
   > Android Gradle plugin requires Java 11 to run. You are currently using Java 1.8.
     Your current JDK is located in  /Library/Java/JavaVirtualMachines/temurin-8.jdk/Contents/Home/jre
     You can try some of the following options:
       - changing the IDE settings.
       - changing the JAVA_HOME environment variable.
       - changing `org.gradle.java.home` in `gradle.properties`.

Copy link
Collaborator Author

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I thought the java 11 thing in CI was resolved a while ago. I'm have to dig into that more.

Will this have any impact on our ability to run platform specific tests?

If people are running them manually it'll be a little different, but the script is updated to include the changes.

Does calling Java and ObjC 'alternate' make sense when the Kotlin and Swift generators are still in experimental status?

There wasn't a great name for this second plugin 😐 I named it "alternate" from the perspective of plugin development, where the default languages for create are Kotlin and Swift, since that seems like the most future-proof (at some point these won't be experimental languages), but I'm definitely open to other names.

@@ -0,0 +1,3 @@
## 0.0.1

* TODO: Describe initial release.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this is boilerplate from create; I need to delete these files.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this a read through, looking good. I'll dive more deeply after the CI steps are green. I don't want to look at it then you have to change things around to fix CI.

@@ -298,7 +298,7 @@ run_android_unittests() {
gen_android_unittests_code ./pigeons/void_arg_host.dart VoidArgHost
gen_android_unittests_code ./pigeons/voidflutter.dart VoidFlutter
gen_android_unittests_code ./pigeons/voidhost.dart VoidHost
cd platform_tests/android_unit_tests
cd platform_tests/alternate_language_test_plugin/example
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that we'll move to the standard plugin testing scripts at some point, not here, right? Will @tarrinneal integration tests get run here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that we'll move to the standard plugin testing scripts at some point, not here, right?

Ideally; see PR description.

Will @tarrinneal integration tests get run here?

These scaffolds will host the integration tests as well; running them will be a different step since it will use flutter drive rather than gradle.

@stuartmorgan-g
Copy link
Collaborator Author

Hmm, I thought the java 11 thing in CI was resolved a while ago.

I was thinking of our Linux images; that's where most of our Android plugin testing is done. For now I'm going to roll back the Gradle versions here to avoid the issue. Later we can either see about getting a macOS image with Java 11 (e.g., maybe moving this test to LUCI where we can adjust the images), or making the Android Pigeon tests not run on macOS in CI.

@stuartmorgan-g
Copy link
Collaborator Author

stuartmorgan-g commented Nov 15, 2022

I'm not sure why the macOS tests keep timing out; there's nothing helpful I can find in the logs when it's happening. From the CPU graph it looks like pretty early on something gets into a loop and just stays there forever, because the CPU stays completely pegged for the rest of the run.

(Edit: And locally this whole test run takes me a little under 7 minutes, so it's not like we're close to the 1 hour bot timeout if things are working.)

@stuartmorgan-g
Copy link
Collaborator Author

Since CI logs aren't helping and I can't repro locally, I think the best option is to turn this into a series of PRs and land them incrementally, to narrow down the problem.

@stuartmorgan-g
Copy link
Collaborator Author

Closing in favor of #2816; all of the other pieces have now landed separately. (The problem is with moving the ObjC tests into the new harness, but I'm not sure why yet.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

override: no changelog needed Override the check requiring CHANGELOG updates for most changes override: no versioning needed Override the check requiring version bumps for most changes p: pigeon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants