[wasm] Harden ReferenceNewAssemblyRebuildTest against dependency-closure changes#130655
Closed
lewing wants to merge 1 commit into
Closed
[wasm] Harden ReferenceNewAssemblyRebuildTest against dependency-closure changes#130655lewing wants to merge 1 commit into
lewing wants to merge 1 commit into
Conversation
The test relied on System.Security.Cryptography being absent from the WasmBasicTestApp's AOT closure: swapping the entry point to one that used SHA256 was expected to introduce a new AOT module and thus change driver-gen.c (the AOT modules table). That assumption broke when System.IO.Compression started referencing System.Security.Cryptography (#122093), which roots crypto into the base app via ZipArchiveInteropTest. Crypto is now always AOT'd, so the swapped entry point adds nothing, driver-gen.c is unchanged, and the test fails (#130540). This is the second time the test rotted this way (Json -> crypto in #109069). Instead of chasing another BCL assembly that happens to be absent, use a dedicated first-party library (the existing empty "Library" test project) that the default app does not reference. It is trimmed away on the first build and only enters the AOT module set once the swapped entry point uses it, so the test fully controls the new-assembly signal and is immune to future changes in the app's dependency closure. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f05b19b1-9e98-4565-9efb-bee27f2a85c2
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the Wasm.Build.NativeRebuild.Tests.ReferenceNewAssemblyRebuildTest to make the “new AOT module added” signal deterministic by referencing a dedicated first-party test library, instead of relying on an incidental BCL dependency being absent/present in the app’s dependency closure.
Changes:
- Injects a
ProjectReferencetoWasmBasicTestApp/Libraryinto the copied test asset and populatesLibrary.cswith a controlled type. - Updates the “swapped entry point” (
NativeRebuildNewAssembly.cs) to reference the new library type instead ofSystem.Security.Cryptography. - Adds a new entrypoint source file (
NativeRebuildReferencedLibrary.cs) used to overwrite the library’s source during the test setup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/mono/wasm/Wasm.Build.Tests/NativeRebuildTests/ReferenceNewAssemblyRebuildTest.cs | Injects a ProjectReference to the test Library project and replaces Library.cs so the rebuild deterministically pulls in a new rooted assembly. |
| src/mono/wasm/testassets/EntryPoints/NativeRebuildReferencedLibrary.cs | New testasset snippet defining NewlyReferencedType, used as the “new assembly” signal. |
| src/mono/wasm/testassets/EntryPoints/NativeRebuildNewAssembly.cs | Switches the rebuilt entry point to call into NewlyReferencedType rather than crypto APIs. |
Contributor
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
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
Wasm.Build.NativeRebuild.Tests.ReferenceNewAssemblyRebuildTesthas been failing on the browser-wasm AOT leg (Known Build Error #130540, "Expected changed file: driver-gen.c"). This hardens the test so it no longer depends on the app's incidental dependency closure.Root cause
The test replaces the app's entry point with one that references a "new" assembly, then asserts that
driver-gen.c— the AOT modules table, onemono_aot_register_moduleline per AOT'd assembly — changes because a new module was added.It relied on
System.Security.Cryptographybeing absent fromWasmBasicTestApp's AOT closure (the swapped entry point usedSHA256). That assumption broke when #122093 ("Add Zip Archives password support") added an unconditionalSystem.Security.CryptographyProjectReferencetoSystem.IO.Compression. The app always compilesZipArchiveInteropTest.cs(usesSystem.IO.Compression), so crypto is now always in the closure and always AOT'd. The swapped entry point therefore adds no new module,driver-gen.cis byte-identical between build and rebuild,MonoAOTCompiler'sCopyIfDifferentskips the write, the timestamp is unchanged, andCompareStatfails.This is the second time the test rotted this way — #109069 previously switched it from
JsontoSystem.Security.Cryptographyfor the same reason ("Json lib was already referenced before the change in the test").Fix
Stop guessing which BCL assembly happens to be absent. Instead reference the existing dedicated first-party
Librarytest project (an empty library not referenced by the default app), following the same pattern already used byPInvokeTableGeneratorTests:<ProjectReference Include="..\Library\Library.csproj" />viaCopyTestAsset(extraItems: ...).Library.cswith a small type viaReplaceFile.NativeRebuildNewAssembly.cs) at that type instead ofSHA256.Libraryis unused in the first build → trimmed away → absent from the AOT module set. The swapped entry point uses it → rooted → new module in the rebuild →driver-gen.cchanges. BecauseLibraryis first-party and the test fully controls whether it is referenced, the new-assembly signal is deterministic and immune to future changes in the app's dependency closure.Fixes #130540.
Related
The underlying dependency regression (crypto rooted into every trimmed app that uses
ZipArchive, a browser-wasm size concern) is tracked separately in #130650.Testing
Not built/run locally — WBT requires a browser-wasm runtime baseline + emscripten and executes in a browser via the Helix/xharness harness. Relying on CI (the failing AOT leg exercises exactly this test) to validate.
Note
This PR was authored with the assistance of GitHub Copilot.