Skip to content

Fix combined ActiveIssue restrictions in runtime test wrappers - #133571

Open
MichalStrehovsky with Copilot wants to merge 9 commits into
mainfrom
copilot/fix-activeissue-filtering-runtime-test-wrappers
Open

MichalStrehovsky with Copilot wants to merge 9 commits into
mainfrom
copilot/fix-activeissue-filtering-runtime-test-wrappers

Conversation

Copilot AI commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Require platform AND target framework AND runtime to match the four-argument ActiveIssue overload. Check framework/runtime before applying the existing platform decorator, preserving runtime OS checks for AnyOS builds.
  • Apply the same semantics to conditional OuterLoop attributes at priority zero; preserve higher-priority inclusion and existing skip reasons/reporters.
  • Add 46 host-side xUnit generator tests, registered in clr.toolstests, covering ordinary, process-isolated, and merged runners, including referenced legacy entry points.
  • Mark calli_excep.il explicitly Windows-only. Fixing the over-skip exposed its kernel32 dependency on Linux; directly running its body confirmed DllNotFoundException.

Root cause

The old implementation chained three independent skip transformations. A match in any dimension could permanently remove the test body; TargetFrameworkMonikers.Any matched Netcoreapp regardless of the runtime/platform restrictions. The fix follows the conjunction in the pinned Arcade implementation.

#126517 addressed a different switch branch: unspecified SkipOnCoreClr dimensions defaulting to Any instead of zero. Its zero defaults remain unchanged.

Validation

  • Checked CoreCLR/release libraries baseline succeeded: build.sh clr+libs -lc release -rc checked.
  • 46 passed, zero failed/skipped, through both VSTest and the repository /t:Test target. Tests compile generated runners, inspect actual invocation syntax and skip reasons, and execute ordinary runners with a test-body counter to reject successful no-ops.
  • Restoring only the original four-argument composition caused 22 failures, including the referenced-assembly cases; restoring the fix returned all 46 to passing.
  • Built Linux Directed_ro, inspected the restored call to calli_excep before adding its Windows restriction, and verified that the final Linux runner excludes this Windows-only test.
  • Independent code review found no actionable issues. Secret scanning and the added dependency advisory check passed.

Limitations

Actual Windows/Mono SEH execution was not available; those generator configurations were tested on the Linux host. Later builds retained unavailable NuGet-audit-endpoint warnings using local WarningsNotAsErrors=NU1900;NU1905; audit remained enabled. The baseline also reported existing Microsoft.DiaSymReader.Native advisories. Automated code review was unavailable, and CodeQL skipped analysis because its database was too large.

Note

This pull request was generated by GitHub Copilot.

Copilot AI and others added 2 commits September 10, 2026 04:55
…ctions

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
…atform skips

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
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.

@MichalStrehovsky

Copy link
Copy Markdown
Member

/azp run runtime-nativeaot-outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

@MichalStrehovsky

Copy link
Copy Markdown
Member

Cool, we now run the calli_excep test again and as expected, it fails with native AOT. This test was supposed to be disabled on dotnet/runtimelab#166 but we lost the disablement and never noticed because the test just stopped running everywhere in #123248.

00:19:34.542 Running test: JIT\Directed\pinvoke\calli_excep\calli_excep.dll
Expected: 100
Actual: -1073741801
END EXECUTION - FAILED

Cc @jkoritzinsky

@MichalStrehovsky MichalStrehovsky left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also delete the XUnitWrapperGenerator.Tests and the related infra, I'm not going to read any of that.

Comment thread src/tests/JIT/Directed/pinvoke/calli_excep.il
Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
@MichalStrehovsky

Copy link
Copy Markdown
Member

@copilot delete the XUnitWrapperGenerator.Tests and related test infra that was added in this branch.

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>

Copilot AI commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

@copilot delete the XUnitWrapperGenerator.Tests and related test infra that was added in this branch.

Removed in 1ebd1a4: the test project, test source, Directory.Build imports, and clr.toolstests registration. The generator and calli_excep fixes are unchanged. Directed_ro builds successfully, and MSBuild selects exactly the five original tools-test projects.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 2 pipeline(s).

Copilot AI left a comment

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.

Copilot review overview

🔵 Needs a closer look

Add the missing host-side regression tests and clr.toolstests registration.

Review effort: Lite
Findings: None

What changed in this PR

Fixes combined ActiveIssue/OuterLoop filtering so platform, framework, and runtime restrictions are conjunctive, and marks calli_excep.il Windows-only.

Changes:

  • Corrects combined restriction evaluation.
  • Adds Windows/platform guards to the interop test.
  • Claimed generator regression tests are not present in the submitted files.
File Description
src/​tests/​Common/​XUnitWrapperGenerator/​XUnitWrapperGenerator.cs Corrects combined restriction handling.
src/​tests/​JIT/​Directed/​pinvoke/​calli_excep.il Marks the test Windows-only.

@MichalStrehovsky

Copy link
Copy Markdown
Member

@copilot

Restore the CoreCLR/browser exclusions for these three tests that were enabled by #133367:

  • src/tests/JIT/Directed/callconv/CdeclMemberFunction/CdeclMemberFunctionTest.csproj
  • src/tests/JIT/Directed/callconv/PlatformDefaultMemberFunction/PlatformDefaultMemberFunctionTest.csproj
  • src/tests/JIT/Directed/callconv/StdCallMemberFunction/StdCallMemberFunctionTest.csproj

Why: The old ActiveIssue generator bug incorrectly skipped these tests on CoreCLR, masking unresolved WASM interop problems. Fixing the skip semantics now executes their bodies; CdeclMemberFunctionTest fails with missing WASM calli thunk MS8ii.

Required changes:

  1. Restore the conditional WasmBuildTestCorerun=false opt-out removed by [wasm] Build a test-specific corerun for nine more runtime tests #133367 for these three projects, limited to RuntimeFlavor=coreclr and TargetOS=browser.
  2. Reference the existing interop-gap tracking issue, [browser-wasm] CoreCLR runtime tests blocked on interop gaps after test-specific corerun enablement #131811, in the explanatory comments.
  3. Verify that disabling the test-specific corerun actually causes the tests to skip. If their existing guards are insufficient, add explicit CoreCLR/browser exclusions referencing [browser-wasm] CoreCLR runtime tests blocked on interop gaps after test-specific corerun enablement #131811—otherwise they could simply fail on missing native assets instead.
  4. Preserve the ActiveIssue conjunction fix in Fix combined ActiveIssue restrictions in runtime test wrappers #133571 and all existing restrictions for other configurations. Leave the unrelated tests enabled by [wasm] Build a test-specific corerun for nine more runtime tests #133367 unchanged.

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 24, 2026 09:00

Copilot AI commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

@copilot

Restore the CoreCLR/browser exclusions for these three tests that were enabled by #133367:
...

Addressed in d6bfcee. Restored the three CoreCLR/browser-only corerun opt-outs with #131811 comments. Their existing guards were insufficient, so each entry point now has an explicit IsBrowser && IsCoreCLR ActiveIssue exclusion. Existing Mono/Windows restrictions, the conjunction fix, and unrelated tests are unchanged.

Validation: baseline and all three browser managed builds passed; generated wrappers skip before calling the bodies and report #131811. All three tests execute and pass on Linux (zero skipped); 15 project evaluations confirm the opt-outs’ scope. Live browser execution remains unverified.

Independent review and secret scanning passed. Automated review was unavailable; CodeQL skipped its oversized database. Post-merge disabled-test labeling of #131811 remains pending.

Copilot AI left a comment

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.

Copilot review overview

🔵 Needs a closer look

Three browser CoreCLR project opt-outs require resolution before approval.

Review effort: Lite
Findings: None

@MichalStrehovsky

Copy link
Copy Markdown
Member

@copilot for the WASM specific changes to tests, stick to the wording that was removed in #133367. also looks like we don't need the ActiveIssue, WasmBuildTestCorerun should be enough.

Copilot AI commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

@copilot for the WASM specific changes to tests, stick to the wording that was removed in #133367. also looks like we don't need the ActiveIssue, Wasm...

The original wording is restored locally, but removing the attributes exposed a concrete problem: all three compiled browser wrappers call TestEntryPoint unconditionally. WasmBuildTestCorerun=false skips private-host generation only; the launcher still uses CORE_ROOT/corerun.js, and these tests have no native-asset guard.

Baseline, browser managed builds, and all three Linux test runs passed; actual browser execution was not run. I have not committed the pending changes. Should I retain the explicit guards, or replace them with a CoreCLR/browser project-level exclusion?

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>

Copilot AI left a comment

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.

Copilot review overview

🔵 Needs a closer look

Regression coverage described in the PR is absent, and the validation summary is stale.

Review effort: Lite
Findings: None

@MichalStrehovsky

Copy link
Copy Markdown
Member

@copilot JIT/Directed/callconv/ThisCall/ThisCallTest is also failing on WASM, we need to restore the disabling for that one too.

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 24, 2026 13:54

Copilot AI commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

@copilot JIT/Directed/callconv/ThisCall/ThisCallTest is also failing on WASM, we need to restore the disabling for that one too.

Addressed in 04b2092. Restored ThisCall’s CoreCLR/browser corerun opt-out with the original wording and #131811 link. Since this test has no native-assets guard, added a matching CLRTestTargetUnsupported exclusion so it actually stays out of the browser runner—no new ActiveIssue attribute.

Baseline and targeted Linux build passed; ThisCall executed with exit 100. Browser runner discovery excludes it while retaining its four siblings; configuration checks preserve Mono/browser and CoreCLR Linux/Windows/WASI. Live browser execution was not run. Other tests and the generator fix are unchanged.

Copilot AI left a comment

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.

Copilot review overview

🟡 Changes recommended

Unresolved moderate findings (3 votes each) concern reintroduced browser/WASM exclusions.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Previously missed (6)

In code that hasn't changed since last review

Medium severity Remove ActiveIssue regressing verified browser coverage

src/​tests/​JIT/​Directed/​callconv/​CdeclMemberFunction/​CdeclMemberFunctionTest.cs:71

This browser/CoreCLR ActiveIssue makes the test body skip on the exact configuration that #133367 explicitly enabled and verified: the PR built CdeclMemberFunctionTest with a test-specific corerun.wasm, linked its native assets, and passed it. Adding this condition therefore regresses the browser coverage rather than fixing an unsupported test. Please remove it unless a newer reproducible failure supersedes that result, and update the tracking issue if so.

Medium severity Do not reintroduce browser opt-out for CdeclMemberFunctionTest

src/​tests/​JIT/​Directed/​callconv/​CdeclMemberFunction/​CdeclMemberFunctionTest.csproj:5

This reintroduces the browser opt-out that #133367 removed for CdeclMemberFunctionTest. That PR explicitly rebuilt this project with a test-specific corerun.wasm and recorded it among the 9/9 browser-wasm projects that linked and passed; the current #131811 issue likewise lists it as enabled. Keeping this property now sends the test back to the shared host and loses the native coverage that was verified on main, contradicting the stated reason for the workaround. Please retain it only if a newer reproducible failure supersedes #133367 and update the tracking issue accordingly.

Medium severity Remove ActiveIssue regressing verified browser coverage

src/​tests/​JIT/​Directed/​callconv/​PlatformDefaultMemberFunction/​PlatformDefaultMemberFunctionTest.cs:70

This browser/CoreCLR ActiveIssue makes the test body skip on the exact configuration that #133367 explicitly enabled and verified: the PR built PlatformDefaultMemberFunctionTest with a test-specific corerun.wasm, linked its native assets, and passed it. Adding this condition therefore regresses the browser coverage rather than fixing an unsupported test. Please remove it unless a newer reproducible failure supersedes that result, and update the tracking issue if so.

Medium severity Do not reintroduce browser opt-out for PlatformDefaultMemberFunctionTest

src/​tests/​JIT/​Directed/​callconv/​PlatformDefaultMemberFunction/​PlatformDefaultMemberFunctionTest.csproj:5

This reintroduces the browser opt-out that #133367 removed for PlatformDefaultMemberFunctionTest. That PR explicitly rebuilt this project with a test-specific corerun.wasm and recorded it among the 9/9 browser-wasm projects that linked and passed; the current #131811 issue likewise lists it as enabled. Keeping this property now sends the test back to the shared host and loses the native coverage that was verified on main, contradicting the stated reason for the workaround. Please retain it only if a newer reproducible failure supersedes #133367 and update the tracking issue accordingly.

Medium severity Remove ActiveIssue regressing verified browser coverage

src/​tests/​JIT/​Directed/​callconv/​StdCallMemberFunction/​StdCallMemberFunctionTest.cs:71

This browser/CoreCLR ActiveIssue makes the test body skip on the exact configuration that #133367 explicitly enabled and verified: the PR built StdCallMemberFunctionTest with a test-specific corerun.wasm, linked its native assets, and passed it. Adding this condition therefore regresses the browser coverage rather than fixing an unsupported test. Please remove it unless a newer reproducible failure supersedes that result, and update the tracking issue if so.

Medium severity Do not reintroduce browser opt-out for StdCallMemberFunctionTest

src/​tests/​JIT/​Directed/​callconv/​StdCallMemberFunction/​StdCallMemberFunctionTest.csproj:5

This reintroduces the browser opt-out that #133367 removed for StdCallMemberFunctionTest. That PR explicitly rebuilt this project with a test-specific corerun.wasm and recorded it among the 9/9 browser-wasm projects that linked and passed; the current #131811 issue likewise lists it as enabled. Keeping this property now sends the test back to the shared host and loses the native coverage that was verified on main, contradicting the stated reason for the workaround. Please retain it only if a newer reproducible failure supersedes #133367 and update the tracking issue accordingly.

Comment thread src/tests/JIT/Directed/callconv/ThisCall/ThisCallTest.csproj
@MichalStrehovsky

Copy link
Copy Markdown
Member

@radekdoulik @lewing could you have a look at the WASM test disablement in this PR? These tests were enabled for WASM in #133367 but they don't work. We were just not running them anywhere due to the bug I'm fixing.

You can see the failure mode at the commit that Jeremy approved:

	Expression: !"GetCookieForCalliSig: unknown thunk signature"
	Location:   /__w/1/s/src/coreclr/vm/wasm/helpers.cpp:1546
	Function:   GetCookieForCalliSig 

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants