Skip to content

Fix GetModule_ReturnsModule test failure on browser-wasm#125286

Open
lewing wants to merge 1 commit intomainfrom
fix-wasm-getmodule-test
Open

Fix GetModule_ReturnsModule test failure on browser-wasm#125286
lewing wants to merge 1 commit intomainfrom
fix-wasm-getmodule-test

Conversation

@lewing
Copy link
Member

@lewing lewing commented Mar 7, 2026

The else branch added in PR #125080 assumed !HasAssemblyFiles implies NativeAOT, but browser-wasm also has !HasAssemblyFiles (since Assembly.Location is empty). On wasm (Mono), Module.Name returns the real DLL name ("System.Reflection.Context.Tests.dll"), not "<Unknown>", so the assertion fails.

Split the else branch to handle NativeAOT and wasm separately:

  • NativeAOT: assert Module.Name == "<Unknown>"
  • wasm/other: call GetModule(moduleName) and assert not null

Fixes #125245

The else branch added in PR #125080 assumed !HasAssemblyFiles implies
NativeAOT, but browser-wasm also has !HasAssemblyFiles. On wasm (Mono),
Module.Name returns the real DLL name, not "<Unknown>", so the assertion
fails. Split the else branch to handle NativeAOT and wasm separately.

Fixes #125245

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 7, 2026 00:05
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib
See info in area-owners.md if you want to be subscribed.

Copy link
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

Fixes a failing System.Reflection.Context test on browser-wasm by separating the previous !HasAssemblyFiles behavior into distinct expectations for NativeAOT vs wasm/other platforms, aligning assertions with runtime-specific Module.Name results.

Changes:

  • Update GetModule_ReturnsModule to treat !HasAssemblyFiles as either NativeAOT (expect "<Unknown>") or non-NativeAOT (expect GetModule(name) succeeds).
  • Keep the test unconditionally runnable while branching assertions per platform behavior.

Comment on lines +116 to +120
{
// On browser-wasm, HasAssemblyFiles is false but Module.Name still returns the real name
Module module = _customAssembly.GetModule(moduleName);
Assert.NotNull(module);
}
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

In the non-NativeAOT paths, both branches do the same thing (call GetModule(moduleName) and assert not null). This can be simplified to reduce duplication (e.g., handle IsNativeAot first, otherwise always assert GetModule is non-null), and the comment should be generalized since this else applies to any !HasAssemblyFiles && !IsNativeAot, not only browser-wasm.

Copilot uses AI. Check for mistakes.
else if (PlatformDetection.IsNativeAot)
{
// On native AOT, Module.Name returns "<Unknown>" and GetModule with that name returns null
Assert.Equal("<Unknown>", moduleName);
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

The comment says GetModule returns null on native AOT, but this branch only asserts moduleName is "". Either update the comment to match what’s being asserted, or add an assertion that _customAssembly.GetModule(moduleName) is null to validate the stated behavior.

Suggested change
Assert.Equal("<Unknown>", moduleName);
Assert.Equal("<Unknown>", moduleName);
Module module = _customAssembly.GetModule(moduleName);
Assert.Null(module);

Copilot uses AI. Check for mistakes.
}
else
{
// On browser-wasm, HasAssemblyFiles is false but Module.Name still returns the real name
Copy link
Member

Choose a reason for hiding this comment

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

Is this valid for CoreCLR as well?

Copy link
Member Author

@lewing lewing Mar 7, 2026

Choose a reason for hiding this comment

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

I have no idea, I'd just prefer people not commit red tests, we can revert #125080 and discuss it more

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I am just pointing out that this change is likely moving the red to a different configuration yet again.

I'd just prefer people not commit red tests, we can revert #125080 and discuss it more

The build analysis flagged this test on wasm as a known failure in #125080. The problem was that the matching pattern in #125051 that tracked the know failure was not specific enough. cc @matouskozak

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I wasn't intending on merging it on red, I opened the PR to hand off to the proper owner. I'm tired of tracking down red tests that were recently added and then become my problem.

@lewing lewing requested review from MichalStrehovsky and danmoseley and removed request for MichalStrehovsky March 7, 2026 00:09
@lewing
Copy link
Member Author

lewing commented Mar 7, 2026

@danmoseley we need tests that work for all the versions of the platforms including the outerloops can you take a look please.

@danmoseley danmoseley enabled auto-merge (squash) March 7, 2026 01:33
@lewing lewing disabled auto-merge March 7, 2026 02:22
@lewing lewing assigned danmoseley and unassigned lewing Mar 7, 2026
@agocke
Copy link
Member

agocke commented Mar 7, 2026

Longer term we should have a plan for converging behavior, I.e. coreclr has a consistent behavior for these situations and I presume we will continue to use that behavior on wasm.

@danmoseley
Copy link
Member

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@danmoseley
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@danmoseley
Copy link
Member

Are these two outerloops everything? If they are green too we can merge this?

And in terms of learnings for me, it's that before merging reflection-related test changes I should run various outerloops.

For the match pattern in #125051, I'm not sure what it should have been to be more specific.

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.

[browser-wasm] System.Reflection.Context.Tests.ExtendedAssemblyTests2.GetModule_ReturnsModule failure

5 participants