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

Added progress support for LSP #1

Closed
wants to merge 13 commits into from
167 changes: 167 additions & 0 deletions src/StreamJsonRpc.Tests/JsonRpcClient20InteropTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.VisualStudio.Threading;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
using StreamJsonRpc;
Expand Down Expand Up @@ -149,6 +150,147 @@ public async Task InvokeWithParameterPassedAsObjectAsync_NoParameter()
Assert.Null(request["params"]);
}

[Fact]
public async Task InvokeWithProgressParameterAsArray()
{
int sum = 0;
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(report =>
{
sum += report;
});

int n = 3;
Task<int> invokeTask = this.clientRpc.InvokeAsync<int>("test", new object[] { n, progress });

JToken request = await this.ReceiveAsync();

long progressID = request["params"][1].Value<long>();
Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

.Value() [](start = 46, length = 14)

From a test perspective we don't care if the token is a long or some other JToken. So in the interest of writing resilient tests that fail when the product is faulty but passes when the product changes details that shouldn't matter outside, I suggest you skip the long conversion and store as a JToken. You can directly inject that JToken into your string below and Newtonsoft will serialize it as JSON for you. #Closed


// Send responses as $/progress
int sum2 = 0;
for (int i = 0; i < n; i++)
{
string content = "{ \"jsonrpc\": \"2.0\", \"method\": \"$/progress\", \"params\": { \"token\": " + progressID + ", \"value\": " + i + ", } ,}";
Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

, [](start = 147, length = 2)

Formal JSON should not have trailing commas. The fact that newtonsoft.json accepts them is configurable, and probably not something you mean to be testing in this. i.e. we wouldn't want this test to start failing if we changed streamjsonrpc to stop accepting trailing commas. #Closed

JObject json = JObject.Parse(content);

this.Send(json);

sum2 += i;
}

System.Threading.Thread.Sleep(1000);
Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

System.Threading.Thread.Sleep(1000); [](start = 8, length = 36)

Please never sleep in tests. If you had to insert a delay, use await Task.Delay(1000);. It allows concurrent test execution to go faster.
But in this case, it looks like you've got an unstable test since you're hoping that things happen within a second which on various machines and under different workloads will cause tests to non-deterministically fail. It also causes tests to run substantially slower than necessary since usually this will happen in a split second.
One thing you can do is have your client-side progress handler set an event every time it fires, that you await on. Then you can assert that each individual message is what you expect so we can for example verify that ordering is preserved as well. #Closed

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm unexperienced on how to use the event as a flag that I can await on to make sure the progress report is done, I investigated a little and I thought using ManualResetEvent might be the way, but I tried an approach using that (You can see the code of that approach commented) and it didn't really work.


In reply to: 303654071 [](ancestors = 303654071)

Copy link

Choose a reason for hiding this comment

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

Ya... we don't want to put these events into the product itself. Rather, we want them in the test code that responds to them. I'll conjure up or find you a sample.


In reply to: 304103873 [](ancestors = 304103873,303654071)

Copy link

Choose a reason for hiding this comment

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

Create an AsyncAutoResetEvent local in this test method. Set the event inside your progress callback. await the event where you currently have your WaitOne line.
Also when you await the event, add .WithCancellation(this.TimeoutToken); at the end so that if the event doesn't come in a timely way, the test will fail instead of hang.


In reply to: 304378299 [](ancestors = 304378299,304103873,303654071)

Assert.Equal(sum2, sum);

this.Send(new
{
jsonrpc = "2.0",
id = request["id"].Value<long>(),
Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

.Value() [](start = 30, length = 14)

Similar to another JToken comment, I think you can just leave this conversion off, since your test shouldn't depend on implementation details. #Closed

result = sum,
});

int result = await invokeTask;
Assert.Equal(sum2, result);
}

[Fact]
public async Task InvokeWithProgressParameter()
Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

InvokeWithProgressParameter [](start = 22, length = 27)

all the same comments as the test method above.
There's enough in common between these tests that you may want to refactor them to share a helper method that just takes a delegate that does the original client invocation but does the rest exactly the same. #Closed

{
int sum = 0;
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(report =>
{
sum += report;
});

int n = 3;
Task<int> invokeTask = this.clientRpc.InvokeWithParameterObjectAsync<int>("test", new { Bar = n, Progress = progress });

JToken request = await this.ReceiveAsync();

long progressID = request["params"]["Progress"].Value<long>();

// Send responses as $/progress
int sum2 = 0;
for (int i = 0; i < n; i++)
{
string content = "{ \"jsonrpc\": \"2.0\", \"method\": \"$/progress\", \"params\": { \"token\": " + progressID + ", \"value\": " + i + ", } ,}";
JObject json = JObject.Parse(content);

this.Send(json);

sum2 += i;
}

System.Threading.Thread.Sleep(1000);
Assert.Equal(sum2, sum);

this.Send(new
{
jsonrpc = "2.0",
id = request["id"].Value<long>(),
result = sum,
});

int result = await invokeTask;
Assert.Equal(sum2, result);
}

[Fact]
public async Task InvokeWithProgressParameter_NoMemoryLeakConfirm()
{
Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

This needn't be an interop test. Can we move this to JsonRpcTests? #Closed

WeakReference weakRef = await this.InvokeWithProgressParameter_NoMemoryLeakConfirm_Helper();
GC.Collect();
Assert.False(weakRef.IsAlive);
}

[Fact]
public async Task InvokeWithProgressParameterAsObject_CheckProgressObjectIsReplacedForId()
{
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(report => { });

Task<int> invokeTask = this.clientRpc.InvokeWithParameterObjectAsync<int>("test", new { Bar = "value", Progress = progress });

JToken request = await this.ReceiveAsync();

Assert.Equal(JTokenType.Object, request["params"].Type);
Assert.Equal("value", request["params"]["Bar"].ToString());
Assert.Equal(JTokenType.Integer, request["params"]["Progress"].Type);
}

[Fact]
public async Task InvokeWithProgressParameterAsArray_CheckProgressObjectIsReplacedForId()
{
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(report => { });

Task<int> invokeTask = this.clientRpc.InvokeAsync<int>("test", new object[] { "value", progress });

JToken request = await this.ReceiveAsync();

Assert.Equal(JTokenType.Array, request["params"].Type);
Assert.Equal("value", request["params"][0].ToString());
Assert.Equal(JTokenType.Integer, request["params"][1].Type);
}

Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

I don't know what this test covers (aside from implementation detail) that your prior tests don't already confirm (i.e. that it works). I suggest you remove these tests unless I'm missing something interesting about them. #Closed

[Fact]
public async Task InvokeWithProgressParameter_IncrementalId()
{
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(report => { });

Task<int> invokeTask = this.clientRpc.InvokeAsync<int>("test", new object[] { "value", progress });
JToken request = await this.ReceiveAsync();

long firstId = request["params"][1].Value<long>();

invokeTask = this.clientRpc.InvokeAsync<int>("test", new object[] { "value", progress });
request = await this.ReceiveAsync();

Assert.Equal(firstId + 1, request["params"][1].Value<long>());

invokeTask = this.clientRpc.InvokeAsync<int>("test", new object[] { "value", progress });
request = await this.ReceiveAsync();

Assert.Equal(firstId + 2, request["params"][1].Value<long>());
}

[Fact]
Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

This test is interesting in that it verifies that each request's progress gets a distinct token. But it goes about it by focusing on implementation details. Instead, can we simply create more than one concurrent request and report progress on each and verify that the right client Progress<T> object handles each of our $/progress messages? #Closed

Copy link

Choose a reason for hiding this comment

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

Also, this doesn't feel like it needs to be an interop test. Can we get better coverage for both JSON and MessagePack by moving to JsonRpcTests?


In reply to: 303656982 [](ancestors = 303656982)

public void NotifyWithParameterPassedAsObjectAsync_ThrowsExceptions()
{
Expand Down Expand Up @@ -352,4 +494,29 @@ public async Task ServerReturnsOurRequestIdAsString()
string result = await invokeTask;
Assert.Equal("pass", result);
}

private async Task<WeakReference> InvokeWithProgressParameter_NoMemoryLeakConfirm_Helper()
Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

InvokeWithProgressParameter_NoMemoryLeakConfirm_Helper [](start = 38, length = 54)

This method needs a [MethodImpl(MethodImplOptions.NoInlining)] attribute. Otherwise the JIT can inline the method and defeat the reason we separated it to begin with, causing the test to sometimes fail making it look like a leak where there actually is none. #Closed

{
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(report => { });

WeakReference weakRef = new WeakReference(progress);

Task<int> invokeTask = this.clientRpc.InvokeWithParameterObjectAsync<int>("test", new { Bar = 1, Progress = progress });

JToken request = await this.ReceiveAsync();

this.Send(new
{
jsonrpc = "2.0",
id = request["id"].Value<long>(),
result = 5,
});

// Clear progress variable locally
progress = null;

int result = await invokeTask;

return weakRef;
}
}
96 changes: 96 additions & 0 deletions src/StreamJsonRpc.Tests/JsonRpcTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,58 @@ public async Task InvokeWithParameterObject_DefaultParameters()
Assert.Equal(12, sum);
}

[Fact]
public async Task InvokeWithParameterObject_ProgressParameter()
{
int report = 0;
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(n =>
{
report = n;
});

int result = await this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithProgressParameter), new { p = progress }, this.TimeoutToken);

Assert.Equal(1, report);
Assert.Equal(1, result);
}

[Fact]
public async Task InvokeWithParameterObject_ProgressParameterAndFields()
{
int report = 0;
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(n =>
{
report += n;
});

int sum = await this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithProgressAndMoreParameters), new { p = progress, x = 2, y = 5 }, this.TimeoutToken);

Assert.Equal(7, report);
Assert.Equal(7, sum);
}

[Fact]
public async Task InvokeWithParameterObject_ProgressAndDefaultParameters()
{
int report = 0;
ProgressWithCompletion<int> progress = new ProgressWithCompletion<int>(n =>
{
report += n;
});

int sum = await this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithProgressAndMoreParameters), new { p = progress, x = 2 }, this.TimeoutToken);

Assert.Equal(12, report);
Assert.Equal(12, sum);
}

[Fact]
public async Task InvokeWithParameterObject_ClassIncludingProgressProperty()
{
int sum = await this.clientRpc.InvokeWithParameterObjectAsync<int>(nameof(Server.MethodWithProgressAndMoreParameters), new XAndYFieldsWithProgress { x = 2, y = 5, p = new Progress<int>() }, this.TimeoutToken);
Assert.Equal(7, sum);
}

[Fact]
public async Task CanInvokeServerMethodWithNoParameterPassedAsArray()
{
Expand Down Expand Up @@ -1360,6 +1412,9 @@ private void ReinitializeRpcWithoutListening()
this.serverRpc = new JsonRpc(this.serverMessageHandler, this.server);
this.clientRpc = new JsonRpc(this.clientMessageHandler);

((IJsonRpcInstanceContainer)this.clientMessageFormatter).Rpc = this.clientRpc;
((IJsonRpcInstanceContainer)this.serverMessageFormatter).Rpc = this.serverRpc;

this.serverRpc.TraceSource = new TraceSource("Server", SourceLevels.Verbose);
Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

We shouldn't be setting this in tests. The JsonRpc class itself should set this. #Closed

Copy link
Owner Author

Choose a reason for hiding this comment

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

Not sure I get this comment, do you mean the TraceSource? And what do you mean the JsonRpc should set it, should we create a method in JsonRpc to set his were we send the name and the SourceLevel?


In reply to: 303661727 [](ancestors = 303661727)

Copy link

Choose a reason for hiding this comment

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

Sorry for lack of clarity here: I mean we should not be setting the Rpc property on the formatter. JsonRpc should assign that property when the formatter+handler is first passed to the JsonRpc ctor.


In reply to: 304099982 [](ancestors = 304099982,303661727)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ok, I so when we pass the handler through the constructor the handler should contain the formatter right? But the handler is of type IJsonRpcMessageHandler, so I'd need to cast it to IJsonRpcInstanceContainer so that I can set the JsonRpc instance and not all formatters inherit from IJsonRpcInstanceContainer right? So how should I handle the case where the formatter doesn't have a rpc instance to set? Or maybe I'm misunderstanding again


In reply to: 304378610 [](ancestors = 304378610,304099982,303661727)

Copy link

@AArnott AArnott Jul 17, 2019

Choose a reason for hiding this comment

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

The JsonRpc ctor receives the handler, and the handler has a property on it to get the formatter. So you get the formatter that way, and use an as cast to try to cast the formatter to IJsonRpcInstanceContainer. If the cast succeeds, then you set the property. #Closed

this.clientRpc.TraceSource = new TraceSource("Client", SourceLevels.Verbose);

Expand Down Expand Up @@ -1430,6 +1485,33 @@ public static int MethodWithDefaultParameter(int x, int y = 10)
return x + y;
}

public static int MethodWithProgressParameter(IProgress<int> p)
Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

MethodWithProgressParameter(IProgress p) [](start = 26, length = 45)

We should create a new nested class (sibling to the Server class) that defines just one method which takes a Progress<int> parameter, and then add a test that verifies that this fails when added as a target object in some acceptable way, since we only support server methods that accept IProgress<T> and not the concrete type Progress<T>. #Closed

{
p.Report(1);
return 1;
}

public static int MethodWithProgressAndMoreParameters(IProgress<int> p, int x, int y = 10)
{
int sum = x + y;
p.Report(x);
p.Report(y);
return sum;
}

public static int MethodWithProgressArrayParameter(params IProgress<int>[] progressArray)
{
int report = 0;

foreach (IProgress<int> progress in progressArray)
{
report++;
progress.Report(report);
}

return 0;
}

public int? MethodReturnsNullableInt(int a) => a > 0 ? (int?)a : null;

public int MethodAcceptsNullableArgs(int? a, int? b) => (a.HasValue ? 1 : 0) + (b.HasValue ? 1 : 0);
Expand Down Expand Up @@ -1742,6 +1824,20 @@ public class XAndYFields
#pragma warning restore SA1307 // Accessible fields should begin with upper-case letter
}

[DataContract]
public class XAndYFieldsWithProgress
{
// We disable SA1307 because we must use lowercase members as required to match the parameter names.
#pragma warning disable SA1307 // Accessible fields should begin with upper-case letter
[DataMember]
public int x;
[DataMember]
public int y;
[DataMember]
public IProgress<int> p;
#pragma warning restore SA1307 // Accessible fields should begin with upper-case letter
}

internal class InternalClass
{
}
Expand Down
53 changes: 53 additions & 0 deletions src/StreamJsonRpc.Tests/MessagePackFormatterTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,59 @@ public void JsonRpcRequest_ArgsArray()
Assert.Equal(((CustomType)original.ArgumentsList[2]).Age, ((CustomType)actual.ArgumentsList[2]).Age);
}

[Fact]
public void JsonRpcRequest_ProgressArg()
{
var original = new JsonRpcRequest
{
Id = 5,
Method = "test",
ArgumentsList = new object[] { new Progress<object>() },
};

var actual = this.Roundtrip(original);
Assert.Equal(original.Id, actual.Id);
Assert.Equal(original.Method, actual.Method);
Assert.Equal(typeof(long), actual.ArgumentsList[0].GetType());
}

[Fact]
public void JsonRpcRequest_MultipleProgressArgs()
{
var original = new JsonRpcRequest
{
Id = 5,
Method = "test",
ArgumentsList = new object[] { new Progress<object>(), new Progress<object>(), new Progress<object>() },
};

var actual = this.Roundtrip(original);
Assert.Equal(original.Id, actual.Id);
Assert.Equal(original.Method, actual.Method);
Assert.Equal(typeof(long), actual.ArgumentsList[0].GetType());
Assert.Equal(typeof(long), actual.ArgumentsList[1].GetType());
Assert.Equal(typeof(long), actual.ArgumentsList[2].GetType());
}

[Fact]
public void JsonRpcRequest_MultipleArgsIncludingProgress()
{
var original = new JsonRpcRequest
{
Id = 5,
Method = "test",
ArgumentsList = new object[] { 0, new Progress<object>(), "foo", new ProgressWithCompletion<object>(report => { }) },
};

var actual = this.Roundtrip(original);
Assert.Equal(original.Id, actual.Id);
Assert.Equal(original.Method, actual.Method);
Assert.Equal(original.ArgumentsList[0], actual.ArgumentsList[0]);
Assert.Equal(typeof(long), actual.ArgumentsList[1].GetType());
Assert.Equal(original.ArgumentsList[2], actual.ArgumentsList[2]);
Assert.Equal(typeof(long), actual.ArgumentsList[3].GetType());
}

[Fact]
Copy link

@AArnott AArnott Jul 15, 2019

Choose a reason for hiding this comment

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

I want tests to verify that MessagePack works, but it's not clear to me that these tests verify anything interesting other than an implementation detail which represents just a small part of the overall feature. #Closed

Copy link
Owner Author

Choose a reason for hiding this comment

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

I was not sure how to implement this test, should I add a round trip for a progress notification as well? And maybe for a JsonResponse?


In reply to: 303664893 [](ancestors = 303664893)

Copy link

Choose a reason for hiding this comment

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

Well, given this is a formatter-specific test class, I suppose use of JsonRpc itself is inappropriate here. We do want to make sure that the JsonRpcTests are exercising the MessagePackFormatter and you've already done that. So at this point, I'm not sure these tests are worthwhile since the same functionality is tested at a higher level.


In reply to: 304098095 [](ancestors = 304098095,303664893)

public void JsonRpcResult()
{
Expand Down
20 changes: 20 additions & 0 deletions src/StreamJsonRpc/IJsonRpcInstanceContainer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace StreamJsonRpc
{
/// <summary>
/// Interface to contain an instance of <see cref="JsonRpc"/>.
Copy link

@AArnott AArnott Jul 17, 2019

Choose a reason for hiding this comment

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

Interface to contain an instance of [](start = 8, length = 35)

Reword for clarity for others: Interface optionally implemented by <see cref="IJsonRpcMessageFormatter" /> implementations that need a reference to their owner <see cref="JsonRpc" /> class. #Closed

/// </summary>
public interface IJsonRpcInstanceContainer
{
/// <summary>
/// Gets or sets the <see cref="JsonRpc"/> instance.
/// </summary>
JsonRpc Rpc
{
Copy link

@AArnott AArnott Jul 17, 2019

Choose a reason for hiding this comment

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

Also add:

/// <exception cref="System.InvalidOperationException">May be thrown when set more than once.</exception>
``` #Closed

get;
set;
milopezc marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Loading