-
Notifications
You must be signed in to change notification settings - Fork 324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Protocol v2 changes #672
Protocol v2 changes #672
Conversation
2. Protocol version check between runner and host 3. Object model changes for reducing the verbosity. 4. Serializers for supported protocol versions. 5. Unit tests
@singhsarab, |
Changing the TestCase and TestResult objects, and cleaned up the converters.
/// Used for protocol version check with TestHost | ||
/// </summary> | ||
/// <returns>true if the version check is successful</returns> | ||
bool CheckVersionWithTestHost(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this be used for data collector as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Datacollector will always be bundled with the test runner. So they can communicate in any protocol without any negotiation?
retValue = message.Payload.ToObject<T>(serializer); | ||
} | ||
var versionedMessage = message as VersionedMessage; | ||
var serializer = versionedMessage?.Version == 2 ? payloadSerializer2 : payloadSerializer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have a GetPayloadSerializer(Constants.CurrentVersion)
for this? This can then be reused everywhere here.
if (MessageType.TestMessage.Equals(message.MessageType)) | ||
{ | ||
retValue = message.Payload.ToObject<T>(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming MessageType.TestMessage
would continue to work post this change.
@@ -20,26 +19,33 @@ public class JsonDataSerializer : IDataSerializer | |||
{ | |||
private static JsonDataSerializer instance; | |||
|
|||
private static JsonSerializer serializer; | |||
private static JsonSerializer payloadSerializer; | |||
private static JsonSerializer payloadSerializer2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an array maybe?
/// <returns>JSON string.</returns> | ||
public string Serialize<T>(T data) | ||
public string Serialize<T>(T data, int version = 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this by default be the highest?
@@ -28,18 +28,29 @@ public sealed class TestRequestSender : ITestRequestSender | |||
|
|||
private IDataSerializer dataSerializer; | |||
|
|||
// TODO:sasin Change the version to 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when would this change to 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely in next PR, once IDE to runner is complete.
@@ -76,23 +87,51 @@ public void Close() | |||
} | |||
|
|||
/// <inheritdoc/> | |||
public bool CheckVersionWithTestHost() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Handling special case for dotnet core projects with older test hosts | ||
// Older test hosts are not aware of protocol version check | ||
// Hence we should not be sending VersionCheck message to these test hosts | ||
bool checkRequired = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it the ProxyOperationManagers responsibility to perform this check? Or should this be part of an Initialize
in RequestSender?
// Hence we should not be sending VersionCheck message to these test hosts | ||
bool checkRequired = true; | ||
var property = this.testHostManager.GetType().GetRuntimeProperties().FirstOrDefault(p => string.Equals(p.Name, versionCheckPropertyName, StringComparison.OrdinalIgnoreCase)); | ||
if (property != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't really know if there is something better we can do here. Seems odd :(
Maybe have a config file with the version and check for its existence?
@@ -29,6 +29,11 @@ public abstract class TestObject | |||
private readonly Dictionary<TestProperty, object> store; | |||
|
|||
/// <summary> | |||
/// The store for all the local properties registered. | |||
/// </summary> | |||
private readonly Dictionary<TestProperty, object> localStore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this wont be a breaking change to extensions right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it wont.
@@ -383,12 +383,7 @@ public static class TestResultProperties | |||
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Security", "CA2104:DoNotDeclareReadOnlyMutableReferenceTypes")] | |||
public static readonly TestProperty ErrorStackTrace = TestProperty.Register("TestResult.ErrorStackTrace", Resources.Resources.TestResultPropertyErrorStackTraceLabel, typeof(string), typeof(TestResult)); | |||
#endif | |||
|
|||
private static bool ValidateComputerName(object value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't like this? Why?
payloadSerializer2 = JsonSerializer.Create(jsonSettings); | ||
|
||
payloadSerializer.ContractResolver = new TestPlatformContractResolver1(); | ||
payloadSerializer2.ContractResolver = new DefaultTestPlatformContractResolver(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When v3 comes in, we need to change this code? Why not consider versioning the ContractResolvers suffixed with version number. The class name need not say what's default, let it be determined only in this file.
E.g. TestPlatformContractResolver2
for payloadSerializer2
.
{ | ||
retValue = message.Payload.ToObject<T>(serializer); | ||
} | ||
var versionedMessage = message as VersionedMessage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please log the default serializer determined here.
/// <typeparam name="T">Target type to deserialize.</typeparam> | ||
/// <returns>An instance of <see cref="T"/>.</returns> | ||
public T Deserialize<T>(string json) | ||
public T Deserialize<T>(string json, int version = 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the caller be aware of version
of payload? How would it know, since the version is embedded inside the json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just the payload deserialization, caller reads the version from the message.
{ | ||
serializedPayload = JToken.FromObject(payload, serializer); | ||
} | ||
var serializedPayload = JToken.FromObject(payload, payloadSerializer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these APIs not using versioned messages? How would the caller decide which API to use (and when)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to change the APIs.
This is used for payload serialization/deserialization. Currently, the APIs caller reads the version beforehand
namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities | ||
{ | ||
/// <summary> | ||
/// New construct with version used for communication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New
becomes old ;) Better to not keep a sense of time in documentation. Add a remark specifying which version of vstest this was introduced and that this is the default message for protocol 2+.
using Newtonsoft.Json; | ||
|
||
/// <inheritdoc/> | ||
public class TestCaseConverter : JsonConverter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear when this is used, why is it required. Require documentation.
using Newtonsoft.Json; | ||
|
||
/// <inheritdoc/> | ||
public class TestResultConverter : JsonConverter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Require documentation. Should we include these classes in versioned folders (same for TestCaseConverter)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -28,6 +28,8 @@ public class TestCaseSerializationTests | |||
Traits = { new Trait("Priority", "0"), new Trait("Category", "unit") } | |||
}; | |||
|
|||
#region v1 Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Require test where a TestCase doesn't have any properties and can be serialized properly.
/// <param name="payload">Payload for the message.</param> | ||
/// <param name="version">Version for the message.</param> | ||
/// <returns>Serialized message.</returns> | ||
public string SerializePayload(string messageType, object payload, int version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this tested?
Assert.AreEqual("TestResult.EndTime", properties[7]["Key"]["Id"].Value); | ||
Assert.AreEqual(DateTimeOffset.MaxValue.Year, ((DateTimeOffset)properties[7]["Value"].Value).Year); | ||
} | ||
|
||
[TestMethod] | ||
public void TestResultObjectShouldContainAllPropertiesOnDeserialization() | ||
{ | ||
var json = "{\"TestCase\":{\"Properties\":[{\"Key\":{\"Id\":\"TestCase.FullyQualifiedName\",\"Label\":\"FullyQualifiedName\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":1,\"ValueType\":\"System.String\"},\"Value\":\"sampleTestClass.sampleTestCase\"},{\"Key\":{\"Id\":\"TestCase.ExecutorUri\",\"Label\":\"Executor Uri\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":1,\"ValueType\":\"System.Uri\"},\"Value\":\"executor://sampleTestExecutor\"},{\"Key\":{\"Id\":\"TestCase.Source\",\"Label\":\"Source\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.String\"},\"Value\":\"sampleTest.dll\"}]},\"Attachments\":[],\"Messages\":[],\"Properties\":[{\"Key\":{\"Id\":\"TestResult.Outcome\",\"Label\":\"Outcome\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"Microsoft.VisualStudio.TestPlatform.ObjectModel.TestOutcome\"},\"Value\":1},{\"Key\":{\"Id\":\"TestResult.ErrorMessage\",\"Label\":\"Error Message\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.String\"},\"Value\":\"sampleError\"},{\"Key\":{\"Id\":\"TestResult.ErrorStackTrace\",\"Label\":\"Error Stack Trace\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.String\"},\"Value\":\"sampleStackTrace\"},{\"Key\":{\"Id\":\"TestResult.DisplayName\",\"Label\":\"TestResult Display Name\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":1,\"ValueType\":\"System.String\"},\"Value\":\"sampleTestResult\"},{\"Key\":{\"Id\":\"TestResult.ComputerName\",\"Label\":\"Computer Name\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.String\"},\"Value\":\"sampleComputerName\"},{\"Key\":{\"Id\":\"TestResult.Duration\",\"Label\":\"Duration\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.TimeSpan\"},\"Value\":\"10675199.02:48:05.4775807\"},{\"Key\":{\"Id\":\"TestResult.StartTime\",\"Label\":\"Start Time\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.DateTimeOffset\"},\"Value\":\"0001-01-01T00:00:00+00:00\"},{\"Key\":{\"Id\":\"TestResult.EndTime\",\"Label\":\"End Time\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.DateTimeOffset\"},\"Value\":\"9999-12-31T23:59:59.9999999+00:00\"}]}"; | ||
var json = "{\"TestCase\":{\"Properties\":[{\"Key\":{\"Id\":\"TestCase.FullyQualifiedName\",\"Label\":\"FullyQualifiedName\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":1,\"ValueType\":\"System.String\"},\"Value\":\"sampleTestClass.sampleTestCase\"},{\"Key\":{\"Id\":\"TestCase.ExecutorUri\",\"Label\":\"Executor Uri\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":1,\"ValueType\":\"System.Uri\"},\"Value\":\"executor://sampleTestExecutor\"},{\"Key\":{\"Id\":\"TestCase.Source\",\"Label\":\"Source\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.String\"},\"Value\":\"sampleTest.dll\"}]},\"Attachments\":[],\"Messages\":[],\"Properties\":[{\"Key\":{\"Id\":\"TestResult.Outcome\",\"Label\":\"Outcome\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"Microsoft.VisualStudio.TestPlatform.ObjectModel.TestOutcome\"},\"Value\":1},{\"Key\":{\"Id\":\"TestResult.ErrorMessage\",\"Label\":\"Error Message\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.String\"},\"Value\":\"sampleError\"},{\"Key\":{\"Id\":\"TestResult.ErrorStackTrace\",\"Label\":\"Error Stack Trace\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.String\"},\"Value\":\"sampleStackTrace\"},{\"Key\":{\"Id\":\"TestResult.DisplayName\",\"Label\":\"TestResult Display Name\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":1,\"ValueType\":\"System.String\"},\"Value\":\"sampleTestResult\"},{\"Key\":{\"Id\":\"TestResult.ComputerName\",\"Label\":\"Computer Name\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.String\"},\"Value\":\"sampleComputerName\"},{\"Key\":{\"Id\":\"TestResult.Duration\",\"Label\":\"Duration\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.TimeSpan\"},\"Value\":\"10675199.02:48:05.4775807\"},{\"Key\":{\"Id\":\"TestResult.StartTime\",\"Label\":\"Start Time\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.DateTimeOffset\"},\"Value\":\"2007-03-10T00:00:00+00:00\"},{\"Key\":{\"Id\":\"TestResult.EndTime\",\"Label\":\"End Time\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.DateTimeOffset\"},\"Value\":\"9999-12-31T23:59:59.9999999+00:00\"}]}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the testcase to have realistic value rather than default min value.
@@ -90,7 +93,6 @@ public void TestResultObjectShouldSerializeAttachments() | |||
var result = new TestResult(testCase); | |||
result.Attachments.Add(new AttachmentSet(new Uri("http://dummyUri"), "sampleAttachment")); | |||
var expectedJson = "{\"TestCase\":{\"Properties\":[{\"Key\":{\"Id\":\"TestCase.FullyQualifiedName\",\"Label\":\"FullyQualifiedName\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":1,\"ValueType\":\"System.String\"},\"Value\":\"sampleTestClass.sampleTestCase\"},{\"Key\":{\"Id\":\"TestCase.ExecutorUri\",\"Label\":\"Executor Uri\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":1,\"ValueType\":\"System.Uri\"},\"Value\":\"executor://sampleTestExecutor\"},{\"Key\":{\"Id\":\"TestCase.Source\",\"Label\":\"Source\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.String\"},\"Value\":\"sampleTest.dll\"}]},\"Attachments\":[{\"Uri\":\"http://dummyUri\",\"DisplayName\":\"sampleAttachment\",\"Attachments\":[]}],\"Messages\":[],\"Properties\":[]}"; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please undo. New line is required between Arrange
and Act
parts of a test method.
var result = new TestResult(testCase); | ||
result.Attachments.Add(new AttachmentSet(new Uri("http://dummyUri"), "sampleAttachment")); | ||
var expectedJson = "{\"TestCase\":{\"Properties\":[{\"Key\":{\"Id\":\"TestCase.FullyQualifiedName\",\"Label\":\"FullyQualifiedName\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":1,\"ValueType\":\"System.String\"},\"Value\":\"sampleTestClass.sampleTestCase\"},{\"Key\":{\"Id\":\"TestCase.ExecutorUri\",\"Label\":\"Executor Uri\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":1,\"ValueType\":\"System.Uri\"},\"Value\":\"executor://sampleTestExecutor\"},{\"Key\":{\"Id\":\"TestCase.Source\",\"Label\":\"Source\",\"Category\":\"\",\"Description\":\"\",\"Attributes\":0,\"ValueType\":\"System.String\"},\"Value\":\"sampleTest.dll\"}]},\"Attachments\":[{\"Uri\":\"http://dummyUri\",\"DisplayName\":\"sampleAttachment\",\"Attachments\":[]}],\"Messages\":[],\"Properties\":[]}"; | ||
var json = Serialize(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New line between arrange and act. Fix elsewhere in file.
/// <param name="messageType">Type of Message to be sent, for instance TestSessionStart</param> | ||
/// <param name="payload">payload to be sent</param> | ||
/// <param name="version">version to be sent</param> | ||
public void SendMessage(string messageType, object payload, int version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Require tests?
} | ||
else if (message.MessageType == MessageType.ProtocolError) | ||
{ | ||
EqtTrace.Error("TestRequestSender: VersionCheck Failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protocol.Error doesn't specify what's the highest version supported at the other end? How does user find why version check failed?
var protocolVersion = this.dataSerializer.DeserializePayload<int>(message); | ||
|
||
// TODO: Should we check if this is valid ? | ||
this.highestNegotiatedVersion = protocolVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log this please. What does TODO
mean?
// Handling special case for dotnet core projects with older test hosts | ||
// Older test hosts are not aware of protocol version check | ||
// Hence we should not be sending VersionCheck message to these test hosts | ||
bool checkRequired = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this move out as an static extension method to ITestHostManager
?
@@ -212,20 +231,20 @@ public void Close() | |||
/// <inheritdoc/> | |||
public void SendTestCases(IEnumerable<TestCase> discoveredTestCases) | |||
{ | |||
this.communicationManager.SendMessage(MessageType.TestCasesFound, discoveredTestCases); | |||
this.communicationManager.SendMessage(MessageType.TestCasesFound, discoveredTestCases, protocolVersion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.protocolVersion
@@ -277,7 +308,7 @@ public void StartTestRunWithSourcesShouldCallHandleTestRunStatsChange() | |||
|
|||
waitHandle.WaitOne(); | |||
|
|||
this.mockCommunicationManager.Verify(mc => mc.SendMessage(MessageType.StartTestExecutionWithSources, runCriteria), Times.Once); | |||
this.mockCommunicationManager.Verify(mc => mc.SendMessage(MessageType.StartTestExecutionWithSources, runCriteria, this.version), Times.Once); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to extract a method VerifySendMessage(messageType, payload)
for all repetitions.
// Can only do this after InitializeCommunication because TestHost cannot "Send Log" unless communications are initialized | ||
if (!string.IsNullOrEmpty(EqtTrace.LogFile)) | ||
{ | ||
this.SendLog(TestMessageLevel.Informational, string.Format("Logging TestHost Diagnostics in file: {0}", EqtTrace.LogFile)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be added to a resource?
@@ -269,7 +295,7 @@ private void PrivateSetPropertyValue(TestProperty property, object value) | |||
/// Convert passed in value from TestProperty's specified value type. | |||
/// </summary> | |||
/// <returns>Converted object</returns> | |||
private static object ConvertPropertyFrom<T>(TestProperty property, CultureInfo culture, object value) | |||
internal static object ConvertPropertyFrom<T>(TestProperty property, CultureInfo culture, object value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required now. Switched back.
/// <summary> | ||
/// Set TestProperty's value to the specified value T. | ||
/// </summary> | ||
protected void SetLocalPropertyValue<T>(TestProperty property, T value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is the right thing to do. In protocol V2 too, user will unnecessarily pay the cost of an additional dictionary and hashing several properties in it.
Related to #396.