[wasm][coreclr] Re-enable System.Formats.Nrbf tests#129334
Merged
radekdoulik merged 13 commits intoJun 24, 2026
Merged
Conversation
The System.Formats.Nrbf.Tests suite was excluded on browser-wasm + CoreCLR because test discovery crashed with a FileNotFoundException for System.Runtime.Serialization.Formatters, Version=11.0.0.0. Root cause: for a self-contained browser-wasm app, @(ReferenceCopyLocalPaths) contains two copies of System.Runtime.Serialization.Formatters - the shared framework's non-functional 8.1.0.0 stub (from the runtime pack) and the functional 11.0.0.0 build the test references app-local. The WebAssembly SDK task ComputeWasmBuildAssets dedupes webcil bundle candidates by relative path on a first-wins, version-blind basis, so the 8.1.0.0 stub got bundled into _framework and shadowed the app-local 11.0.0.0 copy. xunit discovery then materializes [InlineData(FormatterTypeStyle.X)] attribute blobs, triggering an Assembly.Load of the 11.0.0.0 build, which is not present - aborting discovery. Unlike desktop (framework-dependent deployment, or single-file publish whose bundler consumes the already version-resolved ResolvedFileToPublish set), the wasm webcil bundle is assembled at build time from candidates that are not version-conflict-resolved against ProjectReferences. Fix: add a CoreCLR-wasm target that applies copy-local-wins semantics before the bundle candidates are gathered - it drops runtime-pack copy-local assemblies that are shadowed by a same-named app-local copy. Apps without an app-local copy of a framework assembly are unaffected. With the fix the bundled Formatters webcil is the functional 11.0.0.0 build, discovery finds 153 test cases (was 6) and the suite passes; the ReadTests hierarchy correctly skips on browser since BinaryFormatter is unsupported there. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
radekdoulik
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR re-enables System.Formats.Nrbf.Tests for browser-wasm + CoreCLR by removing the test exclusion and adding an MSBuild-time workaround to ensure app-local assemblies win over runtime-pack copies when building the WASM bundle.
Changes:
- Add a CoreCLR browser-wasm target that removes runtime-pack
ReferenceCopyLocalPathsentries when an app-local assembly with the same name exists (so the app-local assembly is the one bundled). - Remove the
System.Formats.Nrbf.Testsexclusion fromsrc/libraries/tests.projfor browser/CoreCLR.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/mono/browser/build/BrowserWasmApp.CoreCLR.targets | Adds a pre-bundling MSBuild target to prefer app-local assemblies over runtime-pack copies for browser-wasm CoreCLR builds. |
| src/libraries/tests.proj | Re-enables System.Formats.Nrbf.Tests on browser/CoreCLR by removing its ProjectExclusions entry. |
$(MicrosoftNetCoreAppRuntimePackDir) is not normalized in this file (it can come straight from %(ResolvedRuntimePack.PackageDirectory) or a global property), so a missing trailing separator or non-native path separators could make the StartsWith checks fail to identify runtime-pack assemblies, silently skipping the dedupe. Normalize it once with [MSBuild]::NormalizeDirectory and use that for both checks so it reliably matches %(FullPath). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The target previously only ran when $(MicrosoftNetCoreAppRuntimePackDir) was set, but WBT/Helix-created CoreCLR-wasm projects can leave that property empty and rely on %(ResolvedRuntimePack.PackageDirectory) instead, so the target was skipped and the runtime-pack vs app-local shadowing persisted for them. Resolve the runtime pack dir inside the target the same way the WebAssembly SDK does (Browser.targets): prefer the property, fall back to the ResolvedRuntimePack metadata, then normalize. Both ItemGroups are guarded so an unresolved dir is a safe no-op. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
maraf
reviewed
Jun 15, 2026
_GatherWasmFilesToBuild has no DependsOnTargets, so a target hooked only via BeforeTargets could run before ResolveReferences populates @(ReferenceCopyLocalPaths), making the dedupe a no-op. Add DependsOnTargets=ResolveReferences so the prune is correct-by-construction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jun 15, 2026
Open
Replace the name-based "copy-local wins" rule with the SDK's own ResolvePackageFileConflicts task so the higher AssemblyVersion wins, matching desktop self-contained publish. This keeps the functional System.Runtime.Serialization.Formatters 11.0.0.0 over the 8.1.0.0 framework stub in the wasm _framework bundle. The task ships in every .NET SDK, so it resolves on user systems for any Microsoft.NET.Sdk browser-wasm build, not just the in-repo build. A name-based fallback (drop shadowed runtime-pack copies) is kept for the unlikely case the SDK build-tasks assembly is unavailable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Feed $(PackageConflictPreferredPackages) into the ResolvePackageFileConflicts call so an exact AssemblyVersion+FileVersion tie is broken the same way the SDK publish target (_HandlePackageFileConflictsForPublish) breaks it — preferring the framework runtime-pack package — instead of leaving the tie to the downstream first-wins webcil dedup. The property is populated by ResolveTargetingPackAssets, which runs as part of the ResolveReferences dependency, so it is available when this target executes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The StartsWith path-prefix checks and the Contains name-membership checks were ordinal case-sensitive. On a case-insensitive filesystem a casing difference between $(MicrosoftNetCoreAppRuntimePackDir) and the enumerated %(FullPath) values (e.g. drive-letter or user-overridden casing) could silently misclassify a runtime-pack copy as app-local and no-op the fix with no error. Compare with System.StringComparison.OrdinalIgnoreCase so classification is robust, matching how the SDK itself does path-prefix checks (e.g. Microsoft.FSharp.Targets). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4 tasks
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The section header still described the target as unconditionally preferring app-local assemblies over runtime-pack copies, but the resolution is now version-based via ResolvePackageFileConflicts (publish parity), where the runtime-pack copy can legitimately win when it has the higher version. Reword the banner and body so the comment matches the implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
_CoreCLRPreferAppLocalAssembliesOverRuntimePack implied app-local always wins, but the target resolves runtime-pack vs app-local conflicts by version (publish parity), where the runtime-pack copy can win when it is the higher version. Rename to _CoreCLRResolveRuntimePackVsAppLocalConflicts to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
maraf
approved these changes
Jun 24, 2026
The case-insensitive name membership checks used
$(list.IndexOf(';name;', System.StringComparison.OrdinalIgnoreCase) != -1).
MSBuild's property-function expander evaluates a method-call chain and returns
its value, but it rejects a trailing comparison operator (!= -1, >= 0) placed
inside the $(), raising error MSB4184 "cannot be evaluated". This is independent
of the StringComparison argument and of the item/metadata context, and it broke
the wasm CoreCLR build for every library test project (e.g. System.Runtime.Tests).
Contains(string, StringComparison) sidesteps it (returns bool directly) but that
overload is missing on .NET Framework MSBuild.
Keep IndexOf(string, StringComparison) with OrdinalIgnoreCase (available on .NET
Framework 2.0+) and move the "!= -1" test OUTSIDE the $() so it is a top-level
Condition comparison on the returned int. This preserves the original
case-insensitive intent, stays consistent with the StartsWith(..., OrdinalIgnoreCase)
path checks, and needs no lower-casing workaround.
Validated: Nrbf wasm CoreCLR suite builds 0 warnings/0 errors and runs 4/4 passing
from a freshly pruned bundle; the target's ResolvePackageFileConflicts picks
Formatters 11.0.0.0 over the 8.1.0.0 stub by AssemblyVersion.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address multi-model review findings on the runtime-pack vs app-local conflict-resolution target: - Restrict the conflict set to assemblies that deploy to the _framework root (empty DestinationSubDirectory/Culture). The bundle is keyed on a culture-qualified RelativePath, so same-named satellites in different cultures never collide; matching by file name alone could wrongly prune a runtime-pack satellite. Qualify the metadata references so items that lack DestinationSubDirectory/Culture don't trip MSB4096. - Drop the discarded copies the way the SDK's _HandlePackageFileConflicts does (remove every candidate, then re-add the winners) instead of removing an Exclude-derived loser subset. ReferenceCopyLocalPaths removal matches on ItemSpec only, so re-including the winners (which carry full metadata) is the robust, lossless form. - Note the single-runtime-pack invariant the ResolvedRuntimePack metadata fallback relies on, matching the WebAssembly SDK's Browser.targets. Validated: clean browser-wasm CoreCLR build of System.Formats.Nrbf.Tests, 0 warnings/0 errors, 4/4 tests pass (WASM EXIT 0); binlog confirms the functional 11.0.0.0 Formatters wins over the 8.1.0.0 stub. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jun 24, 2026
maraf
approved these changes
Jun 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Re-enables the
System.Formats.Nrbf.Testssuite on browser-wasm + CoreCLR (removes theProjectExclusionsentry insrc/libraries/tests.proj) and fixes the underlying bundling bug that made it crash.Problem
On browser-wasm + CoreCLR, test discovery crashed with:
System.Runtime.Serialization.Formattershas a version split: the shared framework ships a non-functional 8.1.0.0 stub, while the Nrbf test project references the functional 11.0.0.0 build app-local (Private="true" SetTargetFramework="...NetCoreAppMinimum").For a self-contained browser-wasm app,
@(ReferenceCopyLocalPaths)ends up with both copies. The WebAssembly SDK taskComputeWasmBuildAssetsdedupes webcil bundle candidates by relative path on a first-wins, version-blind basis, so the 8.1.0.0 stub got converted to webcil and bundled into_framework, shadowing the app-local 11.0.0.0 copy. At runtime, xunit discovery materializes[InlineData(FormatterTypeStyle.X)]attribute blobs →Assembly.Loadof Formatters 11.0.0.0 → only the stub is present →FileNotFoundException→ discovery aborts (only 6 of ~153 cases were ever found).Why desktop isn't affected
deps.jsonresolution prefers the higher version.ResolvedFileToPublishset (ComputeResolvedFilesToPublishListprefers the higherAssemblyVersion), so the stub is dropped before bundling.The wasm webcil bundle is assembled at build time from candidates that are not version-conflict-resolved against
ProjectReferences, so it bypassed the resolution that protects the desktop paths.Fix
Add a CoreCLR-wasm target (
_CoreCLRResolveRuntimePackVsAppLocalConflictsinBrowserWasmApp.CoreCLR.targets) that runs afterResolveReferencesand before the webcil bundle candidates are gathered. For assemblies that appear in@(ReferenceCopyLocalPaths)as both a runtime-pack copy and a same-named app-local copy, it resolves the conflict the same way a self-contained desktop publish does:ResolvePackageFileConflictstask — the same task desktop publish uses — which picks the winner by higher AssemblyVersion → FileVersion →PreferredPackagesrank, and removes the loser from@(ReferenceCopyLocalPaths)so only the winning copy reaches the bundle.PreferredPackages="$(PackageConflictPreferredPackages)"is passed for exact tie-break parity with publish.$(MicrosoftNETBuildTasksAssembly)can't be located.For the canonical case this selects the functional 11.0.0.0 Formatters over the 8.1.0.0 stub, matching desktop self-contained publish. Apps without an app-local copy of a framework assembly are unaffected (no conflict, nothing is removed).
Behavior note: because selection is version-based (matching publish) rather than an unconditional app-local preference, an app-local copy with a lower version than the runtime-pack copy will now lose — the same outcome it would get from a self-contained publish.
All runtime-pack path/name comparisons use
OrdinalIgnoreCaseso the resolution is robust to user-overridden output-path casing.Follow-up
The root cause is the version-blind, first-wins dedup in the shared WebAssembly SDK task
ComputeWasmBuildAssets(uniqueRelativePaths.Add(relativePath)), which both Mono and CoreCLR wasm go through. This PR deliberately scopes the fix to the CoreCLR-only targets file to avoid changing the common Mono/CoreCLR bundling path and risking a .NET 11 Mono regression (Mono doesn't hit this bug). Moving the resolution intoComputeWasmBuildAssetsis tracked by #129741 and planned for after the .NET 12 branch, at which point this workaround target can be removed.Validation
Ran
./dotnet.sh build -c Debug /t:Test src/libraries/System.Formats.Nrbf/tests/System.Formats.Nrbf.Tests.csproj /p:TargetOS=browser /p:TargetArchitecture=wasm /p:RuntimeFlavor=CoreCLR /p:Scenario=WasmTestOnChrome:_frameworkFormatters webcil: 69913 → 156953 bytes (8.1.0.0 stub → functional 11.0.0.0).FileNotFoundException.ReadTestshierarchy correctly skips on browser via[ConditionalClass(IsBinaryFormatterSupported)](BinaryFormatter is unsupported on browser by design), leaving the 4 platform-independentStartsWithPayloadHeaderTestscases running and passing — the same behavior as any platform without BinaryFormatter support.Note
This pull request was authored by GitHub Copilot.