Skip to content
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

Merged
merged 10 commits into from
Apr 6, 2017
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,14 @@ public interface ICommunicationManager
/// <param name="payload">payload to be sent</param>
void SendMessage(string messageType, object payload);

/// <summary>
/// Writes message to the binary writer with payload
/// </summary>
/// <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>
void SendMessage(string messageType, object payload, int version);

/// <summary>
/// Send serialized raw message
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,14 @@ public interface IDataSerializer
/// <param name="payload">Payload of the message</param>
/// <returns>Raw Serialized message</returns>
string SerializePayload(string messageType, object payload);

/// <summary>
/// Serializes and creates a raw message given a message type and the object payload
/// </summary>
/// <param name="messageType">Message Type</param>
/// <param name="payload">Payload of the message</param>
/// <param name="version">version to be sent</param>
/// <returns>Raw Serialized message</returns>
string SerializePayload(string messageType, object payload, int version);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ public interface ITestRequestSender : IDisposable
/// <returns>Port Number of the communication channel</returns>
int InitializeCommunication();

/// <summary>
/// Used for protocol version check with TestHost
/// </summary>
/// <returns>true if the version check is successful</returns>
bool CheckVersionWithTestHost();
Copy link
Contributor

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?

Copy link
Contributor

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?


/// <summary>
/// Waits for Request Handler to be connected
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities
using System.IO;

using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Serialization;

using Newtonsoft.Json;
Expand All @@ -20,26 +19,33 @@ public class JsonDataSerializer : IDataSerializer
{
private static JsonDataSerializer instance;

private static JsonSerializer serializer;
private static JsonSerializer payloadSerializer;
private static JsonSerializer payloadSerializer2;
Copy link
Contributor

Choose a reason for hiding this comment

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

an array maybe?


/// <summary>
/// Prevents a default instance of the <see cref="JsonDataSerializer"/> class from being created.
/// </summary>
private JsonDataSerializer()
{
serializer = JsonSerializer.Create(
new JsonSerializerSettings
{
ContractResolver = new TestPlatformContractResolver(),
DateFormatHandling = DateFormatHandling.IsoDateFormat,
DateParseHandling = DateParseHandling.DateTimeOffset,
DateTimeZoneHandling = DateTimeZoneHandling.Utc,
TypeNameHandling = TypeNameHandling.None
});
var jsonSettings = new JsonSerializerSettings
{
DateFormatHandling = DateFormatHandling.IsoDateFormat,
DateParseHandling = DateParseHandling.DateTimeOffset,
DateTimeZoneHandling = DateTimeZoneHandling.Utc,
TypeNameHandling = TypeNameHandling.None
};

payloadSerializer = JsonSerializer.Create(jsonSettings);
payloadSerializer2 = JsonSerializer.Create(jsonSettings);

payloadSerializer.ContractResolver = new TestPlatformContractResolver1();
payloadSerializer2.ContractResolver = new DefaultTestPlatformContractResolver();
Copy link
Contributor

@codito codito Apr 4, 2017

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.


#if DEBUG
// MemoryTraceWriter can help diagnose serialization issues. Enable it for
// debug builds only.
serializer.TraceWriter = new MemoryTraceWriter();
payloadSerializer.TraceWriter = new MemoryTraceWriter();
payloadSerializer2.TraceWriter = new MemoryTraceWriter();
#endif
}

Expand All @@ -61,7 +67,9 @@ public static JsonDataSerializer Instance
/// <returns>A <see cref="Message"/> instance.</returns>
public Message DeserializeMessage(string rawMessage)
{
return JsonConvert.DeserializeObject<Message>(rawMessage);
// Convert to VersionedMessage
// Message can be deserialized to VersionedMessage where version will be 0
return JsonConvert.DeserializeObject<VersionedMessage>(rawMessage);
}

/// <summary>
Expand All @@ -74,28 +82,24 @@ public T DeserializePayload<T>(Message message)
{
T retValue = default(T);

// TODO: Currently we use json serializer auto only for non-testmessage types
// CHECK: Can't we just use auto for everything
if (MessageType.TestMessage.Equals(message.MessageType))
{
retValue = message.Payload.ToObject<T>();
}
Copy link
Contributor

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.

else
{
retValue = message.Payload.ToObject<T>(serializer);
}
var versionedMessage = message as VersionedMessage;
Copy link
Contributor

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.

var serializer = versionedMessage?.Version == 2 ? payloadSerializer2 : payloadSerializer;
Copy link
Contributor

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.


retValue = message.Payload.ToObject<T>(serializer);
return retValue;
}

/// <summary>
/// Deserialize raw JSON to an object using the default serializer.
/// </summary>
/// <param name="json">JSON string.</param>
/// <param name="version">Version of serializer to be used.</param>
/// <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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

{
var serializer = version == 2 ? payloadSerializer2 : payloadSerializer;

using (var stringReader = new StringReader(json))
using (var jsonReader = new JsonTextReader(stringReader))
{
Expand All @@ -121,30 +125,41 @@ public string SerializeMessage(string messageType)
/// <returns>Serialized message.</returns>
public string SerializePayload(string messageType, object payload)
{
JToken serializedPayload = null;

// TODO: Currently we use json serializer auto only for non-testmessage types
// CHECK: Can't we just use auto for everything
if (MessageType.TestMessage.Equals(messageType))
{
serializedPayload = JToken.FromObject(payload);
}
else
{
serializedPayload = JToken.FromObject(payload, serializer);
}
var serializedPayload = JToken.FromObject(payload, payloadSerializer);
Copy link
Contributor

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)?

Copy link
Contributor Author

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


return JsonConvert.SerializeObject(new Message { MessageType = messageType, Payload = serializedPayload });
}

/// <summary>
/// Serialize a message with payload.
/// </summary>
/// <param name="messageType">Type of the message.</param>
/// <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)
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this tested?

{
var serializer = version == 2 ? payloadSerializer2 : payloadSerializer;
var serializedPayload = JToken.FromObject(payload, serializer);

var message = version == 1 ?
new Message { MessageType = messageType, Payload = serializedPayload } :
new VersionedMessage { MessageType = messageType, Version = version, Payload = serializedPayload };

return JsonConvert.SerializeObject(message);
}

/// <summary>
/// Serialize an object to JSON using default serialization settings.
/// </summary>
/// <typeparam name="T">Type of object to serialize.</typeparam>
/// <param name="data">Instance of the object to serialize.</param>
/// <param name="version">Version to be stamped.</param>
/// <returns>JSON string.</returns>
public string Serialize<T>(T data)
public string Serialize<T>(T data, int version = 1)
Copy link
Contributor

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?

{
var serializer = version == 2 ? payloadSerializer2 : payloadSerializer;

using (var stringWriter = new StringWriter())
using (var jsonWriter = new JsonTextWriter(stringWriter))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

/// <summary>
/// Construct used for communication
/// </summary>
public class Message
{
/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ public static class MessageType
/// </summary>
public const string VersionCheck = "ProtocolVersion";

/// <summary>
/// Protocol Error
/// </summary>
public const string ProtocolError = "ProtocolError";

/// <summary>
/// The session start.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities
{
/// <summary>
/// New construct with version used for communication
Copy link
Contributor

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+.

/// </summary>
public class VersionedMessage : Message
{
/// <summary>
/// Gets or sets the version of the message
/// </summary>
public int Version { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Serializati
/// <summary>
/// JSON contract resolver for mapping test platform types.
/// </summary>
public class TestPlatformContractResolver : DefaultContractResolver
public class DefaultTestPlatformContractResolver : DefaultContractResolver
{
/// <inheritdoc/>
protected override JsonContract CreateContract(Type objectType)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Serialization
{
using System;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Newtonsoft.Json;

/// <inheritdoc/>
public class TestCaseConverter : JsonConverter
Copy link
Contributor

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.

{
/// <inheritdoc/>
public override bool CanRead => false;

/// <inheritdoc/>
public override bool CanConvert(Type objectType)
{
return typeof(TestCase) == objectType;
}

/// <inheritdoc/>
public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)
{
throw new NotImplementedException();
}

/// <inheritdoc/>
public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
{
// P2 to P1
var testCase = value as TestCase;
var properties = testCase.GetProperties();

writer.WriteStartObject();
writer.WritePropertyName("Properties");
writer.WriteStartArray();
foreach (var property in properties)
{
serializer.Serialize(writer, property);
}

writer.WriteEndArray();
writer.WriteEndObject();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Serialization
{
using System;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Newtonsoft.Json.Serialization;

/// <summary>
/// JSON contract resolver for mapping test platform types for v1 serialization.
/// </summary>
public class TestPlatformContractResolver1 : DefaultTestPlatformContractResolver
{
/// <inheritdoc/>
protected override JsonContract CreateContract(Type objectType)
{
var contract = base.CreateContract(objectType);
if (typeof(TestCase) == objectType)
{
contract.Converter = new TestCaseConverter();
}

if (typeof(TestResult) == objectType)
{
contract.Converter = new TestResultConverter();
}

return contract;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Serialization
{
using System;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Newtonsoft.Json;

/// <inheritdoc/>
public class TestResultConverter : JsonConverter
Copy link
Contributor

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{
/// <inheritdoc/>
public override bool CanRead => false;

/// <inheritdoc/>
public override bool CanConvert(Type objectType)
{
return typeof(TestResult) == objectType;
}

/// <inheritdoc/>
public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)
{
throw new NotImplementedException();
}

/// <inheritdoc/>
public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
{
// P2 to P1
var testResult = value as TestResult;
var properties = testResult.GetProperties();

writer.WriteStartObject();
writer.WritePropertyName("TestCase");
serializer.Serialize(writer, testResult.TestCase);
writer.WritePropertyName("Attachments");
serializer.Serialize(writer, testResult.Attachments);
writer.WritePropertyName("Messages");
serializer.Serialize(writer, testResult.Messages);

writer.WritePropertyName("Properties");
writer.WriteStartArray();
foreach (var property in properties)
{
serializer.Serialize(writer, property);
}

writer.WriteEndArray();
writer.WriteEndObject();
}
}
}
Loading