Allow Flutter golden file tests to be flaky

[Piinks]

Fixes #111325

Adds the ability to specify Flutter framework tests as flaky, allowing them to be uploaded to Skia Gold for visual confirmation that a flake has been fixed. Prior to this change, there was no way to see if a flake had been fixed, the test would be re-enabled and then the tree would break, letting us know it was still flaky.
This will prevent a test from breaking the tree while it is in flaky mode.

Also added tech debt tracking and analyzer enforcement.

• will need to update wiki https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[Piinks]

Oook, holy yak shave batman. 🙃

Hey @yjbanov WDYT? I forked #113396 and made a few tweaks. Mainly:

• made a FlakyMixin for flutter_goldens to share for io and web
• altered some of the imports/exports to make things simpler, no need for conditional imports all over the framework when tests need to be flaky
• I made the flaky functionality single-use, if a test is flaky, use expectFlakyGolden, otherwise test normally. I didn't want folks to be confused about which method to use if there were multiple options, or migrating all golden file tests later to a special method.. meaning more imports.. etc
• Also avoided importing AsyncMatcher, no need for // ignore: implementation_imports from the other PR
• Added markedFlaky to the keys json we send to gold so it is easy to search/identify/verify flaky tests on the Gold dashboard

I did not realize so much of the web implementation was outside of flutter_goldens before, so in general thank you for yak shaving this down and making this a lot easier to work with. It took me a minute to understand this because there was a lot of web golden code I was super unfamiliar with. Having web and io together in the package feels so much better. :)

BTW - The majority of the diff is because most of flutter_goldens.dart -> src/flutter_goldens_io.dart

[yjbanov]

The golden is not flaky in all situations, but typically depends on the platform. We need some way to control when to use a flaky comparison and when not to use it. Because expectFlakyGolden always compares in flaky mode the only way to do it is via if/else containing duplicates of the statement that does the comparison:

if (isBrowser) {
  await expectFlakyGolden(
    find.byType(CupertinoDatePicker),
    'date_picker_test.time.initial.png',
  );
} else {
  await expectLater(
    find.byType(CupertinoDatePicker),
    matchesGoldenFile('date_picker_test.time.initial.png'),
  );
}

There's also the function vs matcher dichotomy, such that expectFlakyGolden cannot be a drop-in replacement for matchesGoldenFile. It requires that we use two different structures for the golden check statements.

In my previous PRs the isFlaky flag fixed both issues:

  await expectLater(
    find.byType(CupertinoDatePicker),
    matchesFlutterGolden('date_picker_test.time.initial.png', isFlaky: perspectiveTextIsFlaky),
  );

Losing this ability makes me sad, but maybe I can live with it, since there's no loss in technical capabilities.

[Piinks]

Oh right silly me. 🤦 I'll fix that.

if (perspectiveTestIsFlaky) {
  await expectFlakyGolden(
    find.byType(CupertinoDatePicker),
    'date_picker_test.time.initial.png',
  );
} else {
  await expectLater(
    find.byType(CupertinoDatePicker),
    matchesGoldenFile('date_picker_test.time.initial.png'),
  );
}

I am partial to this first case you provided. We can add expectFlakyGolden to the tech debt cost calculator and have easier follow up and maintenance. Ideally it would be nice to pass a list of platforms to filter for, but there isn't a clean way to include web with the target platform enum. I want to avoid making the soecial method have a non-flaky mode, so we don't end up with multiple types non-flaky calls expectFlakyGolden(isFlaky = false) versus matchesGoldenFile

Plus, the other with the flag required importing AsyncMatcher, which required // ignore: implementation_imports.

[yjbanov]

SGTM. I imagine we won't have too many flakes happen all at once, so the value of code deduplication is limited. Making it easier to track tech debt is a good argument too.

[yjbanov]

I see you combined all functions into the same library. I feel like expectFlakyGolden should be in a separate category from test wrappers, because it's meant to be imported in test code, unlike others. processBrowserCommand and testExecutable would be polluting the namespace. This is why in my last PR I separate them into matchers.dart and test_wrapper.dart.

[Piinks]

I feel like that will be more difficult for folks to figure out what they are supposed to import. Some files were importing test wrapper, some were importing matchers. I feel like this is cleaner.

[yjbanov]

SGTM

[yjbanov]

Ooh, nice usage of a mixin!

[yjbanov]

Would the FlakyGoldenMixin mixin allow us to deduplicate expectFlakyGolden between io and web? I think both can do this:

(goldenFileComparator as FlakyGoldenMixin).enableFlakyMode();

Of course, we'd lose the assertion above, which could be a bummer.

[Piinks]

I thought the same thing! I wondered how important the assertion is... but if it is all contained in the same mixin, maybe we don't have to assert that the right comparator class is being used? Since it is all mixed in? :)

[yjbanov]

Since this is limited to our own repo, I think we can figure it out quickly if someone attempts to use expectFlakyGolden without a flutter_test_config.dart.

[yjbanov]

By which I mean: "I agree, the assert isn't that valuable".

[flutter-dashboard]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #114450 at sha e52b022

[yjbanov]

LGTM with a couple of test coverage suggestions.

[yjbanov]

Is there a test that tests tryjobAdd with isFlaky enabled? (or maybe there is and I'm not finding it?)

[Piinks]

Added! :)

[yjbanov]

Is there a way to check that "markedFlaky" was set?

[Piinks]

Also added, thanks!

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #114450 at sha dc9e496