[GitHub] [tinkerpop] FlorianHockmann commented on a diff in pull request #1923: [TINKERPOP-2838] Adds Tests for User Agent in GLVs

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@tinkerpop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org