Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal #139549

Hixie · 2023-12-05T09:10:11Z

Remove all use of global variables.
Always pass in all dependencies, only create them in main or in tests.
Pass in the "print" primitive.
Make all network traffic retry (except when run locally, when it just auto-passes).
Enable tests to be run in random order.

Hixie · 2023-12-05T09:15:54Z

cc @Piinks
sorry, this project got a bit out of hand

Piinks

sorry, this project got a bit out of hand

Sorry? I am glad for it, thank you! CI isn't though...

Hixie · 2023-12-06T07:44:05Z

@Piinks fixed :-)

Piinks

Works in presubmit for sure - images received: https://flutter-gold.skia.org/search?crs=github&issue=139549&master=true&patchsets=1&positive=true

Could we leave the tests as untouched as possible as a first order change, updating only for the parts of the implementation that is changing here? Just thinking about the opportunities for regressions here changing so much all at once.

Also, conflict was added in #139706 (sorry! Needed to fix the tree). Git will likely be unfriendly trying to merge, but it was a small change in determining which comparator to use (isRecommendedForEnvironment). May be easier to add the condition and ignore what git tries to do to reconcile.

packages/flutter_goldens/lib/flutter_goldens.dart

packages/flutter_goldens/lib/skia_client.dart

Piinks · 2023-12-08T18:08:11Z

packages/flutter_goldens/lib/skia_client.dart

getExpectationForTest is only called by the local comparator, which it calls with retry false. What scenario retries here?

good catch. (i was just adding the retry logic everywhere, didn't think of the implication here)

packages/flutter_goldens/test/flutter_goldens_test.dart

Piinks · 2023-12-08T18:16:23Z

packages/flutter_goldens/lib/skia_client.dart

Why is this not needed anymore?

As far as I can tell, it was never needed or used. The overrides were empty and not exposed to tests.

Took me a sec to recall - this was not for the tests in this package, but the actual framework tests that were generating golden files.

While under test, http requests for getExpectationForTest and getImageBytes could not be made without these overrides. I remember it being such a pain at the time to figure out this out. These were only being used by the local comparator, has this branch run the framework tests locally to see that the golden file tests work correctly?

It's all mocked out in the package's tests so I don't think they'll capture it if it broke. Not sure. TBH all of this was so long ago.

gotcha. running the framework tests locally to see. I'm hoping the new way of injecting an HttpClient makes it moot...

Yeah I studied the code and what's going on is that now that we create the HttpClient before we call test, the overrides from the test framework aren't in effect.

packages/flutter_goldens/lib/flutter_goldens.dart

Piinks

Just recalled as well - I think the engine implementation copied much of the code that was here. They probably have the same 502 issue from time to time.

Hixie · 2023-12-09T00:04:03Z

Just recalled as well - I think the engine implementation copied much of the code that was here. They probably have the same 502 issue from time to time.

Ah yes, found the relevant code. Let's see if this fixes things for flutter/flutter, and if so, we can suggest porting the fixes over to engine too.

Piinks

LGTM with just a couple nits.
The HttpOverrides question can be checked with a local run of framework tests. The overrides may not be needed anymore, I would not be surprised if a change made that so over the years. :)

Piinks · 2023-12-12T20:24:41Z

packages/flutter_goldens/test/comparator_selection_test.dart

This whole file is just lovely.

Piinks · 2023-12-12T20:28:14Z

packages/flutter_goldens/test/flutter_goldens_test.dart

Nit: retry can be removed

Piinks · 2023-12-12T20:28:29Z

dev/automated_tests/flutter_test/flutter_gold_test.dart

Here too, retry can be removed

This reverts commit 91ea024.

* Make all network traffic retry (except locally). This works around https://g-issues.skia.org/issues/40044713 using exponential backoff. * Remove all use of global variables. * Always pass in all dependencies, only create them in main or in tests. * Pass in the "print" primitive to avoid stray prints to the console in tests.

…flutter_goldens for extensive technical debt removal (flutter/flutter#139549)

Roll Flutter from 9719097 to 11a9cb7 (32 revisions) flutter/flutter@9719097...11a9cb7 2023-12-13 [email protected] Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal (flutter/flutter#139549) 2023-12-12 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.9 to 2.22.10 (flutter/flutter#140003) 2023-12-12 [email protected] Allow plugins to use compileSdkPreview (flutter/flutter#131901) 2023-12-12 [email protected] Roll pub packages (flutter/flutter#139995) 2023-12-12 [email protected] Select simulator runtime for tests based on Xcode's preferred runtime build (flutter/flutter#139919) 2023-12-12 [email protected] Roll Flutter Engine from 0c527aa1a215 to 9039ac78cf03 (1 revision) (flutter/flutter#139992) 2023-12-12 [email protected] [Docs] Added missing `CupertinoApp.showSemanticsDebugger` (flutter/flutter#139913) 2023-12-12 [email protected] Roll Flutter Engine from 444281eb5c7c to 0c527aa1a215 (2 revisions) (flutter/flutter#139985) 2023-12-12 [email protected] Update Gallery lockfiles for the new version of the video_player plugin (flutter/flutter#139832) 2023-12-12 [email protected] Roll Packages from cb6dbcd to 80aa46a (5 revisions) (flutter/flutter#139982) 2023-12-12 [email protected] Roll Flutter Engine from 3b77b1b7b42f to 444281eb5c7c (1 revision) (flutter/flutter#139979) 2023-12-12 [email protected] Roll Flutter Engine from f8e87ed193f5 to 3b77b1b7b42f (1 revision) (flutter/flutter#139977) 2023-12-12 [email protected] Roll pub packages (flutter/flutter#139969) 2023-12-12 [email protected] Roll Flutter Engine from 4102c7daf1d3 to f8e87ed193f5 (3 revisions) (flutter/flutter#139963) 2023-12-12 [email protected] Roll Flutter Engine from 95dfb1d4ac75 to 4102c7daf1d3 (1 revision) (flutter/flutter#139961) 2023-12-12 [email protected] Roll Flutter Engine from 40bfd2dc1519 to 95dfb1d4ac75 (1 revision) (flutter/flutter#139959) 2023-12-12 [email protected] Roll Flutter Engine from 061ae7023f10 to 40bfd2dc1519 (2 revisions) (flutter/flutter#139958) 2023-12-12 [email protected] Roll Flutter Engine from 75cfb050cd9a to 061ae7023f10 (1 revision) (flutter/flutter#139955) 2023-12-12 [email protected] Roll Flutter Engine from 362d0cb3ab27 to 75cfb050cd9a (1 revision) (flutter/flutter#139954) 2023-12-12 [email protected] Fix dayPeriodColor handling of non-MaterialStateColors (flutter/flutter#139845) 2023-12-12 [email protected] Roll Flutter Engine from ea1a3069e057 to 362d0cb3ab27 (1 revision) (flutter/flutter#139951) 2023-12-12 [email protected] [github actions] Automate Flutter Chery Picks (flutter/flutter#139524) 2023-12-12 [email protected] Roll Flutter Engine from d001419b436e to ea1a3069e057 (5 revisions) (flutter/flutter#139948) 2023-12-12 [email protected] [flutter_tools] catch SocketException writing to ios-deploy stdin (flutter/flutter#139784) 2023-12-11 [email protected] [ci.yaml] Add missing ci.yaml to runIf of android hot reload tests (flutter/flutter#139932) 2023-12-11 [email protected] make the tar c command in prepare_package.dart verbose (flutter/flutter#139687) 2023-12-11 [email protected] Roll Flutter Engine from 5c1f13e1e535 to d001419b436e (4 revisions) (flutter/flutter#139941) 2023-12-11 [email protected] fix typo of 'not' instead of 'now' for `useInheritedMediaQuery` (flutter/flutter#139940) 2023-12-11 [email protected] Roll Flutter Engine from 4c309195b79d to 5c1f13e1e535 (2 revisions) (flutter/flutter#139939) 2023-12-11 [email protected] Implement `switch` expressions in `examples/` and `animation/` (flutter/flutter#139882) 2023-12-11 [email protected] Renamed `appbar` to `app_bar` directory in API Examples Tests (flutter/flutter#139922) 2023-12-11 [email protected] Deprecate `RawKeyEvent`, `RawKeyboard`, et al. (flutter/flutter#136677) 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 Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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 ...

…flutter_goldens for extensive technical debt removal (#139549)" This reverts commit 11a9cb7.

… flutter_goldens for extensive technical debt removal" (#140085) Reverts #139549 Initiated by: Piinks This change reverts the following previous change: Original Description: * Remove all use of global variables. * Always pass in all dependencies, only create them in main or in tests. * Pass in the "print" primitive. * Make all network traffic retry (except when run locally, when it just auto-passes). * Enable tests to be run in random order.

…goldens for extensive technical debt removal Originally landed in flutter#139549 Originally reverted in flutter#140085

…goldens for extensive technical debt removal (#140101) Originally landed in #139549 Originally reverted in #140085 - Remove all use of global variables. - Always pass in all dependencies, only create them in main or in tests. - Pass in the "print" primitive. - Make all network traffic retry (except when run locally, when it just auto-passes). - Enable tests to be run in random order. - Better error messages

…goldens for extensive technical debt removal (flutter#140101) Originally landed in flutter#139549 Originally reverted in flutter#140085 - Remove all use of global variables. - Always pass in all dependencies, only create them in main or in tests. - Pass in the "print" primitive. - Make all network traffic retry (except when run locally, when it just auto-passes). - Enable tests to be run in random order. - Better error messages

…flutter_goldens for extensive technical debt removal (flutter/flutter#139549)

github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Dec 5, 2023

Piinks reviewed Dec 5, 2023

View reviewed changes

Hixie force-pushed the gold502 branch 2 times, most recently from 2627792 to 2828ab7 Compare December 6, 2023 06:48

Hixie force-pushed the gold502 branch 4 times, most recently from 4949ef9 to 212798f Compare December 6, 2023 21:21

Piinks reviewed Dec 8, 2023

View reviewed changes

Hixie force-pushed the gold502 branch 2 times, most recently from 0c9f9a4 to 5918e51 Compare December 8, 2023 23:59

Hixie force-pushed the gold502 branch from 5918e51 to 175a145 Compare December 9, 2023 00:18

Piinks approved these changes Dec 12, 2023

View reviewed changes

Hixie added 4 commits December 12, 2023 15:26

Revert "Don't send images to Gold on release branches (flutter#139706)"

b2ad1b7

This reverts commit 91ea024.

Merge in flutter#139706

b7e1a39

review comments and test fixes

b27baeb

Hixie force-pushed the gold502 branch from 175a145 to b27baeb Compare December 12, 2023 23:29

Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 13, 2023

auto-submit bot merged commit 11a9cb7 into flutter:master Dec 13, 2023

engine-flutter-autoroll mentioned this pull request Dec 13, 2023

Roll Flutter from 97190973d6f5 to 11a9cb7029d7 (32 revisions) flutter/packages#5665

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 13, 2023

11a9cb7 Make tests more resilient to Skia gold failures and refactor …

62c9ec5

…flutter_goldens for extensive technical debt removal (flutter/flutter#139549)

Piinks added the revert Autorevert PR (with "Reason for revert:" comment) label Dec 13, 2023

auto-submit bot mentioned this pull request Dec 13, 2023

Reverts "Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal" #140085

Merged

auto-submit bot pushed a commit that referenced this pull request Dec 13, 2023

Revert "Make tests more resilient to Skia gold failures and refactor …

3394901

…flutter_goldens for extensive technical debt removal (#139549)" This reverts commit 11a9cb7.

auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Dec 13, 2023

Hixie mentioned this pull request Dec 13, 2023

Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal #140101

Merged

Hixie added a commit to Hixie/flutter that referenced this pull request Dec 21, 2023

Make tests more resilient to Skia gold failures and refactor flutter_…

e8270de

…goldens for extensive technical debt removal Originally landed in flutter#139549 Originally reverted in flutter#140085

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024

11a9cb7 Make tests more resilient to Skia gold failures and refactor …

0c7f8dc

…flutter_goldens for extensive technical debt removal (flutter/flutter#139549)

Piinks mentioned this pull request Mar 22, 2024

Fix skipping golden comparator for CI environments #145619

Merged

9 tasks

Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal #139549

Make tests more resilient to Skia gold failures and refactor flutter_goldens for extensive technical debt removal #139549

Uh oh!

Conversation

Hixie commented Dec 5, 2023

Uh oh!

Hixie commented Dec 5, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Hixie commented Dec 6, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Piinks Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Hixie commented Dec 9, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Piinks Dec 8, 2023 •

edited

Loading