Skip to content

[Mono][browser] Quarantine double Min/Max NaN payload tests - #133312

Merged
lewing merged 2 commits into
mainfrom
lewing-quarantine-double-min-max-browser
Sep 6, 2026
Merged

lewing merged 2 commits into
mainfrom
lewing-quarantine-double-min-max-browser

Conversation

@lewing

@lewing lewing commented Sep 5, 2026 •

Copy link
Copy Markdown
Member

Quarantines the four double generic-math Max/Min cases tracked by #133311 on Mono browser-WASM without splitting the shared test methods or data sets.

The affected NaN sign-and-payload rows remain in MaxDouble and MinDouble for every other runtime. Each pair is conditionally omitted only when PlatformDetection.IsMonoRuntime && PlatformDetection.IsWasm; all other Max/Min coverage remains unchanged.

Validation:

  • PATH=/opt/homebrew/bin:$PATH ./build.sh mono+libs -os browser
  • Focused Mono browser-WASM runs: 17/17 applicable MaxTest cases passed and 17/17 applicable MinTest cases passed

Contributes to #133311

Note

This pull request description was generated with GitHub Copilot.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 5, 2026 20:59
@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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@lewing
lewing requested a review from tannergooding September 5, 2026 21:02
@lewing lewing added the arch-wasm WebAssembly architecture label Sep 5, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

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.

🟡 Changes recommended

Moving those NaN sign/payload rows out of the shared MaxDouble/MinDouble member data reduces coverage for other test suites that consume those datasets (e.g., Math and Vector tests), which is likely unintended given the stated goal to quarantine only Mono browser-WASM.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR quarantines the double Max/Min NaN sign-and-payload generic-math test cases on Mono browser-WASM by splitting those rows into dedicated member-data sets and adding two new [Theory] methods that are gated with ActiveIssue for TestPlatforms.Browser + TestRuntimes.Mono.

Changes:

  • Add MaxNaNSignAndPayloadTest and MinNaNSignAndPayloadTest theories with ActiveIssue scoped to Mono Browser.
  • Move the two NaN sign/payload rows out of GenericMathTestMemberData.MaxDouble / MinDouble into new MaxDoubleNaNSignAndPayload / MinDoubleNaNSignAndPayload datasets.
File summaries
File Description
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/DoubleTests.GenericMath.cs Adds two new theories to quarantine the NaN sign/payload cases via ActiveIssue on Mono Browser while keeping the main MaxTest/MinTest theories running.
src/libraries/Common/tests/System/GenericMathTestMemberData.cs Splits the NaN sign/payload rows into dedicated datasets and removes them from the shared MaxDouble/MinDouble datasets.
Review details

Suppressed comments (1)

src/libraries/Common/tests/System/GenericMathTestMemberData.cs:1595

  • Similarly, moving the NaN sign/payload rows out of MinDouble drops those inputs for all other tests that use GenericMathTestMemberData.MinDouble (Math.cs and various Vector* test suites). If the intent is only to quarantine Mono browser-WASM failures in DoubleTests.GenericMath, consider keeping these rows in MinDouble and adding a separate filtered dataset for DoubleTests.GenericMath.MinTest to use.
        public static IEnumerable<object[]> MinDoubleNaNSignAndPayload
        {
            get
            {
                yield return new object[] {  PositiveNaNDouble,          -0.0,                       PositiveNaNDouble };
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/libraries/Common/tests/System/GenericMathTestMemberData.cs Outdated
@lewing

lewing commented Sep 5, 2026

Copy link
Copy Markdown
Member Author

this is more important for the backports to keep things green

Comment thread src/libraries/Common/tests/System/GenericMathTestMemberData.cs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 5, 2026 22:05
@lewing
lewing enabled auto-merge (squash) September 5, 2026 22:08

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.

🟡 Changes recommended

The current IsWasm guard also applies to WASI (IsBrowser || IsWasi), so the quarantine scope is broader than described and should be narrowed or clarified.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/libraries/Common/tests/System/GenericMathTestMemberData.cs:1581

  • Same as above: PlatformDetection.IsWasm includes WASI (IsBrowser || IsWasi), so this also omits the NaN payload rows on Mono WASI. If the quarantine is meant to be browser-only, switch the check to IsBrowser (or clarify/expand the rationale if WASI is intended).
                // [ActiveIssue("https://github.com/dotnet/runtime/issues/133311")]
                if (!(PlatformDetection.IsMonoRuntime && PlatformDetection.IsWasm))
                {
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/libraries/Common/tests/System/GenericMathTestMemberData.cs
@lewing

lewing commented Sep 10, 2026

Copy link
Copy Markdown
Member Author

/backport to release/11.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/11.0 (link to workflow run)

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

Labels

arch-wasm WebAssembly architecture area-System.Numerics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants