Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ci] Switch Android unit tests to LUCI #4406

Merged
merged 25 commits into from
Jul 15, 2023

Conversation

stuartmorgan
Copy link
Contributor

@stuartmorgan stuartmorgan commented Jul 7, 2023

This moves Android unit tests from Cirrus to LUCI. In order to accomplish this:

  • Switches the Android LUCI bots from JDK 11 to JDK 12, to resolve a crash when compiling camera_android unit tests with 11.
  • Adds wrappers to SDK checks where necessary for testability, since the hack to override Build.VERSION.SDK_INT in unit tests (which was already giving warnings when run with JDK 11) no longer works at all in JDK 12.

Part of flutter/flutter#114373

Enables various new LUCI targets and removes the corresponding Cirrus
versions:
- The parts of `repo_checks` that have been migrated.
- Android platform tests other than FTL.
- Web platform tests.

Since the Cirrus Android platform tests are now doing less work, the
number of shards has been reduced slightly.

Part of flutter/flutter#114373
@stuartmorgan
Copy link
Contributor Author

stuartmorgan commented Jul 13, 2023

@reidbaker What do you think of the patterns I'm introducing here?

The background here is that the version of JDK 11 we have available for LUCI has a crash when compiling the camera_android unit tests, and @yusuf-goog wasn't able to find a version of 11 that had the fix, so this updates us to JDK 12 instead. It turns out that the hack we were using in tests to swap out Build.VERSION.SDK_INT in tests completely stopped working in 12+ (in 11 it was giving us "illegal reflection" warnings about how it would be removed in the future; apparently they weren't kidding), so I replaced it with wrappers instead. I'm very open to feedback on how we structure that:

  • For simple cases I used DI for the wrapper, since I generally think DI is much better than globals. On the other hand, the thing I'm wrapping is already a global, so doing it via a global everywhere is a plausible option.
  • For camera_android, where it was spread throughout the codebase, I opted for a global, but I could probably DI everything if we want to go that route.
    • I also made named helpers, since that's something we'd talked about before; I went back and forth on having the named helpers in the global object, vs the global just being dumb and leaving individual objects to have private named wrappers. In a couple of places we needed the same conceptual check so I opted for centralized, but I don't have strong feelings about it either way.

I'm also definitely open to completely different ways of doing this if you have a preferred pattern.

@stuartmorgan stuartmorgan changed the title [ci] WIP: Try JDK 12 [ci] Switch Android unit tests to LUCI Jul 13, 2023
@stuartmorgan stuartmorgan requested a review from reidbaker July 13, 2023 11:04
.ci.yaml Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great readability improvements.

public static int SDK_VERSION = Build.VERSION.SDK_INT;

@ChecksSdkIntAtLeast(api = Build.VERSION_CODES.P)
public static boolean supportsDistortionCorrection() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find these types of classes aid in readability a ton. That said it is even better when there are breadcrumbs to the source of truth but are annoying to find during a refactor and this is already a net improvement. To that end I have added suggestions with links to the android docs for at many as I could find.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 15, 2023
@auto-submit auto-submit bot merged commit 166e2c2 into flutter:main Jul 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 17, 2023
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Jul 17, 2023
flutter/packages@369ee7e...6889cca

2023-07-17 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.20.3 to 2.20.4 (flutter/packages#4490)
2023-07-15 stuartmorgan@google.com [ci] Switch Android unit tests to LUCI (flutter/packages#4406)
2023-07-15 stuartmorgan@google.com [ci] Introduce LUCI versions of Linux desktop platform tests (flutter/packages#4223)
2023-07-14 43054281+camsim99@users.noreply.github.com [camerax] Marks all wrapped classes as immutable (flutter/packages#4451)
2023-07-14 47866232+chunhtai@users.noreply.github.com [go_router] Bumps example go_router version and migrate example code (flutter/packages#4469)

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 flutter-ecosystem@google.com,rmistry@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 31, 2023
flutter/packages@369ee7e...6889cca

2023-07-17 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.20.3 to 2.20.4 (flutter/packages#4490)
2023-07-15 stuartmorgan@google.com [ci] Switch Android unit tests to LUCI (flutter/packages#4406)
2023-07-15 stuartmorgan@google.com [ci] Introduce LUCI versions of Linux desktop platform tests (flutter/packages#4223)
2023-07-14 43054281+camsim99@users.noreply.github.com [camerax] Marks all wrapped classes as immutable (flutter/packages#4451)
2023-07-14 47866232+chunhtai@users.noreply.github.com [go_router] Bumps example go_router version and migrate example code (flutter/packages#4469)

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 flutter-ecosystem@google.com,rmistry@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants