epugh commented on code in PR #3258: URL: https://github.com/apache/solr/pull/3258#discussion_r1991163882
########## solr/core/src/test/org/apache/solr/cli/CLITestHelper.java: ########## @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.cli; + +import static org.apache.solr.cli.SolrCLI.findTool; +import static org.apache.solr.cli.SolrCLI.parseCmdLine; +import static org.junit.Assert.assertEquals; + +import java.io.PrintWriter; +import java.io.Reader; +import java.io.StringReader; +import java.io.StringWriter; +import org.apache.commons.cli.CommandLine; + +public class CLITestHelper { + + /** + * Run a tool with all parameters, as specified in the command line. Fist parameter must be the Review Comment: ```suggestion * Run a tool with all parameters, as specified in the command line. First parameter must be the ``` ########## solr/core/src/test/org/apache/solr/cli/CLITestHelper.java: ########## @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.cli; + +import static org.apache.solr.cli.SolrCLI.findTool; +import static org.apache.solr.cli.SolrCLI.parseCmdLine; +import static org.junit.Assert.assertEquals; + +import java.io.PrintWriter; +import java.io.Reader; +import java.io.StringReader; +import java.io.StringWriter; +import org.apache.commons.cli.CommandLine; + +public class CLITestHelper { + + /** + * Run a tool with all parameters, as specified in the command line. Fist parameter must be the + * tool name. + * + * @param args Complete command line. + * @param clazz Expected class name of tool implementation. + */ + public static int runTool(String[] args, Class<?> clazz) throws Exception { + ToolRuntime runtime = new ValidatingRuntime(); + return runTool(args, runtime, clazz); + } + + /** + * Run a tool with all parameters, as specified in the command line. Fist parameter must be the + * tool name. + * + * @param args Complete command line. + * @param runtime The runtime implementation specified to the tool. In test, should be usually a + * test specific implementation for additional checks. Review Comment: would it make sense to have a @link for the next person to the specific class? ########## solr/core/src/test/org/apache/solr/cli/PackageToolTest.java: ########## @@ -94,7 +94,8 @@ private <T extends SolrRequest<? extends SolrResponse>> T withBasicAuth(T req) { @Test public void testPackageTool() throws Exception { - PackageTool tool = new PackageTool(); + ToolRuntime runtime = new CLITestHelper.ValidatingRuntime(); Review Comment: why not also the `BufferingRuntime` and use that to check the outputs? ########## solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java: ########## @@ -370,13 +362,12 @@ protected void testExample(String exampleName) throws Exception { }; // capture tool output to stdout - ByteArrayOutputStream baos = new ByteArrayOutputStream(); Review Comment: Not having these `baos` is nice... ########## solr/core/src/test/org/apache/solr/cli/StatusToolTest.java: ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.cli; + +import java.util.Map; +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.util.Utils; +import org.apache.solr.embedded.JettySolrRunner; +import org.junit.BeforeClass; +import org.junit.Test; + +public class StatusToolTest extends SolrCloudTestCase { + + @BeforeClass + public static void setupCluster() throws Exception { + configureCluster(2).configure(); + } + + /** Check the tool returns expected details by specifying Solr URL on the command line. */ + @Test + public void testSolrUrlStatus() throws Exception { Review Comment: and here is the answer to my previous comment, you did preserve the positive unit test! ########## solr/core/src/test/org/apache/solr/cli/CLITestHelper.java: ########## @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.cli; + +import static org.apache.solr.cli.SolrCLI.findTool; +import static org.apache.solr.cli.SolrCLI.parseCmdLine; +import static org.junit.Assert.assertEquals; + +import java.io.PrintWriter; +import java.io.Reader; +import java.io.StringReader; +import java.io.StringWriter; +import org.apache.commons.cli.CommandLine; + +public class CLITestHelper { + + /** + * Run a tool with all parameters, as specified in the command line. Fist parameter must be the + * tool name. + * + * @param args Complete command line. + * @param clazz Expected class name of tool implementation. + */ + public static int runTool(String[] args, Class<?> clazz) throws Exception { + ToolRuntime runtime = new ValidatingRuntime(); + return runTool(args, runtime, clazz); + } + + /** + * Run a tool with all parameters, as specified in the command line. Fist parameter must be the + * tool name. + * + * @param args Complete command line. + * @param runtime The runtime implementation specified to the tool. In test, should be usually a + * test specific implementation for additional checks. + * @param clazz Expected class name of tool implementation. + */ + public static int runTool(String[] args, ToolRuntime runtime, Class<?> clazz) throws Exception { + Tool tool = findTool(args, runtime); + + // Check the tool actual class + assertEquals(clazz, tool.getClass()); + + CommandLine cli = parseCmdLine(tool, args); + return tool.runTool(cli); + } + + public static class BufferingRuntime extends ValidatingRuntime { Review Comment: is a BufferingRuntime on top of a ValidatingRuntitme needed? `UnitTestingRuntime`? `TestingRuntime`? ########## solr/core/src/test/org/apache/solr/security/BasicAuthIntegrationTest.java: ########## @@ -298,23 +295,10 @@ public void testBasicAuth() throws Exception { verifySecurityStatus(cl, baseUrl + "/admin/info/key", "key", NOT_NULL_PREDICATE, 20); assertAuthMetricsMinimums(17, 8, 8, 1, 0, 0); - String[] toolArgs = - new String[] {"status", "--solr-url", baseUrl, "--credentials", "harry:HarryIsUberCool"}; - ByteArrayOutputStream baos = new ByteArrayOutputStream(); - PrintStream stdoutSim = new PrintStream(baos, true, StandardCharsets.UTF_8.name()); - StatusTool tool = new StatusTool(stdoutSim); - try { - tool.runTool(SolrCLI.processCommandLineArgs(tool, toolArgs)); - Map<?, ?> obj = (Map<?, ?>) Utils.fromJSON(new ByteArrayInputStream(baos.toByteArray())); - assertTrue(obj.containsKey("version")); - assertTrue(obj.containsKey("startTime")); - assertTrue(obj.containsKey("uptime")); - assertTrue(obj.containsKey("memory")); - } catch (Exception e) { - log.error( - "StatusTool failed due to: {}; stdout from tool prior to failure: {}", - e, - baos.toString(StandardCharsets.UTF_8.name())); // nowarn + String[] toolArgs = new String[] {"status", "--solr-url", baseUrl}; Review Comment: Do we need both the positive "the tool suceeded" test and the "the tool failed" test? I like simplifying our tests, so if the old test with all it's checks isn't needed, great.. ########## solr/core/src/test/org/apache/solr/cli/SolrCLIZkToolsTest.java: ########## Review Comment: i think i like better passing the command in as one of the args.. that had caught me before while reading the code... ########## solr/core/src/test/org/apache/solr/cli/CLITestHelper.java: ########## @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.cli; + +import static org.apache.solr.cli.SolrCLI.findTool; +import static org.apache.solr.cli.SolrCLI.parseCmdLine; +import static org.junit.Assert.assertEquals; + +import java.io.PrintWriter; +import java.io.Reader; +import java.io.StringReader; +import java.io.StringWriter; +import org.apache.commons.cli.CommandLine; + +public class CLITestHelper { + + /** + * Run a tool with all parameters, as specified in the command line. Fist parameter must be the + * tool name. + * + * @param args Complete command line. + * @param clazz Expected class name of tool implementation. + */ + public static int runTool(String[] args, Class<?> clazz) throws Exception { + ToolRuntime runtime = new ValidatingRuntime(); + return runTool(args, runtime, clazz); + } + + /** + * Run a tool with all parameters, as specified in the command line. Fist parameter must be the Review Comment: ```suggestion * Run a tool with all parameters, as specified in the command line. First parameter must be the ``` ########## solr/core/src/test/org/apache/solr/cli/CLITestHelper.java: ########## @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.cli; + +import static org.apache.solr.cli.SolrCLI.findTool; +import static org.apache.solr.cli.SolrCLI.parseCmdLine; +import static org.junit.Assert.assertEquals; + +import java.io.PrintWriter; +import java.io.Reader; +import java.io.StringReader; +import java.io.StringWriter; +import org.apache.commons.cli.CommandLine; + +public class CLITestHelper { + + /** + * Run a tool with all parameters, as specified in the command line. Fist parameter must be the + * tool name. + * + * @param args Complete command line. + * @param clazz Expected class name of tool implementation. + */ + public static int runTool(String[] args, Class<?> clazz) throws Exception { + ToolRuntime runtime = new ValidatingRuntime(); + return runTool(args, runtime, clazz); + } + + /** + * Run a tool with all parameters, as specified in the command line. Fist parameter must be the + * tool name. + * + * @param args Complete command line. + * @param runtime The runtime implementation specified to the tool. In test, should be usually a + * test specific implementation for additional checks. + * @param clazz Expected class name of tool implementation. + */ + public static int runTool(String[] args, ToolRuntime runtime, Class<?> clazz) throws Exception { + Tool tool = findTool(args, runtime); + + // Check the tool actual class + assertEquals(clazz, tool.getClass()); + + CommandLine cli = parseCmdLine(tool, args); + return tool.runTool(cli); + } + + public static class BufferingRuntime extends ValidatingRuntime { Review Comment: Later on, I see how you reference is as part of `CLITestHelper.BufferingRuntime` and that makes it clear what it's for! ########## solr/core/src/test/org/apache/solr/cli/PackageToolTest.java: ########## @@ -94,7 +94,8 @@ private <T extends SolrRequest<? extends SolrResponse>> T withBasicAuth(T req) { @Test public void testPackageTool() throws Exception { - PackageTool tool = new PackageTool(); + ToolRuntime runtime = new CLITestHelper.ValidatingRuntime(); Review Comment: the way we have it, could just define and pass it into the constructor for PackageTool right? ########## solr/core/src/test/org/apache/solr/cli/CreateToolTest.java: ########## @@ -54,13 +50,6 @@ public void testCreateCollectionWithBasicAuth() throws Exception { "--verbose" }; - assertEquals(0, runTool(args)); - } - - private int runTool(String[] args) throws Exception { - Tool tool = findTool(args); - assertTrue(tool instanceof CreateTool); - CommandLine cli = parseCmdLine(tool, args); - return tool.runTool(cli); + assertEquals(0, CLITestHelper.runTool(args, CreateTool.class)); Review Comment: nicer! we had a lot of boiler plate -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
