[mobile] Skip publishing tests ignored by CI#131071
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7a6a3acd-96c4-46b9-9dac-d21804d4576a
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the mobile test publishing pipeline so that projects marked IgnoreForCI=true no longer execute the PublishTestAsSelfContained target. This aligns publishing behavior with existing “skip ignored tests” behavior already applied to other mobile-test steps (e.g., archive/bundle paths).
Changes:
- Gate
PublishTestAsSelfContainedonIgnoreForCI != trueso ignored projects are not published afterBuild. - Prevent downstream publish-time actions (including trimming/ILLink) from running for ignored test projects in mobile builds.
Show a summary per file
| File | Description |
|---|---|
| eng/testing/tests.mobile.targets | Adds an IgnoreForCI condition to PublishTestAsSelfContained to skip publishing ignored test projects. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
|
/azp run runtime-ioslike |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "33f2b989b3c516f1d0bd49fdac86b537db669efa",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "d349aa7988e9adf0ee0f5f0473886c0582975955",
"last_reviewed_commit": "33f2b989b3c516f1d0bd49fdac86b537db669efa",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "d349aa7988e9adf0ee0f5f0473886c0582975955",
"last_recorded_worker_run_id": "29741176640",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "33f2b989b3c516f1d0bd49fdac86b537db669efa",
"review_id": 4734866719
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: The problem is real and well-evidenced. Mobile builds already skip archiving/bundling IgnoreForCI test projects, but PublishTestAsSelfContained still ran after Build, triggering aggressive trimming (ILLink) for tests unsupported on the target platform (e.g. System.Text.Json.SourceGeneration.Roslyn4.4.Tests on the Apple mobile CoreCLR leg). A CI failure log is linked, so the change is justified.
Approach: Correct and minimal. Adding and '$(IgnoreForCI)' != 'true' to the PublishTestAsSelfContained target condition mirrors the guard already applied to the sibling PublishDir/BundleDir properties (lines 6-8) and to the archiving targets in tests.targets, tests.android.targets, and tests.ioslike.targets. The target's DependsOnTargets (ArchiveTests, $(BundleTestAppTargets)) are already no-ops for IgnoreForCI projects, so skipping the publish step for them is consistent and avoids wasted trimming work rather than masking an error.
Summary: ✅ LGTM. A one-line, low-risk build-infra fix that brings the publish target in line with the established IgnoreForCI handling across the mobile testing targets. No correctness, performance, or scope concerns; no new public API surface. No actionable findings.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 41.2 AIC · ⌖ 10.2 AIC · ⊞ 10K
|
|
||
| <Target Name="PublishTestAsSelfContained" | ||
| Condition="'$(IsCrossTargetingBuild)' != 'true'" | ||
| Condition="'$(IsCrossTargetingBuild)' != 'true' and '$(IgnoreForCI)' != 'true'" |
There was a problem hiding this comment.
Is this caused by a specific test that was disabled recently or do you know why we started hitting this now?
There was a problem hiding this comment.
No, this is not a disabled test, System.Text.Json.SourceGeneration.Roslyn4.4.Tests has always been IgnoreForCI on mobile, so it never runs on the Apple mobile CoreCLR legs, but it was still published and trimmed
What actually fails is ILLink crashing during that publish trim with an unhandled ArgumentOutOfRangeException in Mono.Cecil.
The crash first appears in the https://dev.azure.com/dnceng-public/public/_build/results?buildId=1515988, which points to #130164 and #130665, each added a [JsonSerializable(typeof(...))] entry to the TestTypesContext source-generated context that these tests compile, which could have caused the error.
I will create a tracking issue if it doesn't exist.
There was a problem hiding this comment.
I'm wondering if this a real issue not just a CI thing which warrant a bigger attention. Could it be something with trimmer that is broken on mobile @vitek-karas ?
There was a problem hiding this comment.
It is a latent Cecil bug that only appears once the assembly grows past a certain size. I believe it is worth further investigation and don't think it is mobile-specific.
|
Approving to unblock the CI |
Description
Skip
PublishTestAsSelfContainedwhenIgnoreForCIevaluates totrue. Mobile builds currently skip archiving these projects but still publish them afterBuild, which runs aggressive trimming for tests that are not supported on the target platform. This prevents the Apple mobile CoreCLR leg from invokingILLinkforSystem.Text.Json.SourceGeneration.Roslyn4.4.Tests. Failure log https://dev.azure.com/dnceng-public/public/_build/results?buildId=1516430&view=logs&j=fbbe6d24-ec94-5563-68e4-47df2fb4f886&t=fd24ffdb-b6c4-50de-7ee2-f1794d18b0a7