[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #1404: IGNITE-18097 CLI should check if it's already connected before trying to connect

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

PakhomovAlexander commented on code in PR #1404:
URL: https://github.com/apache/ignite-3/pull/1404#discussion_r1038163388


##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/questions/ItConnectToClusterTest.java:
##########
@@ -132,6 +138,33 @@ void connectOnStartAndSave() throws IOException {
                 .isEqualTo("http://localhost:10300");
     }
 
+    @Test
+    @DisplayName("Should ask to connect to different URL")
+    void connectToAnotherUrl() throws IOException {
+        // Given prompt before connect
+        String promptBefore = Ansi.OFF.string(promptProvider.getPrompt());
+        assertThat(promptBefore).isEqualTo("[disconnected]> ");
+
+        // And connected
+        execute("connect");
+
+        // And answer is "y"
+        bindAnswers("y");
+
+        // When connect to different URL
+        execute("connect", "http://localhost:10301");
+
+        // Then
+        assertAll(
+                this::assertErrOutputIsEmpty,
+                () -> assertOutputIs("Connected to http://localhost:10300" + System.lineSeparator()
+                + "Connected to http://localhost:10301" + System.lineSeparator())

Review Comment:
   That looks weird... Say I am connected to http://localhost:10300 and want to switch to another node and type `connect http://localhost:10301` and see 
   
   ```
   Connected to http://localhost:10300
   Connected to http://localhost:10301
   ```
   
   It looks like a bug for the user. I suggest dropping the first line.



##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/connect/ItConnectCommandTest.java:
##########
@@ -107,6 +107,29 @@ void disconnect() {
         assertThat(promptAfter).isEqualTo("[disconnected]> ");
     }
 
+    @Test
+    @DisplayName("Should state that already connected")
+    void connectTwice() {
+        // Given connected to cluster
+        execute("connect");
+        // And prompt is
+        String promptBefore = Ansi.OFF.string(promptProvider.getPrompt());
+        assertThat(promptBefore).isEqualTo("[" + nodeName() + "]> ");
+
+        // When connect again
+        execute("connect");
+
+        // Then
+        assertAll(
+                this::assertErrOutputIsEmpty,
+                () -> assertOutputIs("Connected to http://localhost:10300" + System.lineSeparator()
+                        + "You are already connected to http://localhost:10300" + System.lineSeparator())

Review Comment:
   I think it is enough just to say `You are already connected to http://localhost:10300` without the first line.



-- 
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: notifications-unsubscribe@ignite.apache.org

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