[jira] [Commented] (TINKERPOP-2838) Add UserAgent GLV Tests

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/TINKERPOP-2838?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17655947#comment-17655947 ] 

ASF GitHub Bot commented on TINKERPOP-2838:
-------------------------------------------

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





> Add UserAgent GLV Tests
> -----------------------
>
>                 Key: TINKERPOP-2838
>                 URL: https://issues.apache.org/jira/browse/TINKERPOP-2838
>             Project: TinkerPop
>          Issue Type: Improvement
>          Components: driver, server
>    Affects Versions: 3.5.4
>            Reporter: Cole Greer
>            Priority: Minor
>
> User agents were added to all GLV connection requests with [TINKERPOP-2480|https://issues.apache.org/jira/browse/TINKERPOP-2480]. The Java driver has tests added which utilize SimpleSocketServer to capture and echo back the user agent.
> [TINKERPOP-2819|https://issues.apache.org/jira/browse/TINKERPOP-2819] will bring SimpleSocketServer to all GLV's. Once this is complete additional tests mirroring the Java driver tests should be added to all GLV's to ensure the user agent is being sent and received correctly, and that the enableUserAgentOnConnect configuration is working correctly.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)