Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ internal class DataCollectionRequestHandler : IDataCollectionRequestHandler, IDi
private readonly IFileHelper _fileHelper;
private readonly IRequestData _requestData;

// The protocol version negotiated with the vstest.console sender.
// Set when BeforeTestRunStart is received; used for all responses on this channel.
private int _protocolVersion = 1;

private Task? _testCaseEventMonitorTask;

/// <summary>
Expand Down Expand Up @@ -212,7 +216,7 @@ public void ProcessRequests()
/// </param>
public void SendDataCollectionMessage(DataCollectionMessageEventArgs args)
{
_communicationManager.SendMessage(MessageType.DataCollectionMessage, args);
_communicationManager.SendMessage(MessageType.DataCollectionMessage, args, _protocolVersion);
}

/// <summary>
Expand Down Expand Up @@ -296,6 +300,14 @@ private void AddExtensionAssemblies(BeforeTestRunStartPayload payload)

private void HandleBeforeTestRunStart(Message message)
{
// Negotiate the protocol version: adopt the highest version that both sides support.
// The sender transmits its highest supported version; we respond with the minimum of
// that and our own highest supported version so all subsequent messages use a mutually
// understood serialization format.
_protocolVersion = message.Version > 0
? Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion)
: 1;

// Initialize datacollectors and get environment variables.
var payload = _dataSerializer.DeserializePayload<BeforeTestRunStartPayload>(message);
TPDebug.Assert(payload is not null, "payload is null");
Expand Down Expand Up @@ -355,7 +367,8 @@ private void HandleBeforeTestRunStart(Message message)

_communicationManager.SendMessage(
MessageType.BeforeTestRunStartResult,
new BeforeTestRunStartResult(envVariables, testCaseEventsPort));
new BeforeTestRunStartResult(envVariables, testCaseEventsPort),
_protocolVersion);

EqtTrace.Info("DataCollectionRequestHandler.ProcessRequests : DataCollection started.");
}
Expand Down Expand Up @@ -395,7 +408,7 @@ private void HandleAfterTestRunEnd(Message message)
// As datacollector process exits itself on parent process(vstest.console) exits.
_dataCollectionManager?.Dispose();

_communicationManager.SendMessage(MessageType.AfterTestRunEndResult, afterTestRunEndResult);
_communicationManager.SendMessage(MessageType.AfterTestRunEndResult, afterTestRunEndResult, _protocolVersion);
EqtTrace.Info("DataCollectionRequestHandler.ProcessRequests : Session End message received from server. Closing the connection.");

Close();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ public sealed class DataCollectionRequestSender : IDataCollectionRequestSender
private readonly ICommunicationManager _communicationManager;
private readonly IDataSerializer _dataSerializer;

// The protocol version negotiated with the datacollector.
// Set after SendBeforeTestRunStartAndGetResult reads the response version.
private int _protocolVersion = 1;

/// <summary>
/// Initializes a new instance of the <see cref="DataCollectionRequestSender"/> class.
/// </summary>
Expand Down Expand Up @@ -94,7 +98,7 @@ public void Close()
/// <inheritdoc/>
public void SendTestHostLaunched(TestHostLaunchedPayload testHostLaunchedPayload)
{
_communicationManager.SendMessage(MessageType.TestHostLaunched, testHostLaunchedPayload);
_communicationManager.SendMessage(MessageType.TestHostLaunched, testHostLaunchedPayload, _protocolVersion);
}

/// <inheritdoc/>
Expand All @@ -112,7 +116,10 @@ public void SendTestHostLaunched(TestHostLaunchedPayload testHostLaunchedPayload
IsTelemetryOptedIn = isTelemetryOptedIn
};

_communicationManager.SendMessage(MessageType.BeforeTestRunStart, payload);
// Send at the highest version this side supports; the datacollector echoes back the
// highest version it supports in the BeforeTestRunStartResult response, which then
// becomes the negotiated version for all subsequent messages on this channel.
_communicationManager.SendMessage(MessageType.BeforeTestRunStart, payload, ProtocolVersioning.HighestSupportedVersion);

while (!isDataCollectionStarted)
{
Expand All @@ -133,6 +140,13 @@ public void SendTestHostLaunched(TestHostLaunchedPayload testHostLaunchedPayload
else if (message.MessageType == MessageType.BeforeTestRunStartResult)
{
isDataCollectionStarted = true;
// Adopt the version the datacollector used in the response as the negotiated
// protocol version for all subsequent messages on this channel.
if (message.Version > 0)
{
_protocolVersion = message.Version;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

[IPC Transport & Protocol Stability] _protocolVersion is adopted directly from message.Version with no Math.Min guard. The same pattern appears in DataCollectionTestCaseEventSender.SendTestCaseStart (line 101).

In practice the handler always responds with Math.Min(request.Version, HighestSupportedVersion), so the echoed value will never exceed HighestSupportedVersion. But if a rogue or out-of-sync handler ever reported a higher version, the sender would try to serialize subsequent messages at an unsupported version and hit NotSupportedException in GetPayloadOptions.

Defense-in-depth suggestion for both sites:

if (message.Version > 0)
{
    _protocolVersion = Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion);
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed. Both DataCollectionRequestSender (line 147) and DataCollectionTestCaseEventSender (line 101) now use _protocolVersion = Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion) when adopting the echoed version, guarding against a rogue/out-of-sync handler reporting a version above HighestSupportedVersion.

πŸ”§ Iterated by PR Iteration Agent πŸ”§

πŸ”§ Iterated by PR Iteration Agent πŸ”§

}
Comment on lines +146 to +149

result = _dataSerializer.DeserializePayload<BeforeTestRunStartResult>(message);
}
else if (message.MessageType == MessageType.TelemetryEventMessage)
Expand All @@ -152,7 +166,7 @@ public void SendTestHostLaunched(TestHostLaunchedPayload testHostLaunchedPayload

EqtTrace.Verbose("DataCollectionRequestSender.SendAfterTestRunStartAndGetResult: Send AfterTestRunEnd message with isCancelled: {0}", isCancelled);

_communicationManager.SendMessage(MessageType.AfterTestRunEnd, isCancelled);
_communicationManager.SendMessage(MessageType.AfterTestRunEnd, isCancelled, _protocolVersion);

// Cycle through the messages that the datacollector sends.
// Currently each of the operations are not separate tasks since they should not each take much time. This is just a notification.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ public void ProcessRequests()
attachmentSets = new Collection<AttachmentSet>();
}

_communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets);
_communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets, message.Version);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

[IPC Transport & Protocol Stability] DataCollectionTestEndResult now correctly echoes message.Version, but DataCollectionTestStartAck (nine lines earlier) is still sent with the 1-arg overload and carries no version. The sender currently ignores the ack version so this is harmless, but the asymmetry is a maintenance hazard: if the sender ever starts tracking version from the ack (e.g. to initialise _protocolVersion on the sub-channel), it would silently read 0 instead of the handler's supported version. For consistency, consider echoing message.Version here too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed. DataCollectionTestStartAck now uses the 3-arg overload and echoes Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion), matching the pattern of DataCollectionTestEndResult. A new test ProcessRequestsShouldEchoNegotiatedVersionInTestCaseStartAck verifies the echo.

πŸ”§ Iterated by PR Iteration Agent πŸ”§

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

[IPC Transport & Protocol Stability] DataCollectionTestStartAck (line 100) uses Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion), but DataCollectionTestEndResult here echoes message.Version unguarded.

In the current negotiated flow this is harmless β€” the sender's _protocolVersion was already bounded by the ack's Math.Min, so the version arriving in DataCollectionTestEnd cannot exceed HighestSupportedVersion. However, the asymmetry is a maintenance hazard: if message.Version ever arrived out of range (buggy/rogue sender), GetPayloadOptions would throw NotSupportedException on this response path while the ack path would not.

For consistency with the ack and with the main-channel handler (DataCollectionRequestHandler.HandleBeforeTestRunStart), consider:

_communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets, Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed. DataCollectionTestEndResult now echoes Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion) for consistency with DataCollectionTestStartAck and the main-channel handler. A new test ProcessRequestsShouldNegotiateVersionInTestCaseEndResult verifies the guard with a sender at version 4.

πŸ”§ Iterated by PR Iteration Agent πŸ”§

πŸ”§ Iterated by PR Iteration Agent πŸ”§


EqtTrace.Info("DataCollectionTestCaseEventHandler: Test case '{0} - {1}' completed", testCaseEndEventArgs?.TestCaseName, testCaseEndEventArgs?.TestCaseId);
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public void Close()
/// <inheritdoc />
public void SendTestCaseStart(TestCaseStartEventArgs e)
{
_communicationManager.SendMessage(MessageType.DataCollectionTestStart, e);
_communicationManager.SendMessage(MessageType.DataCollectionTestStart, e, ProtocolVersioning.HighestSupportedVersion);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

[IPC Transport & Protocol Stability] Unlike the main data collection channel (DataCollectionRequestSender/DataCollectionRequestHandler) β€” which negotiates via Math.Min(request.Version, HighestSupportedVersion) and stores the result in _protocolVersion β€” the test case event sub-channel always sends at HighestSupportedVersion with no negotiation and no _protocolVersion field.

The backward-compat table in the PR description covers the main channel only. For this sub-channel, if the DataCollectionTestCaseEventHandler in the datacollector process is from an older vstest build whose GetPayloadOptions switch does not yet include V7 (the current HighestSupportedVersion), it will throw NotSupportedException on every incoming test-case event.

This is mitigated today because both the STJ and Jsonite implementations already list 7 in their version switches, so any datacollector built against a recent vstest can handle V7 payloads. However:

  • V7 carries no documented change in ProtocolVersioning.cs (no summary comment), so the boundary where "old" becomes "unsafe" is unclear.
  • The handler side properly guards DataCollectionTestEndResult by echoing message.Version, but the sender never uses that echoed value to adapt future sends β€” so the echo only helps the sender deserialize the response, not to detect version mismatches before they happen.

Consider threading the negotiated _protocolVersion from the main-channel handshake into DataCollectionTestCaseEventSender (e.g. via BeforeTestRunStartResult), or adding a minimal echo/ack round-trip on this channel similar to CheckVersionWithTestHostAsync, so the sub-channel enjoys the same Math.Min safety as the main channel.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed. The sub-channel now has proper Math.Min negotiation on both sides:

  • DataCollectionTestCaseEventSender gains a _protocolVersion field (initialized to HighestSupportedVersion). After each SendTestCaseStart, the negotiated version is adopted from the DataCollectionTestStartAck version echo for all subsequent sends (SendTestCaseEnd, SendTestSessionEnd).
  • DataCollectionTestCaseEventHandler now sends DataCollectionTestStartAck with Math.Min(message.Version, ProtocolVersioning.HighestSupportedVersion) so the handler's capability boundary is communicated back to the sender.

New tests verify: (1) DataCollectionTestStartAck echoes the min of sender version and highest supported, (2) SendTestCaseEnd and SendTestSessionEnd use the negotiated version after a test-case start negotiation.

πŸ”§ Iterated by PR Iteration Agent πŸ”§


var message = _communicationManager.ReceiveMessage();
if (message != null && message.MessageType != MessageType.DataCollectionTestStartAck)
Expand All @@ -95,7 +95,7 @@ public void SendTestCaseStart(TestCaseStartEventArgs e)
public Collection<AttachmentSet>? SendTestCaseEnd(TestCaseEndEventArgs e)
{
var attachmentSets = new Collection<AttachmentSet>();
_communicationManager.SendMessage(MessageType.DataCollectionTestEnd, e);
_communicationManager.SendMessage(MessageType.DataCollectionTestEnd, e, ProtocolVersioning.HighestSupportedVersion);

var message = _communicationManager.ReceiveMessage();
if (message != null && message.MessageType == MessageType.DataCollectionTestEndResult)
Expand All @@ -109,6 +109,6 @@ public void SendTestCaseStart(TestCaseStartEventArgs e)
/// <inheritdoc />
public void SendTestSessionEnd(SessionEndEventArgs e)
{
_communicationManager.SendMessage(MessageType.SessionEnd, e);
_communicationManager.SendMessage(MessageType.SessionEnd, e, ProtocolVersioning.HighestSupportedVersion);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,13 @@ public void SendDataCollectionMessageShouldSendMessageToCommunicationManager()

_requestHandler.SendDataCollectionMessage(message);

_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.DataCollectionMessage, message), Times.Once);
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.DataCollectionMessage, message, It.IsAny<int>()), Times.Once);
}

[TestMethod]
public void SendDataCollectionMessageShouldThrowExceptionIfThrownByCommunicationManager()
{
_mockCommunicationManager.Setup(x => x.SendMessage(MessageType.DataCollectionMessage, It.IsAny<DataCollectionMessageEventArgs>())).Throws<Exception>();
_mockCommunicationManager.Setup(x => x.SendMessage(MessageType.DataCollectionMessage, It.IsAny<DataCollectionMessageEventArgs>(), It.IsAny<int>())).Throws<Exception>();
var message = new DataCollectionMessageEventArgs(TestMessageLevel.Error, "message");

Assert.ThrowsExactly<Exception>(() => _requestHandler.SendDataCollectionMessage(message));
Expand Down Expand Up @@ -189,14 +189,14 @@ public void ProcessRequestsShouldProcessRequests()

// Verify SessionStarted events
_mockDataCollectionManager.Verify(x => x.SessionStarted(It.IsAny<SessionStartEventArgs>()), Times.Once);
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.BeforeTestRunStartResult, It.IsAny<BeforeTestRunStartResult>()), Times.Once);
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.BeforeTestRunStartResult, It.IsAny<BeforeTestRunStartResult>(), It.IsAny<int>()), Times.Once);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

[Backward Compatibility & Rollback Safety / Test Coverage] It.IsAny<int>() verifies that the 3-arg SendMessage overload is called, but it does not validate the negotiated version value.

The critical invariant is _protocolVersion = Math.Min(request.Version, HighestSupportedVersion). A test that sends BeforeTestRunStart at a version lower than HighestSupportedVersion (e.g. Version = 4) and then asserts the response uses exactly 4 β€” not 7 β€” would catch regressions where the handler accidentally uses the wrong version (e.g. always responds at HighestSupportedVersion, or always responds at 1). The current tests all set Version = 7, so Math.Min(7, 7) == 7 and the It.IsAny<int>() matcher would accept any wrong value silently.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added. A new test ProcessRequestsShouldNegotiateProtocolVersionToMinOfRequestAndHighest sends BeforeTestRunStart at version 4 (less than HighestSupportedVersion = 7) and asserts that both BeforeTestRunStartResult and AfterTestRunEndResult are sent at exactly version 4 β€” verifying the Math.Min(4, 7) = 4 invariant and catching regressions where the handler might respond at the wrong version.

πŸ”§ Iterated by PR Iteration Agent πŸ”§


// Verify TestHostLaunched events
_mockDataCollectionManager.Verify(x => x.TestHostLaunched(1234), Times.Once);

// Verify AfterTestRun events.
_mockDataCollectionManager.Verify(x => x.SessionEnded(It.IsAny<bool>()), Times.Once);
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.AfterTestRunEndResult, It.IsAny<AfterTestRunEndResult>()), Times.Once);
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.AfterTestRunEndResult, It.IsAny<AfterTestRunEndResult>(), It.IsAny<int>()), Times.Once);
}

[TestMethod]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public void SendBeforeTestRunStartAndGetResultShouldSendBeforeTestRunStartMessag
_mockDataSerializer.Setup(x => x.DeserializeMessage(rawMessage)).Returns(new Message() { MessageType = MessageType.BeforeTestRunStartResult });
_requestSender.SendBeforeTestRunStartAndGetResult(string.Empty, testSources, true, null);

_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.BeforeTestRunStart, It.Is<BeforeTestRunStartPayload>(p => p.SettingsXml == string.Empty && p.IsTelemetryOptedIn)));
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.BeforeTestRunStart, It.Is<BeforeTestRunStartPayload>(p => p.SettingsXml == string.Empty && p.IsTelemetryOptedIn), ProtocolVersioning.HighestSupportedVersion));
}

[TestMethod]
Expand All @@ -140,7 +140,7 @@ public void SendBeforeTestRunStartAndGetResultShouldSendRawMessageIfTelemetry()
_requestSender.SendBeforeTestRunStartAndGetResult(string.Empty, testSources, true, handlerMock.Object);

handlerMock.Verify(x => x.HandleRawMessage(rawMessage1));
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.BeforeTestRunStart, It.Is<BeforeTestRunStartPayload>(p => p.SettingsXml == string.Empty && p.IsTelemetryOptedIn)));
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.BeforeTestRunStart, It.Is<BeforeTestRunStartPayload>(p => p.SettingsXml == string.Empty && p.IsTelemetryOptedIn), ProtocolVersioning.HighestSupportedVersion));
}

[TestMethod]
Expand All @@ -154,6 +154,6 @@ public void SendBeforeTestRunStartAndGetResultShouldNotSendRawMessageIfTelemetry
_mockDataSerializer.Setup(x => x.DeserializeMessage(rawMessage2)).Returns(new Message() { MessageType = MessageType.BeforeTestRunStartResult });
_requestSender.SendBeforeTestRunStartAndGetResult(string.Empty, testSources, true, null);

_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.BeforeTestRunStart, It.Is<BeforeTestRunStartPayload>(p => p.SettingsXml == string.Empty && p.IsTelemetryOptedIn)));
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.BeforeTestRunStart, It.Is<BeforeTestRunStartPayload>(p => p.SettingsXml == string.Empty && p.IsTelemetryOptedIn), ProtocolVersioning.HighestSupportedVersion));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public void ProcessRequestsShouldProcessAfterTestCaseCompleteEvent()
requestHandler.ProcessRequests();

_mockDataCollectionManager.Verify(x => x.TestCaseEnded(It.IsAny<TestCaseEndEventArgs>()), Times.Once);
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.DataCollectionTestEndResult, It.IsAny<Collection<AttachmentSet>>()));
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.DataCollectionTestEndResult, It.IsAny<Collection<AttachmentSet>>(), It.IsAny<int>()));
}

[TestMethod]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,15 +86,15 @@ public void SendTestCaseStartShouldSendMessageThroughCommunicationManager()
var testcaseStartEventArgs = new TestCaseStartEventArgs(_testCase);
_dataCollectionTestCaseEventSender.SendTestCaseStart(testcaseStartEventArgs);

_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.DataCollectionTestStart, testcaseStartEventArgs), Times.Once);
_mockCommunicationManager.Verify(x => x.SendMessage(MessageType.DataCollectionTestStart, testcaseStartEventArgs, ProtocolVersioning.HighestSupportedVersion), Times.Once);
_mockCommunicationManager.Verify(x => x.ReceiveMessage(), Times.Once);
}

[TestMethod]
public void SendTestCaseStartShouldThrowExceptionIfThrownByCommunicationManager()
{
var testcaseStartEventArgs = new TestCaseStartEventArgs(_testCase);
_mockCommunicationManager.Setup(x => x.SendMessage(MessageType.DataCollectionTestStart, testcaseStartEventArgs)).Throws<Exception>();
_mockCommunicationManager.Setup(x => x.SendMessage(MessageType.DataCollectionTestStart, testcaseStartEventArgs, It.IsAny<int>())).Throws<Exception>();

Assert.ThrowsExactly<Exception>(() => _dataCollectionTestCaseEventSender.SendTestCaseStart(testcaseStartEventArgs));
}
Expand All @@ -117,7 +117,7 @@ public void SendTestCaseCompletedShouldThrowExceptionIfThrownByCommunicationMana
{
var testCaseEndEventArgs = new TestCaseEndEventArgs();

_mockCommunicationManager.Setup(x => x.SendMessage(MessageType.DataCollectionTestEnd, It.IsAny<TestCaseEndEventArgs>())).Throws<Exception>();
_mockCommunicationManager.Setup(x => x.SendMessage(MessageType.DataCollectionTestEnd, It.IsAny<TestCaseEndEventArgs>(), It.IsAny<int>())).Throws<Exception>();

Assert.ThrowsExactly<Exception>(() => _dataCollectionTestCaseEventSender.SendTestCaseEnd(testCaseEndEventArgs));
}
Expand Down
Loading