[rfw] Increase tolerance for material widget tests #7148
Conversation
victoreronmosele
force-pushed
the
fix-rfw-tests
branch
4 times, most recently
from
3781e87 to
1935f04
Compare
victoreronmosele
force-pushed
the
fix-rfw-tests
branch
from
1935f04 to
6439bf6
Compare
| @@ -23,6 +25,9 @@ void main() { | |||
| } | |||
|
|
|||
| testWidgets('Material widgets', (WidgetTester tester) async { | |||
| setUpTolerantComparator( | |||
| testPath: 'test/material_widget_test.dart', precisionTolerance: 0.01); | |||
There was a problem hiding this comment.
0.01 is 4800px out of the 480000px in the material_test.scaffold.png file.
The actual diff from the failing test is 4px so a more precise value for the tolerance is 0.00000833333.
There was a problem hiding this comment.
Meaning there are four pixels that have a non-identical match to the control?
There was a problem hiding this comment.
I worry that allowing four pixels to be just flat out wrong means that if we start incorrectly rendering a dot somewhere, we won't notice it. "Tolerance" makes it sound like we'd accept it if the four pixels happened to be slightly off in color (e.g. if 0xFFFEFEFE became 0xFFAFAFA or something), not that we'd accept wildly wrong renderings (e.g. 0xFF00FF00 becoming 0xFFFF0000).
There was a problem hiding this comment.
True. What would be a better fix for the issue? Updating the control images?
Also, I implemented the tolerance based on this comment in the original issue: flutter/flutter#151611 (comment).
There was a problem hiding this comment.
Updating the control images is a short-term fix, it doesn't really address the underlying issue (which is that skia and impeller don't guarantee pixel-identical rendering across versions for things like antialiasing and blurs, but the test is assuming they do).
I don't have a good solution. Setting the tolerance to something like 0.00002 (so if I understood correctly, allowing about 10 pixels to drift from the control) might be ok I guess? In the absence of any better ideas...
There was a problem hiding this comment.
Yes, 10px away from the control.
I'll update the tolerance.
There was a problem hiding this comment.
Updated now in 3306ea0.
|
Do we understand what kinds of changes are causing failures here? Is it anti-aliasing? Blurs? Something else? |
I can't tell which one it is, but I generated the test failure artifacts in a GitHub Actions workflow run here. Also, I pushed an update to the PR to set up the tolerant comparator for all the tests in the file since the failure was occurring in multiple tests. |
victoreronmosele
force-pushed
the
fix-rfw-tests
branch
from
3ff15da to
1e616f8
Compare
|
I see nothing different in those diffs at all. :-/ |
|
Cropping and zooming in on the images show some changes in the diffs. See for
Original 
|
victoreronmosele
force-pushed
the
fix-rfw-tests
branch
3 times, most recently
from
f00c494 to
15baa90
Compare
victoreronmosele
force-pushed
the
fix-rfw-tests
branch
from
15baa90 to
3306ea0
Compare
|
The referenced issue is closed; is this obsolete, or is changing the allowed diff still something we want to do? |
|
@stuartmorgan, the main issue is flutter/flutter#151659. It is still open. flutter/flutter#151659 says to fix and enable the This PR is doing the fix. If this gets merged, another PR should be raised to enable the actual tests in the flutter/tests repo. |
|
From triage: @Hixie, what is the next step here from a reviewer standpoint? |
There was a problem hiding this comment.
Sorry, this fell of my radar.
I guess LGTM, but I'm a bit skeptical of this approach. I would prefer to find ways to make Flutter not change rendering across versions (e.g. the way we do for shadows, where in golden tests the shadows become big black rectangles).
stuartmorgan
added
the
autosubmit
flutter/packages@05bf1d4...bb00d34 2024-10-07 [email protected] [google_sign_in] Update Pigeon for non-nullable generics (flutter/packages#7785) 2024-10-07 [email protected] [path_provider] Update Android Pigeon for non-nullable generics (flutter/packages#7783) 2024-10-07 [email protected] [rfw] Increase tolerance for material widget tests (flutter/packages#7148) 2024-10-05 [email protected] [various] Update Java compatibility version to 11 (flutter/packages#7795) 2024-10-04 [email protected] [video_player] Update Pigeon for non-nullable generics (flutter/packages#7790) 2024-10-04 [email protected] [go_router] Added missing implementation for the routerNeglect parameter in GoRouter (flutter/packages#7752) 2024-10-04 [email protected] [google_maps_flutter_platform_interface] Convert `BitmapDescriptor` to typesafe subclasses (flutter/packages#7699) 2024-10-04 [email protected] [google_maps_flutter_platform_interface] Convert `PatternItem` and `Cap` to typesafe structures. (flutter/packages#7703) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This PR introduces a tolerance for rfw material widget tests to fix the golden test error in flutter/flutter#151611.
Part of a fix for flutter/flutter#151659.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.