Android_hardware_smoke_test: Enable pixel exact local file comparator to read goldens from flutter asset URI#188587
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the on-device golden file comparison logic in the Android hardware smoke test. It introduces direct asset loading via the 'asset' URI scheme in PixelExactLocalFileComparator, eliminating the need to copy golden files to temporary directories. The request handling in goldens.dart is refactored into helper methods, and new widget tests are added to verify golden comparisons. Feedback on the changes suggests using Uint8List.sublistView instead of message!.buffer.asUint8List() in the mock asset handlers to safely extract message bytes.
- add support for reading the golden image from flutter asset URI
- use that instead of writing golden to temp
- add widget tests for comparator behavior
- update readme
017e8ce to
67baa15
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the golden image comparison logic in the Android hardware smoke test to load golden references directly from package assets using an asset URI scheme, removing the need to copy files to temporary directories on the device. Feedback highlights that the PixelExactLocalFileComparator only uses golden.path to load assets, which will fail to resolve the correct asset key if the URI is formatted as asset://test_driver/... because test_driver is parsed as the host.
There was a problem hiding this comment.
Code Review
This pull request refactors the golden image comparison logic in the Android hardware smoke test, splitting platform and standard view requests into helper functions and updating PixelExactLocalFileComparator to load golden assets directly via the asset URI scheme instead of copying them to a temporary directory. Comprehensive tests were also added to verify the comparator's behavior. The reviewer suggested a safer and more idiomatic way to retrieve the device pixel ratio using View.of(context) instead of bypassing the widget tree via ui.PlatformDispatcher.
|
@b-luk I've been requesting reviews only from @gaaclarke for this project so far, but I'd like to start distributing reviews across the team. Let me know if you have any questions about it |
|
|
||
| if (performAppSideGoldenCompare) { | ||
| final String? failureMessage = await compareGoldenOnDevice( | ||
| await _handlePlatformViewRequest(testName, completer, targetKey, settleFuture: settleFuture); |
There was a problem hiding this comment.
I wish github would have better UI for detecting and showing moved lines of code for cases like this.
There was a problem hiding this comment.
One guideline that I've seen is to use semantic line breaks. (For a PRs on the Flutter docs website, it's part of the PR template checklist). This suggests having a line break at least for every sentence, and even having breaks on phrases within each sentence.
I have not seen this suggestion followed very closely in most of the dart/flutter docs I've seen, including docs on the Flutter docs website where that checklist exists. So I think it's ignored pretty often. But it's something to consider. It would make it easier to see what changed in diffs like this.
There was a problem hiding this comment.
That makes sense, I'll do a pass in a followup PR
There was a problem hiding this comment.
That makes sense, I'll do a pass in a followup PR
…#12081) Manual roll Flutter from 0c80830e465b to ca9f874f5284 (119 revisions) Manual roll requested by [email protected] flutter/flutter@0c80830...ca9f874 2026-06-30 [email protected] [tool] Don't require a Flutter compile task when staging jniLibs (flutter/flutter#188805) 2026-06-30 [email protected] [Impeller] Fix potential overflow when allocating buffers at the next power of two size (flutter/flutter#188742) 2026-06-30 [email protected] [Framework/Tool] Decouple preview theme imports (flutter/flutter#188176) 2026-06-30 [email protected] Add import of `dart:_js_interop_wasm` in sdk rewriter tool (flutter/flutter#188620) 2026-06-30 [email protected] Detach LLDB and print stack trace on process stop (flutter/flutter#188576) 2026-06-30 [email protected] [Linux] Fix FlCompositorOpenGL.pixels comment (flutter/flutter#188754) 2026-06-30 [email protected] [flutter_tools] Fix crash in flutter create when pubspec.yaml is empty (flutter/flutter#188385) 2026-06-30 [email protected] Roll Packages from 656ccaa to 274ed3e (23 revisions) (flutter/flutter#188792) 2026-06-30 [email protected] In AndroidImageGenerator, check that the destination pixel buffer has sufficient capacity for the decoded data (flutter/flutter#188752) 2026-06-30 [email protected] Roll pub packages (flutter/flutter#188773) 2026-06-30 [email protected] [Impeller] Add a flat VertexAttributeFormat for vertex inputs (flutter/flutter#188684) 2026-06-30 [email protected] Roll pub packages (flutter/flutter#188764) 2026-06-30 [email protected] Roll Dart SDK from 0cb483880b6b to e1bdb9ce3327 (2 revisions) (flutter/flutter#188763) 2026-06-29 [email protected] Handle 'no permissions' adb device state (flutter/flutter#187248) 2026-06-29 [email protected] [windows]: adjusts uniform buffers to hit hlsl optimization (flutter/flutter#188538) 2026-06-29 [email protected] Roll Skia from bfb7860cb9c7 to 71947c4110b0 (9 revisions) (flutter/flutter#188747) 2026-06-29 [email protected] ci: extract wait-for-engine-build logic into a reusable composite action (flutter/flutter#188748) 2026-06-29 [email protected] Provide guided migration logs when iOS app crashes on simulator (flutter/flutter#188736) 2026-06-29 [email protected] Revert "[flutter_tools] Track asset transformer dependencies for hot reload" (flutter/flutter#188751) 2026-06-29 [email protected] Migrate ABI splits to new AGP dsl (flutter/flutter#188369) 2026-06-29 [email protected] Add Impeller+OpenGLES startup benchmark for mokey (flutter/flutter#188495) 2026-06-29 [email protected] Increase macOS minimum supported version from 10.15 to 12 to support Xcode 27 (flutter/flutter#188520) 2026-06-29 [email protected] Moves test ownership validation to flutter/flutter (flutter/flutter#188655) 2026-06-29 [email protected] Add --flavor support for Windows desktop builds (flutter/flutter#187034) 2026-06-29 [email protected] Android_hardware_smoke_test: Enable pixel exact local file comparator to read goldens from flutter asset URI (flutter/flutter#188587) 2026-06-29 [email protected] [flutter_tools] Use DeviceHub.app for iOS simulator path on Xcode 27+ (flutter/flutter#187910) 2026-06-29 [email protected] Roll Skia from 111e7582d081 to bfb7860cb9c7 (2 revisions) (flutter/flutter#188731) 2026-06-29 [email protected] Use `revert` label instead of `revert_wf` (flutter/flutter#188639) 2026-06-29 [email protected] Properly await Dart Development Service shutdown with timeout (flutter/flutter#188387) 2026-06-29 [email protected] [flutter_tools] Track asset transformer dependencies for hot reload (flutter/flutter#187947) 2026-06-29 [email protected] [Tool] Tolerate malformed UTF-8 in process streaming decoders (flutter/flutter#188453) 2026-06-29 [email protected] [flutter_tools] Use new ddc modules in test (flutter/flutter#188240) 2026-06-29 [email protected] Roll Packages from c1f7d92 to 656ccaa (12 revisions) (flutter/flutter#188728) 2026-06-29 [email protected] Remove unused fields (flutter/flutter#188705) 2026-06-29 [email protected] Clear text input handler widget on view dispose (flutter/flutter#188701) 2026-06-29 [email protected] Free compositor in view renderer finalize to avoid use-after-free (flutter/flutter#188702) 2026-06-29 [email protected] [linux] Use GWeakRef in mock signal handler test helper (flutter/flutter#188700) 2026-06-29 [email protected] Fixing few related editing issues with LTR/RTL text (flutter/flutter#188503) 2026-06-29 [email protected] [Flutter GPU] Add Texture.fromImage to wrap a ui.Image texture (flutter/flutter#188605) 2026-06-29 [email protected] [Flutter GPU] Honor the enable argument in RenderPass.setDepthWriteEnable (flutter/flutter#188715) 2026-06-29 [email protected] Roll Skia from ba1942d8c3e1 to 111e7582d081 (1 revision) (flutter/flutter#188721) 2026-06-29 [email protected] Roll Skia from 587d8befe1ee to ba1942d8c3e1 (6 revisions) (flutter/flutter#188717) 2026-06-29 [email protected] Remove some refs to package:intl (flutter/flutter#188504) 2026-06-29 [email protected] [VPAT] Update a11y assessment app FAB example to announce value change when it's updated. (flutter/flutter#188466) ...
…flutter#12081) Manual roll Flutter from 0c80830e465b to ca9f874f5284 (119 revisions) Manual roll requested by [email protected] flutter/flutter@0c80830...ca9f874 2026-06-30 [email protected] [tool] Don't require a Flutter compile task when staging jniLibs (flutter/flutter#188805) 2026-06-30 [email protected] [Impeller] Fix potential overflow when allocating buffers at the next power of two size (flutter/flutter#188742) 2026-06-30 [email protected] [Framework/Tool] Decouple preview theme imports (flutter/flutter#188176) 2026-06-30 [email protected] Add import of `dart:_js_interop_wasm` in sdk rewriter tool (flutter/flutter#188620) 2026-06-30 [email protected] Detach LLDB and print stack trace on process stop (flutter/flutter#188576) 2026-06-30 [email protected] [Linux] Fix FlCompositorOpenGL.pixels comment (flutter/flutter#188754) 2026-06-30 [email protected] [flutter_tools] Fix crash in flutter create when pubspec.yaml is empty (flutter/flutter#188385) 2026-06-30 [email protected] Roll Packages from 656ccaa to 274ed3e (23 revisions) (flutter/flutter#188792) 2026-06-30 [email protected] In AndroidImageGenerator, check that the destination pixel buffer has sufficient capacity for the decoded data (flutter/flutter#188752) 2026-06-30 [email protected] Roll pub packages (flutter/flutter#188773) 2026-06-30 [email protected] [Impeller] Add a flat VertexAttributeFormat for vertex inputs (flutter/flutter#188684) 2026-06-30 [email protected] Roll pub packages (flutter/flutter#188764) 2026-06-30 [email protected] Roll Dart SDK from 0cb483880b6b to e1bdb9ce3327 (2 revisions) (flutter/flutter#188763) 2026-06-29 [email protected] Handle 'no permissions' adb device state (flutter/flutter#187248) 2026-06-29 [email protected] [windows]: adjusts uniform buffers to hit hlsl optimization (flutter/flutter#188538) 2026-06-29 [email protected] Roll Skia from bfb7860cb9c7 to 71947c4110b0 (9 revisions) (flutter/flutter#188747) 2026-06-29 [email protected] ci: extract wait-for-engine-build logic into a reusable composite action (flutter/flutter#188748) 2026-06-29 [email protected] Provide guided migration logs when iOS app crashes on simulator (flutter/flutter#188736) 2026-06-29 [email protected] Revert "[flutter_tools] Track asset transformer dependencies for hot reload" (flutter/flutter#188751) 2026-06-29 [email protected] Migrate ABI splits to new AGP dsl (flutter/flutter#188369) 2026-06-29 [email protected] Add Impeller+OpenGLES startup benchmark for mokey (flutter/flutter#188495) 2026-06-29 [email protected] Increase macOS minimum supported version from 10.15 to 12 to support Xcode 27 (flutter/flutter#188520) 2026-06-29 [email protected] Moves test ownership validation to flutter/flutter (flutter/flutter#188655) 2026-06-29 [email protected] Add --flavor support for Windows desktop builds (flutter/flutter#187034) 2026-06-29 [email protected] Android_hardware_smoke_test: Enable pixel exact local file comparator to read goldens from flutter asset URI (flutter/flutter#188587) 2026-06-29 [email protected] [flutter_tools] Use DeviceHub.app for iOS simulator path on Xcode 27+ (flutter/flutter#187910) 2026-06-29 [email protected] Roll Skia from 111e7582d081 to bfb7860cb9c7 (2 revisions) (flutter/flutter#188731) 2026-06-29 [email protected] Use `revert` label instead of `revert_wf` (flutter/flutter#188639) 2026-06-29 [email protected] Properly await Dart Development Service shutdown with timeout (flutter/flutter#188387) 2026-06-29 [email protected] [flutter_tools] Track asset transformer dependencies for hot reload (flutter/flutter#187947) 2026-06-29 [email protected] [Tool] Tolerate malformed UTF-8 in process streaming decoders (flutter/flutter#188453) 2026-06-29 [email protected] [flutter_tools] Use new ddc modules in test (flutter/flutter#188240) 2026-06-29 [email protected] Roll Packages from c1f7d92 to 656ccaa (12 revisions) (flutter/flutter#188728) 2026-06-29 [email protected] Remove unused fields (flutter/flutter#188705) 2026-06-29 [email protected] Clear text input handler widget on view dispose (flutter/flutter#188701) 2026-06-29 [email protected] Free compositor in view renderer finalize to avoid use-after-free (flutter/flutter#188702) 2026-06-29 [email protected] [linux] Use GWeakRef in mock signal handler test helper (flutter/flutter#188700) 2026-06-29 [email protected] Fixing few related editing issues with LTR/RTL text (flutter/flutter#188503) 2026-06-29 [email protected] [Flutter GPU] Add Texture.fromImage to wrap a ui.Image texture (flutter/flutter#188605) 2026-06-29 [email protected] [Flutter GPU] Honor the enable argument in RenderPass.setDepthWriteEnable (flutter/flutter#188715) 2026-06-29 [email protected] Roll Skia from ba1942d8c3e1 to 111e7582d081 (1 revision) (flutter/flutter#188721) 2026-06-29 [email protected] Roll Skia from 587d8befe1ee to ba1942d8c3e1 (6 revisions) (flutter/flutter#188717) 2026-06-29 [email protected] Remove some refs to package:intl (flutter/flutter#188504) 2026-06-29 [email protected] [VPAT] Update a11y assessment app FAB example to announce value change when it's updated. (flutter/flutter#188466) ...
…lutter#189613) Apply semantic line breaks to android_hardware_smoke_test's readme. This was recommended by @b-luk in a [review comment](flutter#188587 (comment)) ## 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
Cleanup related to #182123
Pre-launch Checklist
///).