Skip to content

Add project exclusion for System.Text.Json tests#128344

Merged
MichalStrehovsky merged 1 commit into
mainfrom
MichalStrehovsky-patch-1
May 19, 2026
Merged

Add project exclusion for System.Text.Json tests#128344
MichalStrehovsky merged 1 commit into
mainfrom
MichalStrehovsky-patch-1

Conversation

@MichalStrehovsky
Copy link
Copy Markdown
Member

Needed to make outerloops green again.

Cc @dotnet/ilc-contrib

Needed to make outerloops green again.
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib
See info in area-owners.md if you want to be subscribed.

@github-actions
Copy link
Copy Markdown
Contributor

Caution

Security scanning requires review for Code Review

Details

The threat detection results could not be parsed. The workflow output should be reviewed before merging.

Review the workflow run logs for details.

🤖 Copilot Code Review — PR #128344

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: Justified. Issue #128286 documents consistent Access Violation crashes in System.Text.Json.SourceGeneration.Roslyn4.4.Tests across multiple NativeAOT legs (windows-arm64, windows-x64, windows-x86, osx-x64). Disabling the test to unblock CI outerloops is a standard practice in this repo.

Approach: Correct — adding an unconditional ProjectExclusions entry under the TestNativeAot guard with a tracking issue link follows the established pattern in this file.

Summary: ✅ LGTM. Simple, well-motivated test exclusion that follows existing conventions.


Detailed Findings

💡 Redundant existing exclusion

The new unconditional exclusion (line 409) for issue #128286 makes the existing conditional exclusion (lines 412-413) for issue #119380 (osx/arm64 only) redundant — the same .csproj is now excluded on all NativeAOT configurations regardless. Consider removing the #119380 entry to avoid confusion, or leave it as-is so it's easy to re-enable the broader exclusion independently once #128286 is fixed. Either way, this is non-blocking.

✅ Correctness

  • The exclusion path matches the existing entry for the same project on line 412.
  • The issue link is correct and references a real, open tracking issue.
  • Placement within the correct ItemGroup condition is correct.

Generated by Code Review for issue #128344 · ● 1.2M ·

@MichalStrehovsky
Copy link
Copy Markdown
Member Author

/ba-g relevant legs already passed, needed to unblock outerloop testing

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants