Skip to content

[fix] fix: add RequestingAssembly to AssemblyResolveEventArgs for binary compat#16076

Merged
Evangelink merged 2 commits into
mainfrom
fix/issue-15765-d9a77a16d2e5f9ab
Jun 1, 2026
Merged

[fix] fix: add RequestingAssembly to AssemblyResolveEventArgs for binary compat#16076
Evangelink merged 2 commits into
mainfrom
fix/issue-15765-d9a77a16d2e5f9ab

Conversation

@nohwnd
Copy link
Copy Markdown
Member

@nohwnd nohwnd commented May 30, 2026

🤖 This is an automated fix by the Issue Triage agent.

Fixes #15765

Root Cause

SDK 10.0.300 ships a version of Microsoft.TestPlatform.Common.dll whose AssemblyResolver.OnResolve method calls args.RequestingAssembly on an AssemblyResolveEventArgs instance. However, AssemblyResolveEventArgs in Microsoft.TestPlatform.PlatformAbstractions.dll never had this property, causing a MissingMethodException at runtime:

System.MissingMethodException : Method not found: 'System.Reflection.Assembly
  Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces.AssemblyResolveEventArgs.get_RequestingAssembly()'.
  at Microsoft.VisualStudio.TestPlatform.Common.Utilities.AssemblyResolver.OnResolve(Object, AssemblyResolveEventArgs)

This only manifests with RunConfiguration.DisableAppDomain=true targeting net462, because that mode runs tests in the current AppDomain, loading vstest's assembly resolver into it. The AppDomain.AssemblyResolve event fires and hits OnResolve, which crashes.

Fix

  • Add RequestingAssembly property to AssemblyResolveEventArgs
  • Add a new constructor overload AssemblyResolveEventArgs(string? name, Assembly? requestingAssembly)
  • Update the net462 PlatformAssemblyResolver bridge to pass ResolveEventArgs.RequestingAssembly through to vstest's AssemblyResolveEventArgs
  • Declare the new public API in PublicAPI.Unshipped.txt

Testing

The fix restores binary compatibility between the two assemblies. The affected scenario (DisableAppDomain=true on net462) would no longer throw MissingMethodException when the assembly resolver fires.

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

…mpat (#15765)

The SDK 10.0.300 ships a version of AssemblyResolver that accesses
AssemblyResolveEventArgs.RequestingAssembly, but this property was
missing from AssemblyResolveEventArgs, causing MissingMethodException
when DisableAppDomain=true is used with net462 tests.

Add RequestingAssembly property and an overloaded constructor to
AssemblyResolveEventArgs, and update the net462 PlatformAssemblyResolver
to pass the requesting assembly from the CLR ResolveEventArgs.

Fixes #15765

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 30, 2026 01:35
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

This PR restores binary compatibility for assembly resolution by adding RequestingAssembly to AssemblyResolveEventArgs and flowing it from the .NET Framework resolver path.

Changes:

  • Adds a new AssemblyResolveEventArgs(string?, Assembly?) constructor.
  • Adds the RequestingAssembly public getter.
  • Passes ResolveEventArgs.RequestingAssembly through the net462 resolver bridge and records the new public API.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/Runtime/IAssemblyResolver.cs Adds the new constructor and RequestingAssembly property to resolver event args.
src/Microsoft.TestPlatform.PlatformAbstractions/net462/Runtime/PlatformAssemblyResolver.cs Forwards the CLR ResolveEventArgs.RequestingAssembly value into platform abstraction event args.
src/Microsoft.TestPlatform.PlatformAbstractions/PublicAPI/PublicAPI.Unshipped.txt Declares the new public API surface.

Copy link
Copy Markdown
Member Author

@nohwnd nohwnd left a comment

Choose a reason for hiding this comment

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

🧠 Reviewed by Expert Code Reviewer

Dimensions activated: Public API Surface Protection · Backward Compatibility & Rollback Safety · Cross-TFM & Framework Resolution · Testhost Assembly Loading & Resolution

Result: No issues found.

Dimension Verdict
Public API Surface Protection ✅ Both new entries correctly declared in PublicAPI.Unshipped.txt; property is appropriately get-only
Backward Compatibility & Rollback Safety ✅ Purely additive — existing single-arg constructor and all current callers are unaffected
Cross-TFM & Framework Resolution System.Reflection.Assembly? available on all TFMs; netcore resolver correctly omits RequestingAssembly since AssemblyLoadContext.Resolving doesn't expose it
Testhost Assembly Loading & Resolution ✅ Directly fixes the MissingMethodException on get_RequestingAssembly for DisableAppDomain=true + net462 scenario

PR description alignment: accurate and complete.

🧠 Reviewed by Expert Code Reviewer 🧠

@Evangelink
Copy link
Copy Markdown
Member

🤖 Expert vstest review (automated)

Small, additive binary-compatibility patch to Microsoft.TestPlatform.PlatformAbstractions — restores the RequestingAssembly property and a matching constructor overload on AssemblyResolveEventArgs that were removed by the revert in a74013a49. Surface change is correctly declared in PublicAPI.Unshipped.txt, the net462 resolver bridge now propagates ResolveEventArgs.RequestingAssembly, and existing callers of the single-arg constructor keep working. No blockers — a couple of nits around the .NET (Core) path and test coverage.

🛑 Blockers

None.

⚠️ Issues

None — the change is mechanical, additive, and consistent with .NET's own ResolveEventArgs.RequestingAssembly shape (get-only).

💡 Suggestions

  1. netcore/Runtime/PlatformAssemblyResolver.cs:73 is not updated, creating a silent platform asymmetry.
    AssemblyLoadContext.Resolving is Func<AssemblyLoadContext, AssemblyName, Assembly?> so there is no requesting-assembly to pass through — passing null is unavoidable. But the practical effect is that args.RequestingAssembly is non-null on net462 and always null on .NET (Core). Worth either:

    • calling out the asymmetry in the XML doc on RequestingAssembly (e.g. "may be null on .NET (Core) because AssemblyLoadContext.Resolving does not expose the requesting assembly"), or
    • explicitly invoking the new two-arg ctor with null in the netcore bridge for symmetry/grepability.
      Otherwise downstream code that one day branches on RequestingAssembly will behave differently across TFMs with no obvious clue why.
  2. No automated test pins the new surface or the net462 propagation.
    The PR description says "the fix restores binary compatibility… would no longer throw MissingMethodException" but nothing in test/ exercises the new constructor, the property, or the net462 PlatformAssemblyResolver invocation path (grep over test/ for AssemblyResolveEventArgs|PlatformAssemblyResolver|RequestingAssembly returns zero hits). The PublicAPI analyzer enforces the declaration, but a one-screen unit test would (a) lock the contract that RequestingAssembly is wired through on net462, and (b) make sure a future refactor of AssemblyResolverEvent doesn't silently drop the argument again — which is exactly the regression class that produced RunConfiguration.DisableAppDomain=true broken for net462 with the .NET 10.0.300 SDK #15765.

  3. PR description doesn't reflect the actual state of main.
    The description implies Microsoft.TestPlatform.Common.AssemblyResolver.OnResolve calls args.RequestingAssembly, but on main that call was reverted in a74013a49 (Revert of Track when vstest provides Newtonsoft.Json for extensions in telemetry #15596). On main this PR is purely a forward-compat / API-restoration patch; there is no in-repo consumer. Worth a one-line clarification in the description so future readers don't grep for the call site and conclude the patch is wrong.

✅ Notes

  • PublicAPI.Unshipped.txt correctly declares both new members. ✓
  • Additive constructor overload — no behavioural change for callers of new AssemblyResolveEventArgs(string?). RequestingAssembly defaults to null for the old ctor, which is consistent with not-knowing. ✓
  • Property is get-only, matching System.ResolveEventArgs.RequestingAssembly semantics. (Yes, Name is get; set; — that's pre-existing.) ✓
  • No Directory.Packages.props / dependency churn, no transitive surface impact, no risk to the net462 binding-redirect matrix. ✓
  • net462 PlatformAssemblyResolver change is a single-token addition (args.RequestingAssembly) — low risk. ✓

Verdict: COMMENT — approve once the .NET (Core) asymmetry is at least documented and ideally a small unit test is added pinning the propagation. :)

- Update RequestingAssembly XML doc to note it is always null on .NET (Core)
  because AssemblyLoadContext.Resolving does not expose the requesting assembly
- Update netcore PlatformAssemblyResolver to explicitly pass null for
  requestingAssembly for symmetry/grepability
- Add AssemblyResolveEventArgsTests covering both constructor overloads

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@nohwnd
Copy link
Copy Markdown
Member Author

nohwnd commented Jun 1, 2026

Commit pushed: 94b81ab

🔧 Iterated by PR Iteration Agent 🔧

@nohwnd
Copy link
Copy Markdown
Member Author

nohwnd commented Jun 1, 2026

Thanks for the review! I've addressed the suggestions in the follow-up commit:

  1. RequestingAssembly asymmetry documented — added a <remarks> block on the property explaining that it is always null on .NET (Core) because AssemblyLoadContext.Resolving doesn't expose the requesting assembly.

  2. Netcore bridge uses explicit two-arg ctorPlatformAssemblyResolver on .NET (Core) now calls new AssemblyResolveEventArgs(args.Name, requestingAssembly: null) with a clarifying comment, making the intentional null explicit and greppable.

  3. Unit tests addedAssemblyResolveEventArgsTests in Microsoft.TestPlatform.Common.UnitTests covers both constructor overloads and pins that RequestingAssembly is correctly wired through (or left null for the single-arg ctor).

Regarding the PR description: the description does mention the revert context in the root cause section, but I'll leave a note here for future readers — as of main, the AssemblyResolver.OnResolve call to args.RequestingAssembly was reverted in a74013a49, so this PR is a forward-compat / API-restoration patch with no current in-repo consumer.

🔧 Iterated by PR Iteration Agent 🔧

Copy link
Copy Markdown
Member Author

@nohwnd nohwnd left a comment

Choose a reason for hiding this comment

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

🧠 Reviewed by Expert Code Reviewer

Scope of this review: Incremental commit 94b81ab on top of the previously-reviewed 285d14f.

Changes since last review:

  • XML doc on RequestingAssembly explaining the always-null behaviour on .NET (Core)
  • netcore PlatformAssemblyResolver now explicitly passes requestingAssembly: null (symmetry / grepability)
  • New AssemblyResolveEventArgsTests covering both constructor overloads

Result: No issues found.

Dimension Verdict
Public API Surface Protection ✅ No new API added in this commit
Correctness ✅ Explicit null on netcore path is correct — AssemblyLoadContext.Resolving does not expose the requesting assembly
Test coverage ✅ Three test cases cover the single-arg constructor (null default), two-arg with assembly, and two-arg with explicit null
Documentation ✅ Remarks correctly document the TFM asymmetry

PR description alignment: accurate and complete — describes both commits together.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

@Evangelink Evangelink enabled auto-merge (squash) June 1, 2026 11:33
@Evangelink Evangelink merged commit 921adfd into main Jun 1, 2026
20 checks passed
@Evangelink Evangelink deleted the fix/issue-15765-d9a77a16d2e5f9ab branch June 1, 2026 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RunConfiguration.DisableAppDomain=true broken for net462 with the .NET 10.0.300 SDK

3 participants