Skip to content

[tests] Verbose apkdiff output for BuildReleaseArm64 regressions #11759

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.

Sign up for GitHub

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

Closed
simonrozsival wants to merge 1 commit into from
Closed

[tests] Verbose apkdiff output for BuildReleaseArm64 regressions #11759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 72 additions & 1 deletion src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Reflection;
using System.Runtime.InteropServices;
using System.Text;
using System.Text.Json;
using System.Threading.Tasks;
using System.Xml;
using System.Xml.Linq;
Expand Down Expand Up @@ -313,8 +314,78 @@ public void BuildReleaseArm64 ([Values] bool forms, [Values (AndroidRuntime.Core
var apkDescPath = Path.Combine (Root, apkDescFilename);
var apkDescReferencePath = Path.Combine (Root, b.ProjectDirectory, apkDescReference);
var (code, stdOut, stdErr) = RunApkDiffCommand ($"-s --save-description-2={apkDescPath} --descrease-is-regression {regressionCheckArgs} {apkDescReferencePath} {apkFile}", Path.Combine (Root, b.ProjectDirectory, "apkdiff.log"));
Comment thread
simonrozsival marked this conversation as resolved.
Assert.IsTrue (code == 0, $"apkdiff regression test failed with exit code: {code}. See test attachments.");
if (code != 0) {
if (File.Exists (apkDescPath)) {
TestContext.AddTestAttachment (apkDescPath, apkDescFilename);
}

var message = new StringBuilder ();
message.AppendLine ($"apkdiff regression test failed with exit code: {code}.");
message.AppendLine ();
message.AppendLine ("== apkdiff output ==");
if (!stdOut.IsNullOrEmpty ()) {
message.AppendLine (stdOut);
}
if (!stdErr.IsNullOrEmpty ()) {
message.AppendLine (stdErr);
}
message.AppendLine ();
message.AppendLine ("== .apkdesc diff (reference -> current) ==");
message.AppendLine (GetApkDescDiff (apkDescReferencePath, apkDescPath));

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 ⚠️ Error handling — On failure, GetApkDescDiff → ReadApkDescEntries runs JsonDocument.Parse (File.ReadAllText (path)) with no try/catch. If either .apkdesc exists but is unreadable or malformed JSON, the exception is thrown here — before Assert.Fail (...) on line 337 — so the test fails with a raw JsonException/IOException and the entire rich message (including the apkdiff stdOut/stdErr already appended just above) is discarded, making diagnostics worse than the original one-line assert. This is exactly the failure path the feature targets: when apkdiff exits non-zero it may have written a truncated/partial current .apkdesc. The PR description says the helper "degrades gracefully ... instead of throwing," but only missing files are guarded — not unreadable/malformed ones. Suggest wrapping the body of GetApkDescDiff in a try/catch that returns a placeholder, e.g. $"(failed to compute apkdesc diff: {ex.Message})".

Rule: Diagnostics must degrade gracefully, not throw

message.AppendLine ();
message.AppendLine ($"If this change is intended, update the reference '{apkDescFilename}' with the current '.apkdesc' attached to this test (or run build-tools/scripts/UpdateApkSizeReference.sh).");
Assert.Fail (message.ToString ());
}
}
}

static string GetApkDescDiff (string referencePath, string currentPath)
{
if (!File.Exists (referencePath)) {
return $"(reference apkdesc not found: {referencePath})";
}
if (!File.Exists (currentPath)) {
return $"(current apkdesc not found: {currentPath})";
}

var reference = ReadApkDescEntries (referencePath);
var current = ReadApkDescEntries (currentPath);

var sb = new StringBuilder ();
foreach (var key in reference.Keys.Union (current.Keys).OrderBy (k => k, StringComparer.Ordinal)) {
bool inReference = reference.TryGetValue (key, out long oldSize);
bool inCurrent = current.TryGetValue (key, out long newSize);
if (inReference && inCurrent) {
if (oldSize != newSize) {
sb.AppendLine ($" {key}");
sb.AppendLine ($"- \"Size\": {oldSize}");
sb.AppendLine ($"+ \"Size\": {newSize} ({(newSize - oldSize):+#,0;-#,0;0} bytes)");

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 💡 Testing — The byte-delta format {(newSize - oldSize):+#,0;-#,0;0} uses the current culture's group separator, so the same regression prints +1,024 on an en-US agent but +1.024 on a de-DE one. For stable, copy-pasteable diagnostics regardless of the CI machine's locale, format with CultureInfo.InvariantCulture — e.g. (newSize - oldSize).ToString ("+#,0;-#,0;0", CultureInfo.InvariantCulture) (needs using System.Globalization;). Optional, since CI is typically invariant/en-US.

Rule: Deterministic, locale-independent test output

}
} else if (inReference) {
sb.AppendLine ($"- {key} (\"Size\": {oldSize}) [removed]");
} else {
sb.AppendLine ($"+ {key} (\"Size\": {newSize}) [added]");
}
}

if (sb.Length == 0) {
return "(no per-entry size differences)";
}
return sb.ToString ();
}

static Dictionary<string, long> ReadApkDescEntries (string path)
{
var result = new Dictionary<string, long> (StringComparer.Ordinal);
using var doc = JsonDocument.Parse (File.ReadAllText (path));
if (doc.RootElement.TryGetProperty ("Entries", out var entries)) {
foreach (var entry in entries.EnumerateObject ()) {
if (entry.Value.TryGetProperty ("Size", out var size)) {
result [entry.Name] = size.GetInt64 ();
}
}
}
return result;
}

static IEnumerable<object[]> Get_BuildHasNoWarningsData ()
Expand Down
1 change: 1 addition & 0 deletions src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/BaseTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ protected static (int code, string stdOutput, string stdError) RunApkDiffCommand
$"\ncontext: https://github.com/xamarin/xamarin-android/blob/main/Documentation/project-docs/ApkSizeRegressionChecks.md" +
$"\nstdOut:\n{result.stdOutput}\nstdErr:\n{result.stdError}";
File.WriteAllText (logFilePath, logContent);
TestContext.AddTestAttachment (logFilePath, Path.GetFileName (logFilePath));
return result;
}

Expand Down
Loading