Skip to content

[pigeon] Add end-to-end integration_test tests for all generators #111505

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

Closed
8 tasks done
stuartmorgan-g opened this issue Sep 13, 2022 · 3 comments · Fixed by flutter/packages#4484
Closed
8 tasks done
Labels
a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. p: pigeon related to pigeon messaging codegen tool P1 High-priority issues at the top of the work list package flutter/packages repository. See also p: labels. team-ecosystem Owned by Ecosystem team triaged-ecosystem Triaged by Ecosystem team

Comments

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Sep 13, 2022

Currently the Pigeon testing is using unit tests on the Dart and native sides; this is useful, but leaves open the potential for unit tests on each side to pass, but for the protocol being unit tested to mis-align slightly such that the end-to-end call doesn't actually work. #111083 is an example of a case that should have been trivially caught by basic integration tests, but has to be more explicitly tested correctly with unit tests based on details of the serialization format on each end. (We also don't have that unit test on the Swift side, which is a problem, but a simple test would be even better.)

I would suggest that we rework the platform_tests directory so that instead of having a combination (depending on language) of bespoke tests and per-language dummy plugins, we just have one single test_plugin plugin that supports every platform/language (or I guess, two plugins: one for ObjC and Java, and one for everything else, since they can't really co-exist with their alternate languages in the same plugin). That plugin would have:

  • Unit tests for each language, using exactly the same structure that native unit tests in flutter/plugins use. That's already how Windows works, and the others could be converted.
  • integration_test tests that would drive full end-to-end calls for a variety of core use cases.

(Eventually I would like to revisit using the plugin repo tooling to drive all of this; I temporarily abandoned that when setting up Windows because it would involve more tool changes than I had time to make, but it would make things more sustainable long term that maintaining lots of run_tests.{dart,sh} logic. Using this structure would have the added benefit of setting ourselves up for that being easier to add later.)

/cc @gaaclarke @tarrinneal

Checklist:

  • Add Swift macOS integration tests.
  • Add Swift iOS integration tests.
    • These are mostly redundant with macOS, but there are features like background platform channels that we don't currently support on desktop. And the added cost of having both should be low. (I don't want to do iOS-only, because for local iteration a macOS integration test is way faster.)
  • Add Windows integration tests.
  • Add Java Android integration tests, local-only (not in CI)
  • Add Kotlin Android integration tests, local-only (not in CI)
  • Update [pigeon] Consolidate platform test harnesses part 3.2 - iOS ObjC packages#2816 to include adding Obj-C integration tests
  • Migrate the handful of (Obj-C-only) tests that are currently in the e2e_tests directory to the new structure, and eliminate e2e_tests.
  • Enable Java and Kotlin integration tests in CI
    • This is currently hard, which is why it's a separate step:
      • We could use FTL, like we do in flutter/plugins, but that's a lot of complexity/setup to bring into the tooling (or we need to adapt the repo tooling to work with Pigeon's unusual structure), and:
      • I'm hopeful that we can use the new (WIP?) LUCI Android emulator support instead, but that will require us to get that part of the testing moved over to LUCI, and emulator support wired in.
@stuartmorgan-g stuartmorgan-g added a: tests "flutter test", flutter_test, or one of our tests p: first party package flutter/packages repository. See also p: labels. p: pigeon related to pigeon messaging codegen tool P1 High-priority issues at the top of the work list labels Sep 13, 2022
@stuartmorgan-g stuartmorgan-g added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Nov 18, 2022
stuartmorgan-g added a commit to stuartmorgan-g/packages that referenced this issue Nov 28, 2022
This sets up initial proof-of-concept integration tests using the new
shared native test harness:
- Integration tests on the Dart side for void->void and
  Everything->Everything calls.
- macOS implementations in the test plugin on the native side.
- A new test target in the test script to drive them via `flutter test`.
- A minimal change to the example app so that `flutter run`-ing it will
  test that the void->void call is wired up.

Since this simple initial test hit
flutter/flutter#111083, which caused the test
to fail, this includes a fix for that.

Short-term future work (by me):
- Add integration test native setup and script targets for the other
  generators. This includes one just to keep the initial review scope
  smaller.
- Update flutter#2816 to include the
  integration test since it's still blocked until I can address the CI
  issues.

Medium-term future work (not all by me):
- Remove the legacy iOS e2e test scaffold that is currently disabled.
- Add significantly more integration test coverage (likely including
  flutter/flutter#115168 to reduce redundant
  API setup), including Flutter API integration tests rather than just
  host API tests.

Part of flutter/flutter#111505
Fixes flutter/flutter#111083
auto-submit bot pushed a commit to flutter/packages that referenced this issue Nov 28, 2022
* [pigeon] Initial integration test setup

This sets up initial proof-of-concept integration tests using the new
shared native test harness:
- Integration tests on the Dart side for void->void and
  Everything->Everything calls.
- macOS implementations in the test plugin on the native side.
- A new test target in the test script to drive them via `flutter test`.
- A minimal change to the example app so that `flutter run`-ing it will
  test that the void->void call is wired up.

Since this simple initial test hit
flutter/flutter#111083, which caused the test
to fail, this includes a fix for that.

Short-term future work (by me):
- Add integration test native setup and script targets for the other
  generators. This includes one just to keep the initial review scope
  smaller.
- Update #2816 to include the
  integration test since it's still blocked until I can address the CI
  issues.

Medium-term future work (not all by me):
- Remove the legacy iOS e2e test scaffold that is currently disabled.
- Add significantly more integration test coverage (likely including
  flutter/flutter#115168 to reduce redundant
  API setup), including Flutter API integration tests rather than just
  host API tests.

Part of flutter/flutter#111505
Fixes flutter/flutter#111083

* Version bump for bugfix

* Check in generated files needed for analysis

* Add the actual integration test file, which was left out

* Address review comments

* Fix incorrect Swift unit test for void call fix

* Analysis ignore

* Autoformat
stuartmorgan-g added a commit to stuartmorgan-g/packages that referenced this issue Nov 28, 2022
Adds a new test target to run the new shared integration tests on
Kotlin, and adds the necessary native-side plumbing.

This is not run by default because it requires a device or emulator to
be present, and we don't have that set up for CI yet (and we don't yet
have a different script entrypoint for manual vs CI tests).

Enabling this in CI is tracked in
flutter/flutter#111505, but landing it now
allows for better manual testing of generator changes during development.

Part of flutter/flutter#111505
stuartmorgan-g added a commit to stuartmorgan-g/packages that referenced this issue Nov 28, 2022
Enables the new shared integration tests for the Windows C++ generator,
adding the new corresponding test script target and running it by
default in CI.

Part of flutter/flutter#111505
auto-submit bot pushed a commit to flutter/packages that referenced this issue Nov 28, 2022
Enables the new shared integration tests for the Windows C++ generator,
adding the new corresponding test script target and running it by
default in CI.

Part of flutter/flutter#111505
auto-submit bot pushed a commit to flutter/packages that referenced this issue Nov 29, 2022
* [pigeon] Add local-only Kotlin integration tests

Adds a new test target to run the new shared integration tests on
Kotlin, and adds the necessary native-side plumbing.

This is not run by default because it requires a device or emulator to
be present, and we don't have that set up for CI yet (and we don't yet
have a different script entrypoint for manual vs CI tests).

Enabling this in CI is tracked in
flutter/flutter#111505, but landing it now
allows for better manual testing of generator changes during development.

Part of flutter/flutter#111505

* Add copyright
stuartmorgan-g added a commit to flutter/packages that referenced this issue Jun 14, 2023
@flutter-triage-bot flutter-triage-bot bot added team-ecosystem Owned by Ecosystem team triaged-ecosystem Triaged by Ecosystem team and removed triaged-ecosystem Triaged by Ecosystem team labels Jul 8, 2023
@flutter-triage-bot
Copy link

This issue is marked P1 but has had no recent status updates.

The P1 label indicates high-priority issues that are at the top of the work list. This is the highest priority level a bug can have if it isn't affecting a top-tier customer or breaking the build. Bugs marked P1 are generally actively being worked on unless the assignee is dealing with a P0 bug (or another P1 bug). Issues at this level should be resolved in a matter of months and should have monthly updates on GitHub.
Please consider where this bug really falls in our current priorities, and label it or assign it accordingly. This allows people to have a clearer picture of what work is actually planned. Thanks!

@stuartmorgan-g
Copy link
Contributor Author

stuartmorgan-g commented Jul 9, 2023

This is blocked on emulator support in CI, but we're now close to having that.

@stuartmorgan-g stuartmorgan-g added the triaged-ecosystem Triaged by Ecosystem team label Jul 9, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this issue Jul 17, 2023
Enables the new emulator support for the Linux custom package test targets, and enables the emulator-based Android integration tests for Pigeon.

Drops the cores from the high-core config (32) to the default (8) since the emulator requires KVM, and there are currently no 32-core KVM machines in the pool. In practice, it appears that this doesn't have much affect on the runtime.

Fixes flutter/flutter#111505
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. p: pigeon related to pigeon messaging codegen tool P1 High-priority issues at the top of the work list package flutter/packages repository. See also p: labels. team-ecosystem Owned by Ecosystem team triaged-ecosystem Triaged by Ecosystem team
Projects
None yet
1 participant