FlorianHockmann commented on code in PR #1923:
URL: https://github.com/apache/tinkerpop/pull/1923#discussion_r1061581137
##########
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/GremlinClientBehaviorIntegrationTests.cs:
##########
@@ -84,5 +84,37 @@ await
gremlinClient.SubmitWithSingleResultAsync<Vertex>(RequestMessage.Build("1"
Assert.NotNull(response2);
Assert.Equal(1, gremlinClient.NrConnections);
}
+
+ [Fact]
+ public async Task ShouldIncludeUserAgentInHandshakeRequest()
+ {
+ var sessionId = Guid.NewGuid().ToString();
Review Comment:
(nitpick) Is the `sessionId` really needed for this test?
##########
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/GremlinClientBehaviorIntegrationTests.cs:
##########
@@ -84,5 +84,37 @@ await
gremlinClient.SubmitWithSingleResultAsync<Vertex>(RequestMessage.Build("1"
Assert.NotNull(response2);
Assert.Equal(1, gremlinClient.NrConnections);
}
+
+ [Fact]
+ public async Task ShouldIncludeUserAgentInHandshakeRequest()
+ {
+ var sessionId = Guid.NewGuid().ToString();
+ var poolSettings = new ConnectionPoolSettings {PoolSize = 1};
+
+ var gremlinServer = new GremlinServer(TestHost, Settings.Port);
+ var gremlinClient = new GremlinClient(gremlinServer,
messageSerializer: Serializer,
Review Comment:
Please add a `using` here to ensure that the `gremlinClient` will be
disposed at the end of this test. The same applies to the other tests in this
class.
##########
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/GremlinClientBehaviorIntegrationTests.cs:
##########
@@ -84,5 +84,37 @@ await
gremlinClient.SubmitWithSingleResultAsync<Vertex>(RequestMessage.Build("1"
Assert.NotNull(response2);
Assert.Equal(1, gremlinClient.NrConnections);
}
+
+ [Fact]
+ public async Task ShouldIncludeUserAgentInHandshakeRequest()
+ {
+ var sessionId = Guid.NewGuid().ToString();
+ var poolSettings = new ConnectionPoolSettings {PoolSize = 1};
Review Comment:
(nitpick) Wouldn't this also work with the default settings? I wouldn't
specify any settings in tests if they aren't needed for the specific test case
to keep the tests as simple as possible.
##########
gremlin-dotnet/src/Gremlin.Net/Process/Utils.cs:
##########
@@ -32,8 +32,11 @@ namespace Gremlin.Net.Process
/// <summary>
/// Contains useful methods that can be reused across the project.
/// </summary>
- internal static class Utils
+ public static class Utils
Review Comment:
I think we should not make such a class public only to use it in tests as it
only includes methods that are intended to be used by the driver internally.
You can instead allow `Gremlin.Net.IntegrationTest` to access internals of
`Gremlin.Net` if you add it
[here](https://github.com/apache/tinkerpop/blob/7f7d3a485c7f100f98047b71672a0c2c9ab855b4/gremlin-dotnet/src/Gremlin.Net/Properties/AssemblyInfo.cs#L26)
just like we did for `Gremlin.Net.UnitTest` already. This unfortunately
requires to also sign the `Gremlin.Net.IntegrationTest` assembly which can be
done by adding [these 3
lines](https://github.com/apache/tinkerpop/blob/master/gremlin-dotnet/test/Gremlin.Net.UnitTest/Gremlin.Net.UnitTest.csproj#L6-L8)
to its csproj file.
(I can help you with this if you need any help.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
