Skip to content

Disable FastReducer_AssertFailure_RegressionTest on 32-bit platforms#126216

Merged
stephentoub merged 2 commits intomainfrom
copilot/disable-assert-failure-regression-test
Mar 28, 2026
Merged

Disable FastReducer_AssertFailure_RegressionTest on 32-bit platforms#126216
stephentoub merged 2 commits intomainfrom
copilot/disable-assert-failure-regression-test

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 27, 2026

Workaround for a failure in FastReducer_AssertFailure_RegressionTest on 32-bit platforms tracked in #126212.

Description

Adds [ActiveIssue] to skip FastReducer_AssertFailure_RegressionTest when running as a 32-bit process:

[Theory]
[MemberData(nameof(AssertFailureRegressionTest_InputData))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/126212", typeof(PlatformDetection), nameof(PlatformDetection.Is32BitProcess))]
public static void FastReducer_AssertFailure_RegressionTest(...)

AI-Generated Content

Note

This PR description and the associated code changes were generated by GitHub Copilot.

@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.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Mar 27, 2026

/azp run runtime-nativeaot-outerloop

@jkotas jkotas marked this pull request as ready for review March 27, 2026 18:33
Copilot AI review requested due to automatic review settings March 27, 2026 18:33
@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Disables a flaky/hanging BigInteger modular exponentiation regression test on 32-bit processes while an upstream issue is investigated (#126212). This keeps CI reliable on 32-bit platforms without removing the test coverage elsewhere.

Changes:

  • Added an [ActiveIssue] attribute to skip FastReducer_AssertFailure_RegressionTest when PlatformDetection.Is32BitProcess is true.

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126216

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: The PR addresses a real, observed CI failure — FastReducer_AssertFailure_RegressionTest hangs/times out on 32-bit platforms (x86, arm32) as documented in #126212, which includes a callstack from @jkotas showing an apparent infinite loop in BigIntegerCalculator.AddSelf. The problem is already labeled as a "Known Build Error" and is actively blocking CI on multiple PRs.

Approach: Using [ActiveIssue] with PlatformDetection.Is32BitProcess is the established dotnet/runtime pattern for disabling tests on specific platforms while a root cause is investigated. The test continues to run on 64-bit, preserving coverage there. The tracking issue is properly linked.

Summary: ✅ LGTM. This is a minimal, well-targeted workaround that follows established codebase conventions. The change is a single attribute addition, correctly uses the [ActiveIssue] pattern, links to a proper tracking issue, and has already been approved by a maintainer (@kg).


Detailed Findings

✅ Correctness — Attribute usage is correct

The [ActiveIssue] attribute is applied with the correct signature: issue URL, typeof(PlatformDetection), and nameof(PlatformDetection.Is32BitProcess). This matches the established pattern used elsewhere in the repo (e.g., ModuleBuilderDefineInitializedData.cs for issue #65558). PlatformDetection.Is32BitProcess is defined as IntPtr.Size == 4, which correctly identifies 32-bit processes.

✅ Scope — Change is appropriately narrow

The PR touches exactly one line in one file. No unrelated changes, no whitespace noise. The test data provider (AssertFailureRegressionTest_InputData) and the test method itself are left intact — only the skip attribute is added. This means the test will automatically re-enable once #126212 is resolved and the [ActiveIssue] is removed.

✅ Test coverage — 64-bit platforms unaffected

The [ActiveIssue] condition is scoped to Is32BitProcess only, so the regression test for #97780 continues to execute on all 64-bit platforms (x64, arm64). This preserves the original test coverage where the hang does not occur.

✅ Tracking — Issue is properly documented

Issue #126212 includes the CI build links showing the timeout, the callstack identifying the hang location (BigIntegerCalculator.AddSelfFastReducer.SubModFastReducer.Reduce), and is labeled area-System.Numerics + Known Build Error. This provides adequate context for future investigation.

Generated by Code Review for issue #126216 ·

@stephentoub stephentoub enabled auto-merge (squash) March 27, 2026 19:07
Copy link
Copy Markdown
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for unblocking the CI runs!

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Mar 27, 2026

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Mar 28, 2026

/ba-g a lot of Unable to pull image... infra failures unrelated to the change

@stephentoub stephentoub merged commit 4122b6d into main Mar 28, 2026
103 of 120 checks passed
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.

6 participants