-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[browser][CoreCLR] enable all library tests #123860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @agocke, @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enables library tests for the Browser+CoreCLR configuration. Previously, these tests were run in smoke-only mode with shouldRunSmokeOnly: true; this PR switches to full test execution with shouldRunSmokeOnly: false and enables error continuation mode (shouldContinueOnError: true).
Changes:
- Adds project-level exclusions for 6 test projects that are completely incompatible with Browser+CoreCLR (System.IO.FileSystem.Tests, System.Reflection.MetadataLoadContext.Tests, System.Reflection.Metadata.Tests, System.Runtime.Loader.Tests, System.Diagnostics.StackTrace.Tests, System.Reflection.Tests)
- Adds granular test method exclusions across ~25 test projects using
[ActiveIssue]attributes for tests that fail on Browser+CoreCLR due to interpreter reflection bugs and other known issues - Implements two new native function signatures (
fiiandiidi) to support additional delegate and runtime test scenarios on WebAssembly - Adds debugging improvements to detect when a debugger is not attached in WASM environments
- Updates native interop tables with new function mappings (SystemNative_GetEUid, SystemNative_MProtect, SystemNative_FileSystemSupportsLocking reordering)
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| eng/pipelines/runtime.yml | Enables full library test execution for Browser+CoreCLR (was smoke-only), adds continue-on-error flag |
| src/libraries/tests.proj | Excludes 6 test projects incompatible with Browser+CoreCLR environment |
| src/tasks/WasmAppBuilder/coreclr/ManagedToNativeGenerator.cs | Adds "fii" and "iidi" function signatures to support additional delegate scenarios |
| src/coreclr/vm/wasm/callhelpers-interp-to-managed.cpp | Implements CallFunc_I32_I32_RetF32 and CallFunc_I32_F64_I32_RetI32 for new signatures |
| src/coreclr/vm/wasm/callhelpers-pinvoke.cpp | Adds native function declarations (GetEUid, MProtect, FileSystemSupportsLocking), updates entry count to 100 |
| src/coreclr/vm/wasm/callhelpers-reverse.cpp | Updates hash keys for reverse thunks (numeric key changes for consistency) |
| src/coreclr/pal/src/arch/wasm/stubs.cpp | Adds debugger attachment detection using time-based heuristic |
| src/libraries/System..Tests/.cs (25+ files) | Adds [ActiveIssue] attributes to disable failing tests pending issue #123011 resolution |
| [Theory] | ||
| [InlineData(FormatterTypeStyle.TypesAlways)] | ||
| [InlineData(FormatterTypeStyle.TypesAlways | FormatterTypeStyle.XsdString)] | ||
| [SkipOnPlatform(TestPlatforms.Browser, "BinaryFormatter is not supported on Browser")] |
Copilot
AI
Feb 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent attribute usage for Browser platform exclusion. In this file, lines 36 and 95 use [SkipOnPlatform(TestPlatforms.Browser, ...)], while the rest of the PR consistently uses [ActiveIssue("https://github.com/dotnet/runtime/issues/123011", typeof(PlatformDetection), nameof(PlatformDetection.IsBrowser), nameof(PlatformDetection.IsCoreCLR))] for Browser+CoreCLR combinations.
This inconsistency suggests these tests have a different reason for being disabled (BinaryFormatter not being supported) rather than the same issue (#123011) affecting other tests. However, this should be clarified - if these are related to issue #123011, they should use the same ActiveIssue attribute pattern for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 73 out of 73 changed files in this pull request and generated 3 comments.
| [Theory] | ||
| [InlineData(FormatterTypeStyle.TypesWhenNeeded)] | ||
| [InlineData(FormatterTypeStyle.XsdString)] | ||
| [SkipOnPlatform(TestPlatforms.Browser, "BinaryFormatter is not supported on Browser")] |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SkipOnPlatform attribute is inconsistent with the rest of the PR which uses ActiveIssue for the same Browser+CoreCLR combination. For consistency with other test changes in this PR, this should use ActiveIssue with the same parameters as the rest of the changes: [ActiveIssue("https://github.com/dotnet/runtime/issues/123011", typeof(PlatformDetection), nameof(PlatformDetection.IsBrowser), nameof(PlatformDetection.IsCoreCLR))]
| emscripten_debugger(); | ||
| double end = emscripten_get_now(); | ||
| // trying to guess if the debugger was attached | ||
| if (end - start < 100) |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 100ms threshold is a magic number used to determine if a debugger is attached. This value should be extracted to a named constant with a comment explaining why 100ms was chosen as the threshold. Additionally, this heuristic-based approach to detecting debugger attachment is fragile and may produce false positives/negatives depending on system load or debugger behavior.
eng/pipelines/runtime.yml
Outdated
| alwaysRun: ${{ variables.isRollingBuild }} | ||
| shouldRunSmokeOnly: true | ||
| # TODO consider shouldContinueOnError: true | ||
| shouldRunSmokeOnly: false |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change enables all library tests for Browser CoreCLR (not just smoke tests) but also sets shouldContinueOnError: true. This means test failures won't block the build. While this is reasonable for an experimental feature being tested, it should be clearly documented whether this is intentional behavior for the current state of Browser+CoreCLR support or if this is temporary until the linked issue is resolved.
| shouldRunSmokeOnly: false | |
| shouldRunSmokeOnly: false | |
| # Browser+CoreCLR library tests are experimental; keep this leg non-blocking | |
| # so test failures do not fail the build until Browser+CoreCLR support is stabilized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 100 out of 100 changed files in this pull request and generated 3 comments.
| if (PlatformDetection.IsBrowser && PlatformDetection.IsCoreClr && items > 1024) | ||
| { | ||
| // TODO-WASM too slow https://github.com/dotnet/runtime/issues/123011 | ||
| return; | ||
| } |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PlatformDetection doesn't define IsCoreClr (only IsCoreCLR / IsCoreClrInterpreter etc). This will fail to compile; use the existing PlatformDetection.IsCoreCLR property (or the intended runtime check) in this Browser+CoreCLR perf guard.
| <!-- this is here because of python --> | ||
| <PackageReference Condition="'$(HostOS)' == 'windows' and '$(ShouldProvisionEmscripten)' != 'true'" | ||
| Include="Microsoft.NET.Runtime.Emscripten.$(EmsdkVersion).Python.win-x64" | ||
| PrivateAssets="all" | ||
| Version="$(EmsdkPackageVersion)" | ||
| GeneratePathProperty="true" /> |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PackageReference condition depends on $(ShouldProvisionEmscripten), but that property is only set in AcquireEmscriptenSdk.targets, which is imported later in this project. During evaluation, the property will be empty here, so the condition will behave as if provisioning is not needed and can lead to incorrect/duplicate Emscripten Python package references. Consider importing AcquireEmscriptenSdk.targets before this ItemGroup, or change the condition to key off $(EMSDK_PATH) / $(TargetsBrowser) instead.
| int32_t SystemNative_FTruncate (void *, int64_t); | ||
| int32_t SystemNative_FUTimens (void *, void *); | ||
| int32_t SystemNative_FcntlSetFD (void *, int32_t); | ||
| int32_t SystemNative_FileSystemSupportsLocking (void *, int32_t, int32_t); |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type for SystemNative_FileSystemSupportsLocking is declared here as int32_t, but the native signature is uint32_t (see src/native/libs/System.Native/pal_io.h / pal_io.c). Keeping the signature consistent avoids ABI/interop mismatches and makes it clear this is a boolean-like value.
| int32_t SystemNative_FileSystemSupportsLocking (void *, int32_t, int32_t); | |
| uint32_t SystemNative_FileSystemSupportsLocking (void *, int32_t, int32_t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 89 out of 89 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
src/libraries/sendtohelix-wasm.targets:1
- This assignment unconditionally overrides any earlier
_workItemTimeoutvalue wheneverRuntimeFlavorisCoreCLR, which can unintentionally defeat more specific timeouts set above (e.g., scenario-specific timeouts). If the intent is to provide a default only when no timeout is already specified, add a guard likeand '$(_workItemTimeout)' == '', or otherwise structure the conditions so the most specific timeout wins.
src/libraries/System.Linq/tests/OrderByDescendingTests.cs:1 - Using
return;silently turns the test into a no-op for those inputs, which can mask regressions and makes test reporting less clear. Prefer an explicit skip mechanism (e.g., anActiveIssue/conditional attribute tied to the specific data case, or an assertion-based skip pattern used in this repo) so it's clearly reported as skipped rather than passed without executing.
src/libraries/tests.proj:1 - This new path mixes
\\and/separators, while the otherProjectExclusionsentries in the same block use\\. For consistency (and to avoid any edge-case path normalization differences across tooling), consider using a consistent separator style here (matching the surrounding entries).
| - browser_wasm | ||
| alwaysRun: ${{ variables.isRollingBuild }} | ||
| shouldRunSmokeOnly: true | ||
| # shouldRunSmokeOnly: false |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting out shouldRunSmokeOnly removes the setting rather than setting it to false, so the pipeline will fall back to whatever default behavior is defined elsewhere (which may still be smoke-only). If the goal is to enable all library tests, set shouldRunSmokeOnly: false as an actual YAML property (not a comment).
| # shouldRunSmokeOnly: false | |
| shouldRunSmokeOnly: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 109 out of 109 changed files in this pull request and generated 1 comment.
| <PropertyGroup> | ||
| <_workItemTimeout Condition="'$(Scenario)' == 'BuildWasmApps' and '$(_workItemTimeout)' == ''">01:30:00</_workItemTimeout> | ||
| <_workItemTimeout Condition="'$(NeedsToBuildWasmAppsOnHelix)' == 'true'">01:00:00</_workItemTimeout> | ||
| <!-- TODO-WASM CoreCLR interpreter is about 7x slower than Mono at the moment --> | ||
| <_workItemTimeout Condition="'$(RuntimeFlavor)' == 'CoreCLR'">02:30:00</_workItemTimeout> | ||
|
|
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CoreCLR-specific _workItemTimeout assignment will override any previously-set timeouts (e.g., the BuildWasmApps/NeedsToBuildWasmAppsOnHelix values) because the condition doesn’t check '$(_workItemTimeout)' == ''. Consider adding the same empty-check so scenario-specific settings remain effective unless no timeout was set yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 126 out of 126 changed files in this pull request and generated 6 comments.
| using System.Collections.Generic; | ||
| using System.Collections.Tests; | ||
| using System.Diagnostics; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using System.Collections.Tests; is not used anywhere in this file. Unused usings can fail the build (warnings-as-errors); please remove it.
| using System.Collections.Generic; | ||
| using System.Collections.Tests; | ||
| using System.Linq; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using System.Collections.Tests; is not used anywhere in this file. Unused usings can fail the build (warnings-as-errors); please remove it.
|
|
||
| using System.Collections.Generic; | ||
| using System.Collections.Tests; | ||
| using System.Diagnostics; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using System.Collections.Tests; is not used anywhere in this file. Unused usings can fail the build (warnings-as-errors); please remove it.
| using System.Diagnostics; |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Collections.Tests; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using System.Collections.Tests; is not used anywhere in this file. Unused usings can fail the build (warnings-as-errors); please remove it.
| using System.Collections.Tests; |
| using System.Collections.Generic; | ||
| using System.Collections.Tests; | ||
| using System.Linq; | ||
| using System.Runtime.CompilerServices; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using System.Collections.Tests; is not used anywhere in this file. Unused usings can fail the build (warnings-as-errors); please remove it.
| using System.Runtime.CompilerServices; |
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using System; appears to be unused in this file (no System.* types are referenced). With warnings treated as errors, this can break the build; please remove the unused using (or add a usage if intended).
testing ...
https://helix.dot.net/api/jobs/05679ccb-bca6-4ea4-9fd0-cf1b1b4d33d3/workitems?api-version=2019-06-17
https://helix.dot.net/api/jobs/dbd674cf-18dd-44a5-9323-b6c00fc9ba29/workitems?api-version=2019-06-17
https://helix.dot.net/api/jobs/075bc37c-0f7b-4cc5-b7d5-25b2ab4bcb91/workitems?api-version=2019-06-17
https://helix.dot.net/api/jobs/8d446830-7740-46cc-9970-22d32a04b8c1/workitems?api-version=2019-06-17
https://helix.dot.net/api/jobs/f18869d6-1eaf-4b5d-ad74-fa3ac619ba10/workitems?api-version=2019-06-17
https://helix.dot.net/api/jobs/86b502da-1c55-444c-ac54-2d5d4932b6f2/workitems?api-version=2019-06-17