Enable previously suppressed tests on CoreCLR browser - #130970
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc3d8ec-ff97-4a7d-bfec-0f98ccd3db5f
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc3d8ec-ff97-4a7d-bfec-0f98ccd3db5f
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc3d8ec-ff97-4a7d-bfec-0f98ccd3db5f
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc3d8ec-ff97-4a7d-bfec-0f98ccd3db5f
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc3d8ec-ff97-4a7d-bfec-0f98ccd3db5f
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc3d8ec-ff97-4a7d-bfec-0f98ccd3db5f
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc3d8ec-ff97-4a7d-bfec-0f98ccd3db5f
Keep the T4 source aligned with the generated xUnit entry point and ActiveIssue attributes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc3d8ec-ff97-4a7d-bfec-0f98ccd3db5f
Keep the test enabled on CoreCLR desktop and browser while retaining suppression for Mono and NativeAOT. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc3d8ec-ff97-4a7d-bfec-0f98ccd3db5f
Move the reusable runtime classification to PlatformDetection and use it for the StackallocBlkTests ActiveIssue condition. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dc3d8ec-ff97-4a7d-bfec-0f98ccd3db5f
|
Azure Pipelines: Successfully started running 4 pipeline(s). 11 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 'arch-wasm': @lewing, @pavelsavara |
There was a problem hiding this comment.
Pull request overview
This PR narrows a set of ActiveIssue suppression conditions so more runtime tests run on CoreCLR browser/wasm, while keeping suppressions for Mono / other configurations where they’re still needed.
Changes:
- Adjust
ActiveIssuepredicates in several JIT/GC/base-services tests to skip browser only on Mono (or skip interpreter only outside CoreCLR browser). - Add
PlatformDetection.IsCoreCLR/IsNotCoreCLRhelpers in the shared CoreCLR test library. - Update
Runtime_64125T4 template and synchronize its generated.csoutput.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/JIT/SIMD/Matrix4x4.cs | Refines interpreter skip condition to not suppress on CoreCLR browser. |
| src/tests/JIT/Regression/JitBlue/Runtime_64125/Runtime_64125.tt | Updates generated-test template to use xUnit + targeted ActiveIssue conditions. |
| src/tests/JIT/Regression/JitBlue/Runtime_64125/Runtime_64125.cs | Regenerated output to match template changes / updated skip conditions. |
| src/tests/JIT/opt/Vectorization/StackallocBlkTests.cs | Switches suppression to use new PlatformDetection.IsNotCoreCLR helper. |
| src/tests/JIT/Methodical/eh/interactions/switchinfinally.cs | Narrows browser suppression to Mono browser only. |
| src/tests/JIT/HardwareIntrinsics/General/HwiOp/HwiSideEffects.cs | Narrows browser suppression to Mono browser only. |
| src/tests/GC/Coverage/271010.cs | Changes suppression from “CoreCLR” to a custom predicate excluding browser. |
| src/tests/Common/CoreCLRTestLibrary/PlatformDetection.cs | Adds shared CoreCLR vs non-CoreCLR runtime detection helpers. |
| src/tests/baseservices/invalid_operations/ManagedPointers.cs | Splits browser suppression to Mono browser only (and keeps Apple mobile suppressed). |
| src/tests/baseservices/invalid_operations/Arrays.cs | Splits browser suppression to Mono browser only (and keeps Apple mobile suppressed). |
Use the shared CoreCLR runtime detection for GC 271010 and remove a redundant Apple mobile suppression from Runtime_64125. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e841e19-0192-4e92-a86d-8b8a4ffca32d
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "5e56a0b5d34deafb4025a48d920506c5b05dc33b",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "402ed14c4080491d0965638b7a1dfd673239b586",
"last_reviewed_commit": "5e56a0b5d34deafb4025a48d920506c5b05dc33b",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "402ed14c4080491d0965638b7a1dfd673239b586",
"last_recorded_worker_run_id": "29741201585",
"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": "7019627882b14597ac09731720fdaeda53cdcc4a",
"review_id": 4730770800
},
{
"commit": "5e56a0b5d34deafb4025a48d920506c5b05dc33b",
"review_id": 4734878622
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Several runtime tests were unconditionally suppressed on TestPlatforms.Browser (or TestRuntimes.Mono) even though the underlying issues are Mono-specific. As CoreCLR gains browser/wasm support, these tests actually pass on CoreCLR-browser, so blanket suppressions cost coverage. The PR narrows the ActiveIssue conditions so the tests run on CoreCLR-browser while remaining suppressed on Mono/NativeAOT where the issues still apply.
Approach: The change replaces platform-flag suppressions (TestPlatforms.Browser) with member-based ActiveIssue conditions that combine IsBrowser and IsMonoRuntime (which the generator ANDs together, so suppression only triggers on Mono-browser), and adds a new shared PlatformDetection.IsCoreCLR/IsNotCoreCLR helper. Apple-mobile and Wasi suppressions are preserved as separate attributes. StackallocBlkTests is switched to IsNotCoreCLR. The Runtime_64125.tt T4 template is synchronized with its previously hand-edited generated .cs (adding using TestLibrary;/using Xunit;, public class Program, and the xunit [Fact]/[ActiveIssue]/TestEntryPoint shape).
Summary: This is a well-scoped, low-risk test-infrastructure change. The AND-combination semantics of multi-member ActiveIssue conditions (confirmed in XUnitWrapperGenerator.DecorateWithUserDefinedCondition, which joins members with &&) make the IsBrowser + IsMonoRuntime pairs correct: they keep Mono-browser suppressed while enabling CoreCLR-browser. The new IsCoreCLR helper (!IsMonoRuntime && IsNotNativeAot) is reasonable and mirrors existing Utilities definitions. Syncing the .tt with the generated .cs is a good correctness improvement. My one substantive concern is the StackallocBlkTests change, which broadens the suppression from Windows+Mono to all non-CoreCLR configs (all Mono OSes plus NativeAOT) — flagged inline. Overall verdict: LGTM, pending the author confirming the intended StackallocBlkTests scope.
Detailed Findings
- StackallocBlkTests suppression broadened (inline on
src/tests/JIT/opt/Vectorization/StackallocBlkTests.cs): switching fromTestPlatforms.Windows, runtimes: TestRuntimes.MonotoIsNotCoreCLRsuppresses on all Mono OSes and NativeAOT, not just Windows-Mono. Confirm this is intended or narrow the condition.
No other actionable issues. The remaining changes correctly preserve prior suppression behavior on Mono/Wasi/Apple-mobile while enabling CoreCLR-browser coverage, and the T4/generated-source synchronization is consistent.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 91.9 AIC · ⌖ 10.6 AIC · ⊞ 10K
Keep issue dotnet#84398 scoped to Windows Mono so the test continues to run on other Mono platforms and NativeAOT. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e841e19-0192-4e92-a86d-8b8a4ffca32d
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Holistic Review
Motivation: Several runtime tests were unconditionally suppressed on TestPlatforms.Browser (or TestRuntimes.Mono) even though the underlying issues are Mono-specific. As CoreCLR gains browser/wasm support, these tests pass on CoreCLR-browser, so blanket suppressions cost coverage. The PR narrows the ActiveIssue conditions so tests run on CoreCLR-browser while staying suppressed on Mono/NativeAOT where the issues still apply.
Approach: Platform-flag suppressions are replaced with member-based ActiveIssue conditions that combine IsBrowser and IsMonoRuntime (the generator ANDs members together, so suppression only triggers on Mono-browser), plus a shared PlatformDetection.IsCoreCLR helper. Apple-mobile and Wasi suppressions are preserved as separate attributes, and the Runtime_64125.tt template is synchronized with its generated .cs. The latest commit (5e56a0b) revises the StackallocBlkTests suppression from IsNotCoreCLR to the two-member IsWindows + IsMonoRuntime condition and drops the now-unused IsNotCoreCLR helper.
Summary: The incremental change directly resolves the single substantive concern from the prior review. StackallocBlkTests now suppresses exactly on Windows+Mono (matching the original TestPlatforms.Windows, runtimes: TestRuntimes.Mono semantics via the generator's &&-joined member conditions in XUnitWrapperGenerator), rather than broadening to all non-CoreCLR configs. The IsNotCoreCLR helper was removed cleanly with no dangling references, and IsCoreCLR remains in use by GC/Coverage/271010.cs. This is a well-scoped, low-risk test-infrastructure change. Overall verdict: LGTM.
Assessment History
- review 4730770800 (commit
7019627): verdict LGTM pending author confirmation of theStackallocBlkTestsscope. Current verdict: LGTM. Changed: the risk concern is now resolved. Commit5e56a0bnarrowed the suppression fromIsNotCoreCLR(all Mono OSes + NativeAOT) back toIsWindows+IsMonoRuntime, restoring the original Windows-Mono-only behavior, so the previously flagged over-broad suppression no longer applies.
Detailed Findings
No actionable findings in the incremental change. The revised StackallocBlkTests condition is correct and the helper cleanup is safe.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 71.3 AIC · ⌖ 10.5 AIC · ⊞ 10K
|
/ba-g the failing job is unrealted, failed on main too |
Summary
Narrow several runtime-test
ActiveIssueconditions so tests that work with CoreCLR on browser/wasm are no longer suppressed.StackallocBlkTestssuppressed for Mono and NativeAOT while enabling it on CoreCLR desktop and browser.PlatformDetection.Runtime_64125T4 template with its generated source.Testing
./build.sh clr+libs -lc release -rc checked./build.sh -arch wasm -os browser -c Debug -subset clr+libsThe two broad-suite failures are unrelated upstream regressions:
TestConvertFromIntegralinJIT.IL_Conformanceb05617inJIT.Regression.Regression_7All executable tests affected by this change passed.
Note
This description was generated with GitHub Copilot.