Repository navigation
[flaky-ci] Stabilize BenchmarkDotNet device test results - #12718
Conversation
Use instrumentation results for benchmark and argument assertions so dropped logcat output cannot fail the test. Add structured diagnostics for discovery, report success, and validation errors.\n\nCo-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
simonrozsival
added
the
flaky-ci
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
The structured instrumentation-result approach correctly removes the test dependency on nondeterministic logcat delivery, and the added assertions cover argument forwarding, benchmark discovery, report success, and validation errors. I found 0 errors, 0 warnings, and 1 suggestion (inline) to improve parser failure diagnostics.
CI is currently red: Azure build 1586987 reports failures in Package Tests macOS > Tests > APKs 1 and MAUI Tests MAUI Integration. The remaining 41 checks passed. I could not determine whether those two failures are flaky/infrastructure-related because Azure CLI could not initialize in this runner, so CI must be investigated or rerun before merge.
Generated by Android PR Reviewer for #12718 · gpt56 · 260.3 AIC · ⌖ 3.76 AIC · ⊞ 27.7K
Comment /review to run again
Report missing keys separately from malformed integer values so output-path failures remain actionable. Co-authored-by: Copilot App <[email protected]>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors · 1 warning · 0 suggestions.
The structured instrumentation bundle is a sound replacement for timing-sensitive logcat assertions, and the added success/report/validation checks strengthen the test. One diagnostic-ordering issue remains: exception runs can fail while parsing absent success keys before the test checks the instrumentation result code.
CI build #1587831 is currently red in the Linux, macOS, and Windows build lanes. The Azure CLI could not read the public timeline in this runner because its configured profile location is not writable, so I could not determine whether those failures are related to this PR.
Generated by Android PR Reviewer for #12718 · gpt56 · 222.7 AIC · ⌖ 13.6 AIC · ⊞ 25.7K
Comment /review to run again
Co-authored-by: Copilot App <[email protected]>
Check the instrumentation and process completion status before parsing success-only result values, and surface the instrumentation error bundle when execution is canceled. Co-authored-by: Copilot App <[email protected]>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new instrumentation-result parsing trims extracted values, which can mutate the “authoritative” forwarded args/greeting and undermine the stated goal of returning exact values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs — TryParseInstrumentationStringResult() trims the whole line and the extracted value. That can… |
What changed in this PR
This PR updates the DotNetRunBenchmarkDotNet MSBuild device integration test to avoid flakiness caused by nondeterministic delivery of app logcat markers when am instrument exits, by treating structured instrumentation bundle results as the authoritative signal.
Changes:
- Extend the in-app
Instrumentationto emit benchmark/run metadata (args/greeting, benchmark/report counts, validation error counts) viaINSTRUMENTATION_RESULT. - Update the test to assert on
INSTRUMENTATION_RESULTkeys instead of relying on app logcat markers, while still printing logcat-marker presence as diagnostics. - Refactor parsing helpers to support both integer and string instrumentation result values.
| File | Description |
|---|---|
| tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs | Makes BenchmarkDotNet device-test assertions rely on instrumentation results rather than app logcat, and adds additional structured result keys for stability/diagnostics. |
| foreach (var rawLine in output.Split ('\n')) { | ||
| var line = rawLine.Trim (); | ||
| if (line.StartsWith (prefix, StringComparison.Ordinal)) { | ||
| var valueStr = line.Substring (prefix.Length).Trim (); | ||
| if (int.TryParse (valueStr, out int value)) | ||
| return value; | ||
| return line.Substring (prefix.Length).Trim (); | ||
| } |
Fixes #12704
Tracker: #12704
Focused tracker: N/A
Why
InstallAndRunTests.DotNetRunBenchmarkDotNetfailed in builds 1573240 and 1580340 even though the app built, installed, launched, ran BenchmarkDotNet, returnedreports=1, and finished instrumentation successfully. Only app logcat events were missing. Local repeated validation reproduced a run with exit code 0 and complete successful instrumentation results while both app logcat markers were absent, confirming that logcat delivery—not build, installation, launch, benchmark startup, device performance, parsing, or timeout behavior—was nondeterministic.Microsoft.Android.Runstops its logcat reader whenam instrumentexits, so assertions must not depend on app logcat arriving before that shutdown.What changed
argsandgreetingvalues in the instrumentation result bundle.Validation
./dotnet-local.sh build tests/MSBuildDeviceIntegration/MSBuildDeviceIntegration.csproj -c Debug -v:minimalDotNetRunBenchmarkDotNetpassed three consecutive emulator runs.A pre-fix repetition reproduced missing app logcat with successful structured benchmark results.
Useful description of why the change is necessary
Links to issues fixed
Focused device integration coverage