[tests] Upload expected app size files as artifacts on test failure#25482
Conversation
It seems that the results depend on the macOS version of the machine executing the tests. That's unfortunate when all my macs are at 26.5 and the bots are at 26.2, because I can't re-generate the expected app sizes locally. So ignore these tests until I can come up with a better solution.
When WRITE_KNOWN_FAILURES is not set, instead of telling users to set the environment variable and re-run, generate a unified diff patch file that can be applied to update the expected files. The patch file is stored in a temporary directory (via Cache.CreateTemporaryDirectory) and uploaded as an Azure DevOps artifact via ##vso[artifact.upload]. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rolfbjarne
left a comment
rolfbjarne
left a comment
There was a problem hiding this comment.
✅ LGTM
Summary: Clean PR that improves UX when app size tests fail — uploads the corrected expected file as an artifact instead of just telling users to re-run with an env var. The Copilot skill provides good guidance for downloading and applying.
CI: Build passed ✓. Simulator tests still pending.
Positive callouts:
- The
UploadUpdatedExpectedFilehelper is simple and well-focused - Good that
AssertAssemblyReportgates the upload behindif (addedAPIs.Count > 0 || removedAPIs.Count > 0)so no artifact is uploaded when there's nothing wrong - Skill documentation covers both automated and manual fallback paths
0 ❌ errors · 0
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… upload The ##vso[artifact.upload] command doesn't work inside NUnit test runners because the test framework captures stdout. Instead, write files to $(Build.ArtifactStagingDirectory)/updated-expected-sizes/ and add a PublishPipelineArtifact step that uploads the directory after tests complete. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Previously, UploadUpdatedExpectedFile was only called when the total app size exceeded the tolerance. If files were added/removed from the bundle but the total size stayed within tolerance, no artifact was uploaded and the test would fail with a misleading message saying the artifact was available (when it wasn't). Restructure AssertAppSize to: 1. Detect both size differences AND file-list differences 2. Upload the updated expected file for ANY meaningful difference 3. Produce a clear failure message distinguishing file-list changes from size changes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Applied artifacts from all 4 platforms after the file-list tolerance fix. Changes include: - iOS: corrected truncated AppBundleSize + restored Info.plist entry - tvOS: restored comment line and SizeTestApp entry + updated sizes - MacCatalyst: added AppBundleSize header + NSComparisonResult.Same - macOS: restored System.Web.dll entry + updated sizes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR improves the workflow around app size (and related expected-file) test failures by writing updated “expected” outputs into Azure DevOps artifact staging so they can be published and easily downloaded/applied, and adds a GitHub Copilot skill documenting how to fetch/apply those artifacts.
Changes:
- Update
AppSizeTest.csto write updated expected files to$(Build.ArtifactStagingDirectory)/updated-expected-sizes(or a temp dir outside CI) whenever size and/or file-list/API-set differences are detected. - Add an Azure DevOps pipeline step to publish the
updated-expected-sizesdirectory as a pipeline artifact. - Refresh multiple expected
*-size.txtbaselines and add a.github/skills/workflow doc for downloading/applying the artifacts.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/devops/automation/templates/tests/run-tests.yml | Publishes a new pipeline artifact containing updated expected files. |
| tests/dotnet/UnitTests/AppSizeTest.cs | Writes updated expected files on failure (size/file-list/API diffs) to an artifact staging directory. |
| tests/dotnet/UnitTests/expected/TVOS-NativeAOT-TrimmableStatic-size.txt | Updates expected app bundle size output. |
| tests/dotnet/UnitTests/expected/TVOS-MonoVM-interpreter-size.txt | Updates expected app bundle size output. |
| tests/dotnet/UnitTests/expected/MacOSX-CoreCLR-Interpreter-TrimmableStatic-size.txt | Updates expected app bundle size output. |
| tests/dotnet/UnitTests/expected/MacCatalyst-MonoVM-size.txt | Updates expected app bundle size output. |
| tests/dotnet/UnitTests/expected/iOS-NativeAOT-size.txt | Updates expected app bundle size output. |
| tests/dotnet/UnitTests/expected/iOS-MonoVM-interpreter-size.txt | Updates expected app bundle size output. |
| .github/skills/update-expected-app-size/SKILL.md | Adds a skill guide for downloading/applying updated expected files from Azure DevOps artifacts. |
Comments suppressed due to low confidence (1)
.github/skills/update-expected-app-size/SKILL.md:116
- In the manual download section, step 4 says to place each file as
tests/dotnet/UnitTests/expected/{artifactName}.txt, butartifactNamehere refers to the pipeline artifact name (e.g.,...updated-expected-sizes-dotnettests_ios-1), not the individual expected file names inside the artifact. This should instruct the user to keep the original filenames from the artifact (e.g.,iOS-MonoVM-interpreter-size.txt) when copying intotests/dotnet/UnitTests/expected/.
If automated download fails (auth issues, etc.), provide the user with:
1. The Azure DevOps build URL
2. Instructions to navigate to the build → Summary → Artifacts section
3. Look for individual artifacts whose names match the patterns above
4. Download each file and place it as `tests/dotnet/UnitTests/expected/{artifactName}.txt`
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #d022761] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #d022761] Build passed (Build packages) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [PR Build #d022761] Build passed (Build macOS tests) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #d022761] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 183 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
When app size tests fail in Azure DevOps, write the updated expected files to
$(Build.ArtifactStagingDirectory)/updated-expected-sizes/so they get published as a pipeline artifact. This makes it easy to download and apply the corrected files without having to reproduce the build locally.Also add a Copilot skill that can fetch the artifacts and apply them locally.
Changes:
PublishPipelineArtifact@1step to upload theupdated-expected-sizesdirectory after tests complete..github/skills/update-expected-app-size/): Helps download these artifacts from Azure DevOps and apply them locally.🤖 Pull request created by Copilot