Skip to content

[pigeon] Initial integration test setup #2851

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 8 commits into from
Nov 28, 2022
Merged

[pigeon] Initial integration test setup #2851

auto-submit merged 8 commits into from
Nov 28, 2022

Conversation

@stuartmorgan-g
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g commented Nov 28, 2022 •
edited
Loading

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):

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 [pigeon] Reorganize test pigeons 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

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
[pigeon] Initial integration test setup
008576c 
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
@stuartmorgan-g
Version bump for bugfix
f89c57b
@flutter-dashboard
Copy link

flutter-dashboard bot commented Nov 28, 2022

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gaaclarke
gaaclarke reviewed Nov 28, 2022
View reviewed changes
packages/pigeon/platform_tests/test_plugin/example/lib/main.dart Outdated
class _MyAppState extends State<MyApp> {
// ignore: unused_field
final TestPlugin _testPlugin = TestPlugin();
late AllVoidHostApi api;
Copy link
Member

@gaaclarke gaaclarke Nov 28, 2022

Choose a reason for hiding this comment

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

final

Copy link
Collaborator Author

@stuartmorgan-g stuartmorgan-g Nov 28, 2022

Choose a reason for hiding this comment

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

Done.

packages/pigeon/platform_tests/test_plugin/macos/Classes/TestPlugin.swift Outdated
HostEverythingSetup.setUp(binaryMessenger: registrar.messenger, api: plugin)
}

public func handle(_ call: FlutterMethodCall, result: @escaping FlutterResult) {
Copy link
Member

@gaaclarke gaaclarke Nov 28, 2022

Choose a reason for hiding this comment

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

Can you not pull this out? It's a bit confusing to have.

Copy link
Collaborator Author

@stuartmorgan-g stuartmorgan-g Nov 28, 2022

Choose a reason for hiding this comment

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

I forgot it was marked as @optional in the protocol; removed.

Copy link
Collaborator Author

@stuartmorgan-g stuartmorgan-g Nov 28, 2022

Choose a reason for hiding this comment

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

I forgot it was marked as @optional in the protocol; removed.

packages/pigeon/tool/run_tests.dart
'macos_swift_unittests': _TestInfo(
function: _runMacOSSwiftUnitTests,
description: 'Unit tests on generated Swift code on macOS.'),
'macos_swift_integration_tests': _TestInfo(
Copy link
Member

@gaaclarke gaaclarke Nov 28, 2022

Choose a reason for hiding this comment

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

These are called e2e tests above, we should standardize the name.

Copy link
Collaborator Author

@stuartmorgan-g stuartmorgan-g Nov 28, 2022

Choose a reason for hiding this comment

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

My intent was to standardize on integration test (as that's what Flutter now calls those tests) in this file, and in the legacy shell script keep using e2e for now to avoid churn in a file that we want to eliminate anyway.

I've removed the no-op e2e iOS test from this file (it's only actually implemented in the shell script, and even there it's broken and will be removed later) to accelerate that process.

packages/pigeon/platform_tests/test_plugin/example/lib/main.dart Outdated
class _MyAppState extends State<MyApp> {
// ignore: unused_field
final TestPlugin _testPlugin = TestPlugin();
late AllVoidHostApi api;
Copy link
Member

@gaaclarke gaaclarke Nov 28, 2022

Choose a reason for hiding this comment

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

Shouldn't there be an EverythingApi too somewhere?

Copy link
Collaborator Author

@stuartmorgan-g stuartmorgan-g Nov 28, 2022

Choose a reason for hiding this comment

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

That's in the actual integration test file (which I discovered I didn't git add 🤦🏻 ). The change to main.dart isn't actually necessary for this PR, I just think it's less confusing to have it do a trivial "is Pigeon doing something" sanity test, and show a Pigeon call, than for it to do nothing, so I added a single call. I don't plan to add anything else here in the future (unless at some point someone wants to build some kind of interesting manual test UI, but I can't see a good use case for manual tests for Pigeon if we have good integration tests.)

gaaclarke
gaaclarke approved these changes Nov 28, 2022
View reviewed changes
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.

lgtm!

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 28, 2022
@auto-submit auto-submit bot merged commit ff16ee0 into flutter:main Nov 28, 2022
@stuartmorgan-g stuartmorgan-g deleted the pigeon-integration-initial branch November 28, 2022 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaaclarke gaaclarke gaaclarke approved these changes

@tarrinneal tarrinneal Awaiting requested review from tarrinneal tarrinneal is a code owner

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App needs tests p: pigeon

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[pigeon] void method generation for Swift produces PlatformException

2 participants

@stuartmorgan-g @gaaclarke