Add other types of platform view tests to android_hardware_smoke_test#188244
Conversation
|
Updated goldens look like this: platformViewHybridCompositionPlusPlusTest (vulkan only) platformViewHybridCompositionTest platformViewTextureLayerTest |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Android hardware smoke test suite to support and test three platform view composition modes: Texture Layer Hybrid Composition (TLHC), legacy Hybrid Composition (HC), and Hybrid Composition++ (HCPP), including skip logic for devices without HCPP support. The review feedback points out an incorrect platform views channel name used in the mock setup ('flutter/platform_views_2' instead of 'flutter/platform_views') and suggests refactoring duplicated 'onCreated' callback logic across the platform view test cases.
There was a problem hiding this comment.
Code Review
This pull request refactors the Android hardware smoke test to support and test multiple platform view composition modes: Texture Layer Hybrid Composition (TLHC), legacy Hybrid Composition (HC), and Hybrid Composition++ (HCPP). It introduces a PlatformViewMode enum, updates AndroidPlatformView to initialize views using the corresponding mode, and adds logic to skip HCPP tests if the device does not support it. Feedback on the changes suggests correcting the mocked platform views method channel name in widget_test.dart from 'flutter/platform_views_2' to 'flutter/platform_views' to ensure the HCPP support check is mocked correctly, and removing a redundant local variable goldenName in goldens.dart.
- add comment about why `platform_views_2` is used - remove reduntant variable
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM, just a few minor nits
| find.byType(AndroidPlatformView), | ||
| ); | ||
| final PlatformViewMode expectedMode = switch (testName) { | ||
| 'platformViewTextureLayerTest' => PlatformViewMode.textureLayer, |
There was a problem hiding this comment.
worth pulling out these constants
There was a problem hiding this comment.
I did a pass on the whole directory to pull out all constants in both dart and kotlin
| const String platformViewTextureLayerTestName = 'platformViewTextureLayerTest'; | ||
| const String platformViewHybridCompositionTestName = | ||
| 'platformViewHybridCompositionTest'; | ||
| const String platformViewHybridCompositionPlusPlusTestName = | ||
| 'platformViewHybridCompositionPlusPlusTest'; |
There was a problem hiding this comment.
these can have internal linkage (_platformViewTextureLayerTestName)
There was a problem hiding this comment.
removed redundant constants
github-actions
Bot
added
CICD
There was a problem hiding this comment.
It'd be nice if we could generate these. I know that is a bit of a refactor, something to put on the back-burner.
There was a problem hiding this comment.
I think we can generate them fairly simply. I considered doing that here but it felt too far away from the purpose of this PR and maybe not worth the added complexity. I'll file a low priority sub-issue to follow up
andywolff
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/188244, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
Whoops, looks like the constants commit contained a new a ktlint violation. Fixed. Sorry, could you please re-approve? |
andywolff
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/188244, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
andywolff
added
the
autosubmit
|
flutter-dashboard
Bot
removed
the
autosubmit
…flutter#188244) Expand test coverage to address more of flutter#182123 This adds tests to cover the three types of platform views described in https://docs.flutter.dev/platform-integration/android/platform-views For HCPP, we skip the test if we determine HCPP is not supported in the current run. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
3 participants
Expand test coverage to address more of #182123
This adds tests to cover the three types of platform views described in https://docs.flutter.dev/platform-integration/android/platform-views
For HCPP, we skip the test if we determine HCPP is not supported in the current run.
Pre-launch Checklist
///).