[GitHub] [ignite-3] Pochatkin commented on a diff in pull request #1473: IGNITE-18220: Implement integration tests for completers

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

Pochatkin commented on code in PR #1473:
URL: https://github.com/apache/ignite-3/pull/1473#discussion_r1057282264


##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/CliCommandTestInitializedIntegrationBase.java:
##########
@@ -31,10 +32,14 @@ public class CliCommandTestInitializedIntegrationBase extends CliCommandTestNotI
     @Override
     void beforeAll(TestInfo testInfo) {
         startNodes(testInfo);
-        super.initializeCluster(metaStorageNodeName(testInfo));
+        initializeCluster(metaStorageNodeName(testInfo));
     }
 
     protected String metaStorageNodeName(TestInfo testInfo) {
         return testNodeName(testInfo, 0);
     }
+
+    protected List<String> allNodeNames() {
+        return CLUSTER_NODE_NAMES;
+    }

Review Comment:
   Why is it here? This static field in IgniteTestBase has protected modifier and you expand it by NON static protected method in some child class, absolutely don't understand what the reason



##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/CliCommandTestNotInitializedIntegrationBase.java:
##########
@@ -77,6 +77,7 @@ public class CliCommandTestNotInitializedIntegrationBase extends IntegrationTest
     @BeforeEach
     public void setUp(TestInfo testInfo) throws Exception {
         super.setUp(testInfo);
+

Review Comment:
   Why empty line?



##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/completer/NodeUrlProvider.java:
##########
@@ -40,7 +40,7 @@ public NodeUrlProvider(Session session, ConfigManagerProvider configManagerProvi
     }
 
     /** Resolves the url for given words. */
-    public String resolveUrl(String[] words) {
+    public String resolveUrl(String... words) {

Review Comment:
   What the point of vararg here? So, lets imagine that its required change but why you didn't change usages of this method. For example org/apache/ignite/internal/cli/core/repl/completer/hocon/ClusterConfigDynamicCompleterFactory.java:58 still use String array initialization



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