Fix EmbedderTest.CanRenderTextWithImpellerMetal test breakage#186262
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the render_impeller_text_test fixture to render two paragraphs with inverted color schemes. Feedback suggests refactoring the duplicated paragraph construction into a helper function, ensuring ParagraphBuilder.pushStyle calls are balanced with pop(), and using absolute offsets instead of canvas translations for better clarity.
| final paragraphBuilder1 = ParagraphBuilder(ParagraphStyle(fontFamily: 'sans-serif')) | ||
| ..pushStyle(TextStyle(color: darkColor, background: Paint()..color = lightColor)) | ||
| ..addText('Flutter is the best!'); | ||
| final Paragraph paragraph1 = paragraphBuilder1.build() | ||
| ..layout(const ParagraphConstraints(width: 400)); | ||
| canvas.drawParagraph(paragraph1, const Offset(20, 20)); | ||
|
|
||
| canvas.translate(0, 40); | ||
|
|
||
| final paragraphBuilder2 = ParagraphBuilder(ParagraphStyle(fontFamily: 'sans-serif')) | ||
| ..pushStyle(TextStyle(color: lightColor, background: Paint()..color = darkColor)) | ||
| ..addText('Flutter is the best!'); | ||
| final Paragraph paragraph = paragraphBuilder.build() | ||
| final Paragraph paragraph2 = paragraphBuilder2.build() | ||
| ..layout(const ParagraphConstraints(width: 400)); | ||
| canvas.drawParagraph(paragraph, const Offset(20, 20)); | ||
| canvas.drawParagraph(paragraph2, const Offset(20, 20)); |
There was a problem hiding this comment.
The paragraph building logic is duplicated for the dark and light text cases. Refactoring this into a helper function improves maintainability and readability. Additionally, ParagraphBuilder.pushStyle should be balanced with a pop() call as per the dart:ui API contract. Finally, using absolute offsets instead of mixing canvas.translate with relative offsets makes the positioning logic clearer.
void drawParagraph(Color textColor, Color bgColor, double y) {
final builder = ParagraphBuilder(ParagraphStyle(fontFamily: 'sans-serif'))
..pushStyle(TextStyle(color: textColor, background: Paint()..color = bgColor))
..addText('Flutter is the best!')
..pop();
final paragraph = builder.build()
..layout(const ParagraphConstraints(width: 400));
canvas.drawParagraph(paragraph, Offset(20, y));
}
drawParagraph(darkColor, lightColor, 20);
drawParagraph(lightColor, darkColor, 60);|
It passes locally with the image from 276ad48 The Metal embedder tests may need to do some kind of approximate image matching in order to work in both local and CI environments. |
I think our Skia/Flutter gold testing has an allowable pixel different rate of 0.01. (Default defined here. I don't think it's overridden anywhere so I think we always use the default.) Maybe we can use the same threshold for these tests. |
|
https://github.com/flutter/flutter/pull/186262/checks?check_run_id=75113080600 mac_unopt failed again, but due to timing out because of a slow "mounting git" step for host_debug_unopt_arm64: https://ci.chromium.org/ui/p/flutter/builders/try/Mac%20Engine%20Drone/2319766/overview (Related slow "mounting git" step issue: #185767). I'd rerun the test, but I think it's not giving me a re-run button because it's a suppressed test. The host_debug_arm64_tests, host_profile_arm64_tests, and host_release_arm64_tests tests which were previously failing for the golden test have passed. So I think this is good enough to submit. |
…le number of pixel differences
|
I'd like to ensure that these tests can pass both locally and on CI. I tried creating a patch that lets the tests specify an allowable number of pixel differences. This works on my local setup with the golden images from this PR that pass on CI: Can you cherry pick the patch into this PR? |
|
autosubmit label was removed for flutter/flutter/186262, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
This fixes the original mac_unopt failures. However, a different test in mac_unopt is failing. https://ci.chromium.org/ui/p/flutter/builders/try/Mac%20Engine%20Drone/2322886/overview @jason-simmons is working on another PR to fix the Scenario App tests. mac_unopt is expected to pass again after his PR. |
Roll Flutter from 707dbc0420a3 to 23f6f5853f50 (149 revisions) flutter/flutter@707dbc0...23f6f58 2026-05-12 737941+loic-sharma@users.noreply.github.com Add 'cp: review' label to the manual cherrypick process (flutter/flutter#186158) 2026-05-12 engine-flutter-autoroll@skia.org Roll Packages from 19ec8b8 to 93cbed6 (3 revisions) (flutter/flutter#186401) 2026-05-12 30870216+gaaclarke@users.noreply.github.com Removes SDF option for macOS (always enabled) (flutter/flutter#186265) 2026-05-12 nico.reiab@gmail.com docs: fix typos in flutter_tools comments (flutter/flutter#186321) 2026-05-12 15619084+vashworth@users.noreply.github.com Pass XcodeBasedProject instead of String to functions in XcodeProjectInterpreter (flutter/flutter#186378) 2026-05-12 jason-simmons@users.noreply.github.com Update iOS scenario app test goldens to match changes from flutter/flutter#182662 (flutter/flutter#186390) 2026-05-12 engine-flutter-autoroll@skia.org Roll Skia from ad0aff15b9fa to 77a21bc723dc (2 revisions) (flutter/flutter#186396) 2026-05-12 32538273+ValentinVignal@users.noreply.github.com Migrate focus_node.unfocus.0.dart to use `RadioGroup` (flutter/flutter#183979) 2026-05-12 engine-flutter-autoroll@skia.org Roll Skia from 91d3c1e730af to ad0aff15b9fa (7 revisions) (flutter/flutter#186391) 2026-05-12 bdero@google.com [Flutter GPU] Allow customizing the vertex layout on a RenderPipeline (flutter/flutter#186310) 2026-05-12 97480502+b-luk@users.noreply.github.com Fix `EmbedderTest.CanRenderTextWithImpellerMetal` test breakage (flutter/flutter#186262) 2026-05-12 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from rFhU-YPqdCRCtCz7b... to z7ICmPtn4hspu02zk... (flutter/flutter#186384) 2026-05-12 bdero@google.com [Impeller] GLES: lazily allocate texture mip levels on first per-level write (flutter/flutter#186302) 2026-05-12 bdero@google.com [Android] Propagate --enable-flutter-gpu Intent extra to engine args (flutter/flutter#186298) 2026-05-11 47866232+chunhtai@users.noreply.github.com [ci] update no-response workflow to also look for old label name in e… (flutter/flutter#186373) 2026-05-11 bdero@google.com [ImpellerC] Write a depfile when --shader-bundle is in use (flutter/flutter#186341) 2026-05-11 nico.reiab@gmail.com docs: fix doubled-word typos in comments (flutter/flutter#186320) 2026-05-11 engine-flutter-autoroll@skia.org Roll Skia from 32281401997e to 91d3c1e730af (4 revisions) (flutter/flutter#186368) 2026-05-11 15619084+vashworth@users.noreply.github.com Show SwiftPM warnings right before iOS/macOS build (flutter/flutter#185984) 2026-05-11 15619084+vashworth@users.noreply.github.com Convert rebuilding-flutter-tool script to dart (flutter/flutter#185089) 2026-05-11 15619084+vashworth@users.noreply.github.com Use Xcode's LLDB (flutter/flutter#186273) 2026-05-11 mr-peipei@web.de Remove `currentMainUri` from `generateMainDartWithPluginRegistrant` (flutter/flutter#185907) 2026-05-11 engine-flutter-autoroll@skia.org Roll Skia from 2514f6b5f92b to 32281401997e (1 revision) (flutter/flutter#186349) 2026-05-11 engine-flutter-autoroll@skia.org Roll Packages from 92552b1 to 19ec8b8 (4 revisions) (flutter/flutter#186350) 2026-05-11 1063596+reidbaker@users.noreply.github.com Check for absolute paths in skills. (flutter/flutter#185632) 2026-05-11 engine-flutter-autoroll@skia.org Roll Skia from 9fb7d2814642 to 2514f6b5f92b (1 revision) (flutter/flutter#186347) 2026-05-11 engine-flutter-autoroll@skia.org Roll Skia from 8cafb209e836 to 9fb7d2814642 (4 revisions) (flutter/flutter#186335) 2026-05-10 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from sOBiPJb0xznDBZlf5... to rFhU-YPqdCRCtCz7b... (flutter/flutter#186328) 2026-05-10 engine-flutter-autoroll@skia.org Roll Skia from 05a03f99c74e to 8cafb209e836 (1 revision) (flutter/flutter#186315) 2026-05-10 bdero@google.com [Impeller] Vulkan: don't drop user-supplied viewport X, Y, and depth range (flutter/flutter#185886) 2026-05-09 mbrase@google.com Update Fuchsia tests to subpackage their child components (flutter/flutter#186259) 2026-05-09 victorsanniay@gmail.com Fix SelectableText crash with inline lambda contextMenuBuilder (flutter/flutter#184990) 2026-05-09 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 5_TnhTsHSqtCx37o6... to sOBiPJb0xznDBZlf5... (flutter/flutter#186289) 2026-05-09 engine-flutter-autoroll@skia.org Roll Skia from dc78d4bd2efb to 05a03f99c74e (2 revisions) (flutter/flutter#186283) 2026-05-09 22373191+Hari-07@users.noreply.github.com Improve non rect platform view rendering (flutter/flutter#182662) 2026-05-08 engine-flutter-autoroll@skia.org Roll Skia from 31521f8508c7 to dc78d4bd2efb (1 revision) (flutter/flutter#186278) 2026-05-08 30870216+gaaclarke@users.noreply.github.com Moves wide_gamut_macos to arm64 (flutter/flutter#186214) 2026-05-08 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[iOS] Migrate VSyncClient to a pure Obj-C implementation (#186166)" (flutter/flutter#186266) 2026-05-08 engine-flutter-autoroll@skia.org Roll Skia from a00db8749edb to 31521f8508c7 (2 revisions) (flutter/flutter#186264) 2026-05-08 97480502+b-luk@users.noreply.github.com Optimize compatible `DrawDiffRoundRect` calls to use `DrawRoundRect` (flutter/flutter#186203) 2026-05-08 bdero@google.com [triage] Add Flutter GPU as a triage team (flutter/flutter#186263) 2026-05-08 dmgr@google.com doc: Unified Check-Run User manual (flutter/flutter#186210) 2026-05-08 engine-flutter-autoroll@skia.org Roll Skia from 5f7adf4403d6 to a00db8749edb (1 revision) (flutter/flutter#186257) 2026-05-08 engine-flutter-autoroll@skia.org Roll Packages from 0411f1d to 92552b1 (1 revision) (flutter/flutter#186256) 2026-05-08 34871572+gmackall@users.noreply.github.com Add logging to figure out jvm crashes for `hot_mode_tests` (flutter/flutter#186107) 2026-05-08 engine-flutter-autoroll@skia.org Roll Skia from 926c09741ce2 to 5f7adf4403d6 (3 revisions) (flutter/flutter#186242) ...
Fix
EmbedderTest.CanRenderTextWithImpellerMetaltest breakage.This broke due to #186074. That PR modified how light text is rendered on macOS, but did not update this test's golden image.
This updates the image.
It also changes the test to explicitly render dark text on a light background and light text on a dark background. Previously it rendered white text on a transparent background.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.