Enable Android dependency install test retries#12059
Enable Android dependency install test retries#12059simonrozsival with Copilot wants to merge 3 commits into
Conversation
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
Pull request overview
This PR fixes the retry behavior of InstallAndroidDependenciesTest by ensuring the test can observe build failures as false return values (instead of exceptions), allowing its existing retry loop to function as intended when SDK/JDK downloads fail transiently in CI.
Changes:
- Disable
ProjectBuilder.ThrowOnBuildFailureforInstallAndroidDependenciesTestso the three-attempt retry loop can continue after a failed build.
Show a summary per file
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidDependenciesTests.cs | Sets ThrowOnBuildFailure = false so transient dependency install failures don’t abort the retry loop via exceptions. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
There was a problem hiding this comment.
✅ LGTM
Verdict: ✅ LGTM — Clean, minimal, correct fix.
Summary
The one-line change (b.ThrowOnBuildFailure = false;) is exactly what the retry loop needs. Previously, ProjectBuilder.Build throws FailedBuildException on the first failed build (Builder.cs:414), which aborted the three-attempt loop before it could retry transient SDK/JDK download failures. Disabling ThrowOnBuildFailure lets b.Build(...) return false, so the loop's cleanup + delay + retry logic works as intended, and the final Assert.IsTrue (installSucceeded, ...) still fails the test if all attempts fail.
Issue counts: ❌ 0 ·
Verification
- ✅ Retry loop (lines 76–92) now functions correctly across all 3 attempts.
- ✅ Final assertion still enforces success.
- ✅ The final default-target build (line 121) remains guarded by
Assert.IsTrue, so disabling throw-on-failure for the whole builder doesn't mask a real failure there. - ✅ Change is scoped to a single test; no production code affected.
Positive callouts
- Good use of a clear code comment referencing the tracking issue (#11973).
- Cleanup-per-attempt (deleting/recreating
sdkPath/jdkPath) correctly avoids partial-download contamination between retries.
Nice, focused change.
Generated by Android PR Reviewer for #12059 · 65.1 AIC · ⌖ 18.5 AIC · ⊞ 6.8K
Comment /review to run again
jonathanpeppers
left a comment
There was a problem hiding this comment.
We should re-enable the ignored test in main, if this is really the fix.
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e0993a88-da38-49ca-b07c-207f4f3d470c
8e2773a to
29c22b9
Compare
InstallAndroidDependenciesTestretries transient SDK/JDK download failures, butProjectBuilderthrows on the first failed build, preventing later attempts.ThrowOnBuildFailurefor this test’s builder.