Enable additional library tests in ReadyToRun pipeline#130821
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3e455e1e-8fd6-4013-a04b-99b97c6e82be
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3e455e1e-8fd6-4013-a04b-99b97c6e82be
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3e455e1e-8fd6-4013-a04b-99b97c6e82be
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3e455e1e-8fd6-4013-a04b-99b97c6e82be
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR expands ReadyToRun (TestReadyToRun) coverage in the libraries test pipeline by removing exclusions for several additional test projects and making small test/publish adjustments needed for those projects to run under the ReadyToRun single-file test runner / publish+archive flow.
Changes:
- Enables ReadyToRun coverage for multiple library test projects by removing them from the
TestReadyToRunexclusion list (keeping Regex excluded). - Updates
System.Reflection.Testsentry-assembly assertion to account for ReadyToRun single-file runner behavior. - Adjusts test asset loading / packaging for ReadyToRun publish+archive scenarios (explicit
LoadFromfor TinyAssembly; delete bind-failure fixture DLLs from the publish bundle before zipping).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/tests.proj | Removes several TestReadyToRun exclusions to allow more projects to participate in the ReadyToRun pipeline (keeps Regex excluded). |
| src/libraries/System.Runtime/tests/System.Reflection.Tests/AssemblyTests.cs | Updates entry-assembly expectation when running under ReadyToRun compiled test mode. |
| src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj | Deletes bind-failure fixture DLLs from the ReadyToRun publish bundle before ZipTestArchive runs. |
| src/libraries/System.Reflection.TypeExtensions/tests/ModuleTests.cs | Loads TinyAssembly.dll explicitly from the test base directory to work in published/ReadyToRun layouts. |
|
Azure Pipelines: Successfully started running 2 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. Some pipeline(s) encountered errors during trigger evaluation. |
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "ae8f57155efa3e0f7bbfccf1c7d4ba2a0650c79f",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "5f7b7651bc0a9d181e1d67e65f200d81387b7b20",
"last_reviewed_commit": "ae8f57155efa3e0f7bbfccf1c7d4ba2a0650c79f",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "5f7b7651bc0a9d181e1d67e65f200d81387b7b20",
"last_recorded_worker_run_id": "29689448814",
"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": "ae8f57155efa3e0f7bbfccf1c7d4ba2a0650c79f",
"review_id": 4730871713
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. This continues the long-running effort (#95928) to expand ReadyToRun test coverage by enabling four previously-excluded library test suites and fixing the specific issues that prevented them from passing under R2R.
Approach: Correct and well-scoped. Each enablement is paired with a targeted fix for the actual failure mode: the R2R runner being the entry assembly (IsReadyToRunCompiled added to the GetEntryAssembly branch), the TinyAssembly fixture needing an explicit LoadFrom from the base directory rather than relying on xUnit's Load→LoadFrom rewrite, and the bind-failure fixtures needing to be removed from the R2R publish/bundle directory (while their deps.json entries are retained) before the archive is zipped.
Summary: ✅ LGTM. The MSBuild target ordering is sound — R2R sets TestSingleFile=true (eng/testing/tests.props), so ZipTestArchive zips from $(BundleDir), and the new RemoveBindFailureTestAssembliesFromReadyToRunPublish target correctly runs AfterTargets=Publish / BeforeTargets=ZipTestArchive deleting from $(BundleDir), mirroring the existing non-R2R RemoveBindFailureTestAssemblies target that deletes from $(OutDir). The tests.proj exclusion cleanup preserves the remaining known-failing exclusion (Regex) with the tracking-issue reference. No correctness or completeness concerns; the change is a net reduction in exclusions with fixes matching each enabled suite.
Detailed Findings
No actionable findings. Minor observations only:
- 💡
ModuleTests.cs: Switching fromAssembly.Load(new AssemblyName("TinyAssembly"))toAssembly.LoadFrom(Path.Combine(AppContext.BaseDirectory, "TinyAssembly.dll"))makes the load path explicit and robust under R2R/single-file, which is appropriate sinceTinyAssemblyis aContentitem rather than a resolvable reference. The removed comment that explained the old implicit rewrite is no longer relevant. Behavior is equivalent for the assertions that follow. - 💡 The new R2R delete target duplicates the three fixture filenames already listed in the existing
RemoveBindFailureTestAssembliestarget. This is acceptable given the different source directory ($(BundleDir)vs$(OutDir)), but if the fixture set changes in the future both lists must be kept in sync.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 93.7 AIC · ⌖ 14.7 AIC · ⊞ 10K
|
/ba-g unrelated OOM |
Summary
System.Runtime.Loader.DefaultContext.Tests,System.Reflection.Tests,System.Reflection.TypeExtensions.Tests, andSystem.Runtime.Loader.Tests.deps.jsonentries.System.Text.RegularExpressions.Testsexcluded for follow-up work.Part of #95928.