[fix] Fix datacollector crash visibility: replace Assert with throwable exceptions

[nohwnd]

🤖 This is an automated fix generated by the Issue Repro Triage & Auto-Fix workflow.

Fixes #15622

Root Cause

When TestElement is null in DataCollectionManager.TestCaseStarted or TestCaseEnded, the code called TPDebug.Assert(testCaseStartEventArgs.TestElement is not null, "...").

TPDebug.Assert delegates to Debug.Assert, which in debug builds (and when no debugger is attached) calls Environment.FailFast through the default trace listener. This terminates the datacollector process immediately, bypassing the try-catch in DataCollectionTestCaseEventHandler.ProcessRequests. As a result, the error surfaces only as a cryptic crash in stderr (Process terminated. Assertion failed.) that is easy to miss in test output.

Fix

Replace the TPDebug.Assert null checks with explicit if/throw statements that throw InvalidOperationException. Since these are inside the try-catch in ProcessRequests, the exception is:

1. Caught by the exception handler
2. Reported via _messageSink.SendMessage(new DataCollectionMessageEventArgs(TestMessageLevel.Error, ...)) — visible in test output
3. Logged via EqtTrace.Error
4. The data collection continues without crashing the entire process

// Before (crashes the process in debug builds)
TPDebug.Assert(testCaseStartEventArgs.TestElement is not null, "testCaseStartEventArgs.TestElement is null");

// After (caught, logged, and reported as a user-visible error)
if (testCaseStartEventArgs.TestElement is null)
{
    throw new InvalidOperationException("TestCaseStartEventArgs.TestElement is null...");
}

Tests

All 390 Microsoft.TestPlatform.Common.UnitTests pass with the change applied.

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates DataCollectionManager to avoid TPDebug.Assert-triggered process termination when TestElement is missing, so failures become catchable/reportable errors instead of a silent datacollector crash.

Changes:

• Replace TPDebug.Assert(...TestElement...) with explicit if/throw InvalidOperationException in TestCaseStarted.
• Replace TPDebug.Assert(...TestElement...) with explicit if/throw InvalidOperationException in TestCaseEnded.

[nohwnd]

Review Summary

Dimensions activated: Backward Compatibility & Rollback Safety · Error Reporting & Diagnostic Clarity · Null Safety & Boundary Validation

The fix is correct and the PR description accurately describes the change.

Key verification points:

• TPDebug.Assert is [Conditional("DEBUG")] — a no-op in release builds. In debug builds it delegates to Debug.Assert, which can call FailFast and bypass the try/catch in ProcessRequests. The new if/throw InvalidOperationException enforces the null check in all build configurations.
• Both new exceptions are caught by the existing try/catch blocks in DataCollectionTestCaseEventHandler.ProcessRequests (TestCaseStarted handler at ~line 88, TestCaseEnded handler at ~line 112) and correctly surfaced via _messageSink.SendMessage(new DataCollectionMessageEventArgs(TestMessageLevel.Error, ...)).
• TestCaseEnded still sends DataCollectionTestEndResult with an empty AttachmentSet on error, which prevents the vstest.console caller from deadlocking while waiting for the result.
• No public API surface changes; IDataCollectionManager interface is unchanged.

No issues found. ✅

---

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠