[fix] Preserve the real exception (type + stack trace) when a test run aborts in BaseRunTests

[Jakub Jareš (nohwnd)]

When a test run aborts, BaseRunTests.RunTests caught the exception and rewrapped it as new Exception(ex.Message, ex.InnerException). That threw away the original exception's type and stack trace — callers (translation layer, VS Test Explorer) saw a bare Exception with a broken (often null) inner chain.

The fix preserves the original exception as the inner exception. The one thing we deliberately unwrap is TargetInvocationException — the reflection wrapper you get when a test extension is instantiated via Activator.CreateInstance and its constructor throws. That wrapper is just noise, so we surface its inner (real) exception. Every other exception is kept as-is. This mirrors the unwrap logic we already use in test/Intent/Runner.cs.

Tests cover both paths: a plain exception is preserved as the inner exception, and a TargetInvocationException is unwrapped to its real inner exception.

Fixes #16161.

---

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍 — refined after review.

[Copilot]

Pull request overview

Fixes an exception-wrapping bug in BaseRunTests.RunTests (CrossPlatEngine execution path) so callers receive the original exception (type + stack trace) as the InnerException, improving diagnostic clarity in downstream consumers.

Changes:

• Preserve the originally thrown exception by wrapping with new Exception(ex.Message, ex) instead of ex.InnerException.
• Add a unit test to ensure the original exception instance is preserved as InnerException in TestRunCompleteEventArgs.Error.

Reviewed changes

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

File	Description
src/Microsoft.TestPlatform.CrossPlatEngine/Execution/BaseRunTests.cs	Fixes exception wrapping to preserve the original exception as InnerException.
test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Execution/BaseRunTestsTests.cs	Adds coverage verifying the original exception instance is retained as the wrapper’s InnerException.

[Jakub Jareš (nohwnd)]

Code Review — BaseRunTests.RunTests Exception Wrapping

Dimensions activated: Error Reporting & Diagnostic Clarity, Parallel Execution & Scheduling Safety, IPC Transport & Protocol Stability (via ExceptionConverter / RemoteException chain)

---

Fix correctness ✅

The one-line change is correct:

// Before — discards ex from the chain entirely
exception = new Exception(ex.Message, ex.InnerException);

// After — preserves ex as InnerException
exception = new Exception(ex.Message, ex);

The old code silently severed ex from the exception chain. If ex had no InnerException (the common case), the resulting TestRunCompleteEventArgs.Error carried the message but no stack trace and no type from the site that actually failed. The fix restores ex as the InnerException, making the full type, stack trace, and inner chain available.

IPC serialization impact ✅

I checked ExceptionConverter.cs and RemoteException. The converter recursively serializes InnerException, so after this fix the wire format becomes:

{
  "ClassName": "System.Exception",
  "Message": "...",
  "InnerException": {
    "ClassName": "System.NotImplementedException",
    "StackTraceString": "<full original stack>",
    "InnerException": null
  }
}

Before the fix, InnerException was null (or the wrong sub-chain). This is a wire-compatible improvement — null → populated is not a breaking change for any reader.

Test ✅

RunTestsShouldPreserveOriginalExceptionAsInnerException correctly captures the TestRunCompleteEventArgs from the handler callback and asserts AreSame(originalException, receivedCompleteArgs.Error!.InnerException). It directly validates the fixed invariant. The use of AreSame (reference equality) is correct here — instance identity is the right check.

Minor observation (non-blocking)

The PR description says the original code discards "The original exception type". After this fix, the top-level wrapper is still System.Exception (not the original type), so type identity at Error.GetType() is still lost. The original type is now accessible one level down via Error.InnerException.GetType(). This is technically accurate given the description says types are "available to callers", but readers expecting the top-level type to be preserved may be surprised.

This is a pre-existing design choice in the wrapping pattern and is outside the scope of this fix. The improvement to diagnostics is real and meaningful.

---

No blocking issues found. The change is minimal, correct, and well-covered by the new test.

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Jakub Jareš (nohwnd)]

Thanks for the review! No blocking issues found — the fix, test, and IPC serialization impact all look good.

Waiting for Azure DevOps CI to complete. Will iterate if any failures come up.

🔧 Iterated by PR Iteration Agent 🔧

[Jakub Jareš (nohwnd)]

Re-review — TargetInvocationException unwrap logic (updated commit)

Dimensions activated: Error Reporting & Diagnostic Clarity, Parallel Execution & Scheduling Safety, IPC Transport & Protocol Stability

This is a follow-up review of the updated commit (c249f42). The previous expert review covered the original ex.InnerException → ex fix. This review focuses on the newly added TargetInvocationException unwrapping.

---

Updated fix correctness ✅

The revised logic:

Exception realException = ex is TargetInvocationException tie && tie.InnerException is not null
    ? tie.InnerException
    : ex;
exception = new Exception(realException.Message, realException);

• Regular exception path: realException = ex — identical to the previous fix, preserves type and stack trace as InnerException.
• TIE unwrap path: strips the reflection wrapper and surfaces the actual constructor-level exception. Consistent with the pattern in test/Intent/Runner.cs:78.
• Edge case — TIE with null InnerException: the is not null guard correctly falls back to ex (the TIE itself), so exception = new Exception(tie.Message, tie). Nothing is lost.

Can TargetInvocationException actually reach this catch? ✅

I traced the call paths. RunTestInternalWithExecutors has its own per-executor catch (Exception e) that swallows exceptions from individual executor runs (lines 518–538). What escapes to the outer catch are pre-loop failures — primarily GetExecutorUriExtensionMap and extension manager creation. Both go through Activator.CreateInstance / reflection, so TargetInvocationException is a realistic exception type here. The unwrap is appropriately scoped.

Test coverage ✅

Both new tests use reference equality (Assert.AreSame) — the right check since instance identity is what matters here. The TargetInvocationException test correctly verifies that the TIE's inner exception becomes Error.InnerException (i.e., the wrapper is fully dropped). The normal exception test verifies the original instance survives the wrap. Both are correct.

Prior observation still applies (non-blocking)

As noted in the previous review: Error.GetType() at the top level is still System.Exception, not the original exception type. The real type is accessible via Error.InnerException.GetType(). This is a pre-existing design choice in the wrapping pattern and is out of scope for this fix.

IPC/wire compatibility ✅

Unchanged from the previous review — null → populated InnerException is wire-additive and non-breaking for old readers.

---

No new blocking issues introduced by the TargetInvocationException unwrapping. The logic is correct, edge-case-safe, and consistent with the codebase convention.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠