Temporarily Disabling Flaky Integration Tests in Plugin Example Apps

[jesswrd]

Temporarily skipping flaky integration tests in plugin example apps for camera, camera_android, and camera_android_camerax to safely land #8037.

The flakes are unrelated to the mentioned PR and can likley be traced back to the following issues:
flutter/flutter#154682
flutter/flutter#157556

Partially addresses flutter/flutter#152656

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.
• 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.

[matanlurey]

Let's leave a TODO linking to an issue to renable these tests (could repurpose the ones you have already for Camille), i.e. at every line where you wrote skip: true. There should be at least one example in the repo if you want a reference.

This is important because otherwise we permanently skip test coverage which can have worse implications down the line.

Thanks for creating the PR!

[jesswrd]

Thanks for the explanation! done.

[camsim99]

LGTM! Thanks for this :)

[stuartmorgan]

Are we sure these are flaky tests? The referenced PR is creating a situation that one of the referenced issues says will break the test, and the PR appears to be failing every Android camera package, even with retries. What's the evidence that this is pre-existing flake rather than the PR actually breaking the tests?

[camsim99]

@stuartmorgan can confirm this test is broken by something in the plugin not flaky and should be disabled until I can investigate.

flutter/flutter#154682 has evidence of it failing locally in the past.
b/376074365 has further evidence of this previously being broken.

I know these don't reference camera but if those run on Android expect them to fail too because I'm guessing that CameraX doesn't support lower resolutions anymore but need to look into it this week. I believe the test only appears to be flaky because it doesn't run all of the time.

If you need more proof, let me know and I can validate its broken-ness to help unblock #8037!

[camsim99]

Ah I just noticed #8037 (comment). It's possible that the bugs I've seen are related to bumping the target SDK version (though I don't think the b/376074365 is; it appears it's even more broken after the target SDK bump).

@stuartmorgan would a good compromise be not bumping the target SDK version until I fix the tests? @jesswrd not sure if your change #8037 relies on that or not.

[jesswrd]

@stuartmorgan would a good compromise be not bumping the target SDK version until I fix the tests? @jesswrd not sure if your change #8037 relies on that or not.

@camsim99 I can't merge without bumping the target sdk version. Google Play requires a target sdk version of at least 33, so I upgraded the target sdk version for camera, camera_android, and camera_android_camera_androidx. I get an error if I don't.

@stuartmorgan Given the above, I think I should disable the tests, but let me know how you'd like me to proceed.

[stuartmorgan]

flutter/flutter#154682 has evidence of it failing locally in the past.

Right, but it was failing in a PR that tried to bump the SDK version. And so the camera part was backed out of that PR. That's not flake, that's deterministic failure under the same conditions as the new PR.

It just seems like there's a mismatch here, where this PR describes the failures as "flakes [...] unrelated to the mentioned PR", but what I'm seeing is significant evidence that the failures are in fact directly caused by that PR, for reasons that have been previously established last time we tried to roll the target SDK.

I believe the test only appears to be flaky because it doesn't run all of the time.

It runs any time anyone touches camera or and android sub-package, and also any time all tests run (such as every roll). If it were failing consistently on main, we would definitely know.

AFAICT the state is that it's fine on main, but attempting to roll the target SDK forward will break it. If we believe that the test is wrong and we want to disable it to roll the target SDK forward we can, but we should be clear that we are doing that—explicitly disabling a working test in order to land a PR that breaks that test—not disabling a flaky test. (And I would suggest we do the disabling in the PR that breaks the tests, in that case.)

[camsim99]

AFAICT the state is that it's fine on main, but attempting to roll the target SDK forward will break it. If we believe that the test is wrong and we want to disable it to roll the target SDK forward we can, but we should be clear that we are doing that—explicitly disabling a working test in order to land a PR that breaks that test—not disabling a flaky test. (And I would suggest we do the disabling in the PR that breaks the tests, in that case.)

This SGTM!

[jesswrd]

AFAICT the state is that it's fine on main, but attempting to roll the target SDK forward will break it. If we believe that the test is wrong and we want to disable it to roll the target SDK forward we can, but we should be clear that we are doing that—explicitly disabling a working test in order to land a PR that breaks that test—not disabling a flaky test. (And I would suggest we do the disabling in the PR that breaks the tests, in that case.)

Sounds good. I'll disable the working tests in #8037 and add a note in the PR description.