You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by PurelyApplied <gi...@git.apache.org> on 2017/07/24 20:51:17 UTC
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
GitHub user PurelyApplied opened a pull request:
https://github.com/apache/geode/pull/652
Geode-2971: Introduce ExitCode to resolve inconsistency of shell exit values
There are a few sections containing `// TODO PSRhomberg`. Most are there to mark a specific request for feedback, and two are there to explain a change that counterintuitively respects existing behavior.
- [ ] Have you removed your `TODO` comments?
- [ ] Is your initial contribution a single, squashed commit?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/PurelyApplied/geode geode-2971
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode/pull/652.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #652
----
commit f8b1685dcecd023bbaa17fec8024dc32af648f33
Author: Patrick Rhomberg <pr...@pivotal.io>
Date: 2017-07-20T19:36:36Z
Added ExitCode to handle all System.exit calls.
* Significant refactoring of Launcher.java and its use of exit states.
commit 60a1bcd93fe3c4e67d0735b3fce6c9d3bed0d9f6
Author: Patrick Rhomberg <pr...@pivotal.io>
Date: 2017-07-20T19:40:34Z
All System.exit calls replaced by an appropriate call of ExitCode.[code].doSystemExit();
commit 57df2484d9a1fc6001dc5ef12522a24483f126f6
Author: Patrick Rhomberg <pr...@pivotal.io>
Date: 2017-07-20T22:54:06Z
Added AcceptanceTest for gfsh member status commands
commit c82b0115099b98bbbfca1eb6e3539a82e6fe054b
Author: Patrick Rhomberg <pr...@pivotal.io>
Date: 2017-07-20T19:42:18Z
Correction of inconsistency in exit status of member status commands.
commit 92da642dcf1b5039def0ecf3d0f317c652a48515
Author: Patrick Rhomberg <pr...@pivotal.io>
Date: 2017-07-21T22:05:48Z
Removed dead code and many needless asides from touched file SystemFailure.java
commit 830d3ee0af16c765b99271e231377439cd659b26
Author: Patrick Rhomberg <pr...@pivotal.io>
Date: 2017-07-21T22:15:27Z
Removed redundant declarations and much dead code from touched file StatArchiveReader.java
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129712729
--- Diff: geode-assembly/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExitCodeStatusCommandsDUnitTest.java ---
@@ -0,0 +1,396 @@
+/*
+ * 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.geode.management.internal.cli.shell;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.internal.AvailablePort;
+import org.apache.geode.internal.ExitCode;
+import org.apache.geode.internal.process.PidFile;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.test.dunit.rules.gfsh.GfshExecution;
+import org.apache.geode.test.dunit.rules.gfsh.GfshRule;
+import org.apache.geode.test.dunit.rules.gfsh.GfshScript;
+import org.apache.geode.test.junit.categories.AcceptanceTest;
+
+// Originally created in response to GEODE-2971
+
+@Category(AcceptanceTest.class)
+@RunWith(JUnitParamsRunner.class)
+public class GfshExitCodeStatusCommandsDUnitTest {
--- End diff --
I recommend removing "DUnit" from the name especially since it's not categorized as DistributedTest.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129713502
--- Diff: geode-assembly/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExitCodeStatusCommandsDUnitTest.java ---
@@ -0,0 +1,396 @@
+/*
+ * 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.geode.management.internal.cli.shell;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.internal.AvailablePort;
+import org.apache.geode.internal.ExitCode;
+import org.apache.geode.internal.process.PidFile;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.test.dunit.rules.gfsh.GfshExecution;
+import org.apache.geode.test.dunit.rules.gfsh.GfshRule;
+import org.apache.geode.test.dunit.rules.gfsh.GfshScript;
+import org.apache.geode.test.junit.categories.AcceptanceTest;
+
+// Originally created in response to GEODE-2971
+
+@Category(AcceptanceTest.class)
+@RunWith(JUnitParamsRunner.class)
+public class GfshExitCodeStatusCommandsDUnitTest {
+ private static File toolsJar;
+ private final static ThreePhraseGenerator nameGenerator = new ThreePhraseGenerator();
+ private final static String memberControllerName = "member-controller";
+
+ @Rule
+ public GfshRule gfsh = new GfshRule();
+ private String locatorName;
+ private String serverName;
+
+ private int locatorPort;
+
+ // Some test configuration shorthands
+ private TestConfiguration LOCATOR_ONLINE_BUT_NOT_CONNECTED =
+ new TestConfiguration(true, false, false);
+ private TestConfiguration LOCATOR_ONLINE_AND_CONNECTED = new TestConfiguration(true, false, true);
+ private TestConfiguration BOTH_ONLINE_BUT_NOT_CONNECTED =
+ new TestConfiguration(true, true, false);
+ private TestConfiguration BOTH_ONLINE_AND_CONNECTED = new TestConfiguration(true, true, true);
+
+ @BeforeClass
+ public static void classSetup() {
+ File javaHome = new File(System.getProperty("java.home"));
+ String toolsPath =
+ javaHome.getName().equalsIgnoreCase("jre") ? "../lib/tools.jar" : "lib/tools.jar";
+ toolsJar = new File(javaHome, toolsPath);
+ }
+
+ @Before
+ public void setup() {
+ locatorName = "locator-" + nameGenerator.generate('-');
+ serverName = "server-" + nameGenerator.generate('-');
+ locatorPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
+ }
+
+ @Test
+ @Parameters(
+ value = {"status locator --port=-10", "status locator --pid=-1", "status server --pid=-1"})
+ public void statusCommandWithInvalidOptionValueShouldFail(String cmd) {
+ GfshScript.of(cmd).withName("test-frame").awaitAtMost(1, MINUTES)
+ .expectExitCode(ExitCode.FATAL.getValue()).execute(gfsh);
+ }
+
+
+ @Test
+ @Parameters(value = {"status locator --host=somehostname", "status locator --port=10334",
+ "status locator --dir=.", "status server --dir=.", "status locator --name=some-locator-name",
+ "status server --name=some-server-name", "status locator --pid=-1", "status server --pid=-1"})
+ public void statusCommandWithValidOptionValueShouldFailWithNoMembers(String cmd) {
+ GfshScript.of(cmd).withName("test-frame").awaitAtMost(1, MINUTES)
+ .expectExitCode(ExitCode.FATAL.getValue()).execute(gfsh);
+ }
+
+
+ @Test
+ public void onlineStatusCommandShouldFailWhenNotConnected_locator_name() {
+ TestConfiguration config = LOCATOR_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByName();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.FATAL);
+ }
+
+
+ @Test
+ public void onlineStatusCommandShouldFailWhenNotConnected_server_name() {
+ TestConfiguration config = BOTH_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByName();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.FATAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenNotConnected_locator_port() {
+ // --host defaults to localhost, so `status locator --port=xxx` should still succeed.
+ TestConfiguration config = LOCATOR_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByPort();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenNotConnected_locator_host_and_port() {
+ // Since this is still local to the testing VM's machine, `status locator --host=localhost
+ // --port=xxx` should succeed
+ TestConfiguration config = LOCATOR_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByHostAndPort();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+
+
+ @Test
+ public void onlineStatusCommandShouldSucceedWhenConnected_locator_name() {
+ TestConfiguration config = LOCATOR_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByName();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+
+ @Test
+ public void onlineStatusCommandShouldSucceedWhenConnected_server_name() {
+ TestConfiguration config = BOTH_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByName();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void onlineStatusCommandShouldSucceedWhenConnected_locator_port() {
+ TestConfiguration config = LOCATOR_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByPort();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void onlineStatusCommandShouldSucceedWhenConnected_locator_host_and_port() {
+ TestConfiguration config = LOCATOR_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByHostAndPort();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenConnected_locator_dir() {
+ TestConfiguration config = LOCATOR_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByDir();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenConnected_server_dir() {
+ TestConfiguration config = BOTH_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByDir();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenConnected_locator_pid() throws IOException {
+ Assume.assumeTrue(toolsJar.exists());
+ TestConfiguration config = LOCATOR_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByPid();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenConnected_server_pid() throws IOException {
+ Assume.assumeTrue(toolsJar.exists());
+ TestConfiguration config = BOTH_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByPid();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+
+
+ @Test
+ public void offlineStatusCommandShouldSucceedEvenWhenNotConnected_locator_dir() {
+ TestConfiguration config = LOCATOR_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByDir();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedEvenWhenNotConnected_server_dir() {
+ TestConfiguration config = BOTH_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByDir();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedEvenWhenNotConnected_locator_pid()
+ throws IOException {
+ Assume.assumeTrue(toolsJar.exists());
+ TestConfiguration config = LOCATOR_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByPid();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedEvenWhenNotConnected_server_pid()
+ throws IOException {
+ Assume.assumeTrue(toolsJar.exists());
+ TestConfiguration config = BOTH_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByPid();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+
+
+ // TODO PSRhomberg -- Input requested: Would these read better inlined into the respective tests?
+ private String startLocatorCommand() {
+ return new CommandStringBuilder("start locator").addOption("name", locatorName)
+ .addOption("port", String.valueOf(locatorPort)).toString();
+ }
+
+
+ private String startServerCommand() {
+ return new CommandStringBuilder("start server").addOption("name", serverName).toString();
+ }
+
+
+ private String connectCommand() {
+ return new CommandStringBuilder("connect")
+ .addOption("locator", String.format("localhost[%d]", locatorPort)).toString();
+ }
+
+
+ private String statusServerCommandByName() {
+ return new CommandStringBuilder("status server").addOption("name", serverName).toString();
+ }
+
+ private String statusServerCommandByDir() {
+ String serverDir = gfsh.getTemporaryFolder().getRoot().toPath().resolve(memberControllerName)
+ .resolve(serverName).toAbsolutePath().toString();
+ return new CommandStringBuilder("status server").addOption("dir", serverDir).toString();
+ }
+
+
+ private String statusServerCommandByPid() throws IOException {
+ int serverPid = snoopMemberFile(serverName, "server.pid");
+ return new CommandStringBuilder("status server").addOption("pid", String.valueOf(serverPid))
+ .toString();
+ }
+
+ private String statusLocatorCommandByName() {
+ return new CommandStringBuilder("status locator").addOption("name", locatorName).toString();
+ }
+
+ private String statusLocatorCommandByPort() {
+ return new CommandStringBuilder("status locator").addOption("port", String.valueOf(locatorPort))
+ .toString();
+ }
+
+ private String statusLocatorCommandByHostAndPort() {
+ return new CommandStringBuilder("status locator").addOption("host", "localhost")
+ .addOption("port", String.valueOf(locatorPort)).toString();
+ }
+
+ private String statusLocatorCommandByDir() {
+ String locatorDir = gfsh.getTemporaryFolder().getRoot().toPath().resolve(memberControllerName)
+ .resolve(locatorName).toAbsolutePath().toString();
+ return new CommandStringBuilder("status locator").addOption("dir", locatorDir).toString();
+ }
+
+
+ private String statusLocatorCommandByPid() throws IOException {
+ int locatorPid = snoopMemberFile(locatorName, "locator.pid");
+ return new CommandStringBuilder("status locator").addOption("pid", String.valueOf(locatorPid))
+ .toString();
+ }
+
+ private int snoopMemberFile(String memberName, String pidFileEndsWith) throws IOException {
+ File directory = gfsh.getTemporaryFolder().getRoot().toPath().resolve(memberControllerName)
+ .resolve(memberName).toFile();
+ File[] files = directory.listFiles();
+ if (files == null) {
+ throw new RuntimeException(String.format(
+ "Expected directory ('%s') for member '%s' either does not denote a directory, or an I/O error occurred.",
+ directory.toString(), memberName));
+ }
+ File pidFile = Arrays.stream(files).filter(file -> file.getName().endsWith(pidFileEndsWith))
+ .findFirst().orElseThrow(() -> new RuntimeException(String
+ .format("Expected member '%s' to have pid file but could not find it.", memberName)));
+ return new PidFile(pidFile).readPid();
+ }
+
+ private void executeScriptWithExpectedExitCode(String statusCommand, TestConfiguration config,
+ ExitCode expectedExit) {
+ String[] gfshScriptCommands = config.addConnectCommandIfNecessary(statusCommand);
+ GfshScript gfshScript = GfshScript.of(gfshScriptCommands).withName("test-frame")
+ .awaitAtMost(1, MINUTES).expectExitCode(expectedExit.getValue());
+ if (toolsJar.exists()) {
+ gfshScript.addToClasspath(toolsJar.getAbsolutePath());
+ }
+ gfshScript.execute(gfsh);
+ }
+
+
+ public class TestConfiguration {
+ TestConfiguration(boolean locatorStarted, boolean serverStarted, boolean connectedToLocator) {
+ this.locatorStarted = locatorStarted;
+ this.serverStarted = serverStarted;
+ this.connectedToLocator = connectedToLocator;
+ }
+
+ private boolean locatorStarted;
+ private boolean serverStarted;
+ private boolean connectedToLocator;
+
+ boolean isConnectedToLocator() {
+ return connectedToLocator;
+ }
+
+ void startNecessaryMembers() {
+ if (!locatorStarted && !serverStarted) {
+ return;
+ }
+
+ ArrayList<String> commands = new ArrayList<>();
--- End diff --
This is more correct:
```java
List<String> commands = new ArrayList<>();
```
There are bad examples throughout Geode that declare vars as ArrayList.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/geode/pull/652
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Posted by PurelyApplied <gi...@git.apache.org>.
Github user PurelyApplied commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129656012
--- Diff: geode-core/src/main/java/org/apache/geode/internal/ExitCode.java ---
@@ -0,0 +1,61 @@
+/*
+ * 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.geode.internal;
+
+import java.util.Arrays;
+
+import org.springframework.shell.core.ExitShellRequest;
+
+
+public enum ExitCode {
+
+ // The choice of redundant values here is to be consistent with existing behavior
+ // while allowing for future extensibility. JVM_TERMINATED_EXIT(99) exists for coverage of
+ // Spring's ExitShellRequest values in fromSpring.
+ NORMAL(0),
+ FATAL(1),
+ DEPENDENCY_GRAPH_FAILURE(-1),
+ COULD_NOT_EXECUTE_COMMAND(1),
+ INVALID_COMMAND(1),
--- End diff --
I agree in principle, but was hesitant to alter existing behavior for system exit calls. I didn't gain any traction in my message to the dev list about using any exit codes other than `0` and `1`, but at the same time felt that some distinction was warranted in the different places that `System.exit` had been called.
Then again, I just finished ripping out dead code that had been in place "for future development," so perhaps I should keep it simple for now and not do the work of future extensibility that may or may not ever arrive.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #652: Geode-2971: Introduce ExitCode to resolve inconsistency of...
Posted by PurelyApplied <gi...@git.apache.org>.
Github user PurelyApplied commented on the issue:
https://github.com/apache/geode/pull/652
Precheckin running.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129713345
--- Diff: geode-assembly/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExitCodeStatusCommandsDUnitTest.java ---
@@ -0,0 +1,396 @@
+/*
+ * 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.geode.management.internal.cli.shell;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.internal.AvailablePort;
+import org.apache.geode.internal.ExitCode;
+import org.apache.geode.internal.process.PidFile;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.test.dunit.rules.gfsh.GfshExecution;
+import org.apache.geode.test.dunit.rules.gfsh.GfshRule;
+import org.apache.geode.test.dunit.rules.gfsh.GfshScript;
+import org.apache.geode.test.junit.categories.AcceptanceTest;
+
+// Originally created in response to GEODE-2971
+
+@Category(AcceptanceTest.class)
+@RunWith(JUnitParamsRunner.class)
+public class GfshExitCodeStatusCommandsDUnitTest {
+ private static File toolsJar;
+ private final static ThreePhraseGenerator nameGenerator = new ThreePhraseGenerator();
+ private final static String memberControllerName = "member-controller";
+
+ @Rule
+ public GfshRule gfsh = new GfshRule();
+ private String locatorName;
+ private String serverName;
+
+ private int locatorPort;
+
+ // Some test configuration shorthands
+ private TestConfiguration LOCATOR_ONLINE_BUT_NOT_CONNECTED =
+ new TestConfiguration(true, false, false);
+ private TestConfiguration LOCATOR_ONLINE_AND_CONNECTED = new TestConfiguration(true, false, true);
+ private TestConfiguration BOTH_ONLINE_BUT_NOT_CONNECTED =
+ new TestConfiguration(true, true, false);
+ private TestConfiguration BOTH_ONLINE_AND_CONNECTED = new TestConfiguration(true, true, true);
+
+ @BeforeClass
+ public static void classSetup() {
+ File javaHome = new File(System.getProperty("java.home"));
+ String toolsPath =
+ javaHome.getName().equalsIgnoreCase("jre") ? "../lib/tools.jar" : "lib/tools.jar";
+ toolsJar = new File(javaHome, toolsPath);
+ }
+
+ @Before
+ public void setup() {
+ locatorName = "locator-" + nameGenerator.generate('-');
+ serverName = "server-" + nameGenerator.generate('-');
+ locatorPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
+ }
+
+ @Test
+ @Parameters(
+ value = {"status locator --port=-10", "status locator --pid=-1", "status server --pid=-1"})
+ public void statusCommandWithInvalidOptionValueShouldFail(String cmd) {
+ GfshScript.of(cmd).withName("test-frame").awaitAtMost(1, MINUTES)
+ .expectExitCode(ExitCode.FATAL.getValue()).execute(gfsh);
+ }
+
+
+ @Test
+ @Parameters(value = {"status locator --host=somehostname", "status locator --port=10334",
+ "status locator --dir=.", "status server --dir=.", "status locator --name=some-locator-name",
+ "status server --name=some-server-name", "status locator --pid=-1", "status server --pid=-1"})
+ public void statusCommandWithValidOptionValueShouldFailWithNoMembers(String cmd) {
+ GfshScript.of(cmd).withName("test-frame").awaitAtMost(1, MINUTES)
+ .expectExitCode(ExitCode.FATAL.getValue()).execute(gfsh);
+ }
+
+
+ @Test
+ public void onlineStatusCommandShouldFailWhenNotConnected_locator_name() {
+ TestConfiguration config = LOCATOR_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByName();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.FATAL);
+ }
+
+
+ @Test
+ public void onlineStatusCommandShouldFailWhenNotConnected_server_name() {
+ TestConfiguration config = BOTH_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByName();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.FATAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenNotConnected_locator_port() {
+ // --host defaults to localhost, so `status locator --port=xxx` should still succeed.
+ TestConfiguration config = LOCATOR_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByPort();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenNotConnected_locator_host_and_port() {
+ // Since this is still local to the testing VM's machine, `status locator --host=localhost
+ // --port=xxx` should succeed
+ TestConfiguration config = LOCATOR_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByHostAndPort();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+
+
+ @Test
+ public void onlineStatusCommandShouldSucceedWhenConnected_locator_name() {
+ TestConfiguration config = LOCATOR_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByName();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+
+ @Test
+ public void onlineStatusCommandShouldSucceedWhenConnected_server_name() {
+ TestConfiguration config = BOTH_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByName();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void onlineStatusCommandShouldSucceedWhenConnected_locator_port() {
+ TestConfiguration config = LOCATOR_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByPort();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void onlineStatusCommandShouldSucceedWhenConnected_locator_host_and_port() {
+ TestConfiguration config = LOCATOR_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByHostAndPort();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenConnected_locator_dir() {
+ TestConfiguration config = LOCATOR_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByDir();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenConnected_server_dir() {
+ TestConfiguration config = BOTH_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByDir();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenConnected_locator_pid() throws IOException {
+ Assume.assumeTrue(toolsJar.exists());
+ TestConfiguration config = LOCATOR_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByPid();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedWhenConnected_server_pid() throws IOException {
+ Assume.assumeTrue(toolsJar.exists());
+ TestConfiguration config = BOTH_ONLINE_AND_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByPid();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+
+
+ @Test
+ public void offlineStatusCommandShouldSucceedEvenWhenNotConnected_locator_dir() {
+ TestConfiguration config = LOCATOR_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByDir();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedEvenWhenNotConnected_server_dir() {
+ TestConfiguration config = BOTH_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByDir();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedEvenWhenNotConnected_locator_pid()
+ throws IOException {
+ Assume.assumeTrue(toolsJar.exists());
+ TestConfiguration config = LOCATOR_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusLocatorCommandByPid();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+ @Test
+ public void offlineStatusCommandShouldSucceedEvenWhenNotConnected_server_pid()
+ throws IOException {
+ Assume.assumeTrue(toolsJar.exists());
+ TestConfiguration config = BOTH_ONLINE_BUT_NOT_CONNECTED;
+ config.startNecessaryMembers();
+
+ String statusCommand = statusServerCommandByPid();
+ executeScriptWithExpectedExitCode(statusCommand, config, ExitCode.NORMAL);
+ }
+
+
+
+ // TODO PSRhomberg -- Input requested: Would these read better inlined into the respective tests?
+ private String startLocatorCommand() {
+ return new CommandStringBuilder("start locator").addOption("name", locatorName)
+ .addOption("port", String.valueOf(locatorPort)).toString();
+ }
+
+
+ private String startServerCommand() {
+ return new CommandStringBuilder("start server").addOption("name", serverName).toString();
+ }
+
+
+ private String connectCommand() {
+ return new CommandStringBuilder("connect")
+ .addOption("locator", String.format("localhost[%d]", locatorPort)).toString();
+ }
+
+
+ private String statusServerCommandByName() {
+ return new CommandStringBuilder("status server").addOption("name", serverName).toString();
+ }
+
+ private String statusServerCommandByDir() {
+ String serverDir = gfsh.getTemporaryFolder().getRoot().toPath().resolve(memberControllerName)
+ .resolve(serverName).toAbsolutePath().toString();
+ return new CommandStringBuilder("status server").addOption("dir", serverDir).toString();
+ }
+
+
+ private String statusServerCommandByPid() throws IOException {
+ int serverPid = snoopMemberFile(serverName, "server.pid");
+ return new CommandStringBuilder("status server").addOption("pid", String.valueOf(serverPid))
+ .toString();
+ }
+
+ private String statusLocatorCommandByName() {
+ return new CommandStringBuilder("status locator").addOption("name", locatorName).toString();
+ }
+
+ private String statusLocatorCommandByPort() {
+ return new CommandStringBuilder("status locator").addOption("port", String.valueOf(locatorPort))
+ .toString();
+ }
+
+ private String statusLocatorCommandByHostAndPort() {
+ return new CommandStringBuilder("status locator").addOption("host", "localhost")
+ .addOption("port", String.valueOf(locatorPort)).toString();
+ }
+
+ private String statusLocatorCommandByDir() {
+ String locatorDir = gfsh.getTemporaryFolder().getRoot().toPath().resolve(memberControllerName)
+ .resolve(locatorName).toAbsolutePath().toString();
+ return new CommandStringBuilder("status locator").addOption("dir", locatorDir).toString();
+ }
+
+
+ private String statusLocatorCommandByPid() throws IOException {
+ int locatorPid = snoopMemberFile(locatorName, "locator.pid");
+ return new CommandStringBuilder("status locator").addOption("pid", String.valueOf(locatorPid))
+ .toString();
+ }
+
+ private int snoopMemberFile(String memberName, String pidFileEndsWith) throws IOException {
+ File directory = gfsh.getTemporaryFolder().getRoot().toPath().resolve(memberControllerName)
+ .resolve(memberName).toFile();
+ File[] files = directory.listFiles();
+ if (files == null) {
+ throw new RuntimeException(String.format(
+ "Expected directory ('%s') for member '%s' either does not denote a directory, or an I/O error occurred.",
+ directory.toString(), memberName));
+ }
+ File pidFile = Arrays.stream(files).filter(file -> file.getName().endsWith(pidFileEndsWith))
+ .findFirst().orElseThrow(() -> new RuntimeException(String
+ .format("Expected member '%s' to have pid file but could not find it.", memberName)));
+ return new PidFile(pidFile).readPid();
+ }
+
+ private void executeScriptWithExpectedExitCode(String statusCommand, TestConfiguration config,
+ ExitCode expectedExit) {
+ String[] gfshScriptCommands = config.addConnectCommandIfNecessary(statusCommand);
+ GfshScript gfshScript = GfshScript.of(gfshScriptCommands).withName("test-frame")
+ .awaitAtMost(1, MINUTES).expectExitCode(expectedExit.getValue());
+ if (toolsJar.exists()) {
+ gfshScript.addToClasspath(toolsJar.getAbsolutePath());
+ }
+ gfshScript.execute(gfsh);
+ }
+
+
+ public class TestConfiguration {
--- End diff --
Recommend making this static unless the inner-class actually needs an instance to the outer class:
```java
public static class TestConfiguration
```
Non-static inner-classes have an implicit reference to the outer class instance.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129712972
--- Diff: geode-assembly/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExitCodeStatusCommandsDUnitTest.java ---
@@ -0,0 +1,396 @@
+/*
+ * 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.geode.management.internal.cli.shell;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.internal.AvailablePort;
+import org.apache.geode.internal.ExitCode;
+import org.apache.geode.internal.process.PidFile;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.test.dunit.rules.gfsh.GfshExecution;
+import org.apache.geode.test.dunit.rules.gfsh.GfshRule;
+import org.apache.geode.test.dunit.rules.gfsh.GfshScript;
+import org.apache.geode.test.junit.categories.AcceptanceTest;
+
+// Originally created in response to GEODE-2971
+
+@Category(AcceptanceTest.class)
+@RunWith(JUnitParamsRunner.class)
+public class GfshExitCodeStatusCommandsDUnitTest {
+ private static File toolsJar;
+ private final static ThreePhraseGenerator nameGenerator = new ThreePhraseGenerator();
+ private final static String memberControllerName = "member-controller";
+
+ @Rule
+ public GfshRule gfsh = new GfshRule();
+ private String locatorName;
+ private String serverName;
+
+ private int locatorPort;
+
+ // Some test configuration shorthands
+ private TestConfiguration LOCATOR_ONLINE_BUT_NOT_CONNECTED =
--- End diff --
These get re-instantiated between each test method. Was that the intention here? Or do you want these to be static final?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129647221
--- Diff: geode-assembly/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExitCodeStatusCommandsDUnitTest.java ---
@@ -0,0 +1,396 @@
+/*
+ * 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.geode.management.internal.cli.shell;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.internal.AvailablePort;
+import org.apache.geode.internal.ExitCode;
+import org.apache.geode.internal.process.PidFile;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.test.dunit.rules.gfsh.GfshExecution;
+import org.apache.geode.test.dunit.rules.gfsh.GfshRule;
+import org.apache.geode.test.dunit.rules.gfsh.GfshScript;
+import org.apache.geode.test.junit.categories.AcceptanceTest;
+
+// Originally created in response to GEODE-2971
+
+@Category(AcceptanceTest.class)
+@RunWith(JUnitParamsRunner.class)
+public class GfshExitCodeStatusCommandsDUnitTest {
+ private static File toolsJar;
+ private final static ThreePhraseGenerator nameGenerator = new ThreePhraseGenerator();
+ private final static String memberControllerName = "member-controller";
+
+ @Rule
+ public GfshRule gfsh = new GfshRule();
+ private String locatorName;
+ private String serverName;
+
+ private int locatorPort;
+
+ // Some test configuration shorthands
+ private TestConfiguration LOCATOR_ONLINE_BUT_NOT_CONNECTED =
+ new TestConfiguration(true, false, false);
+ private TestConfiguration LOCATOR_ONLINE_AND_CONNECTED = new TestConfiguration(true, false, true);
+ private TestConfiguration BOTH_ONLINE_BUT_NOT_CONNECTED =
+ new TestConfiguration(true, true, false);
+ private TestConfiguration BOTH_ONLINE_AND_CONNECTED = new TestConfiguration(true, true, true);
+
+ @BeforeClass
+ public static void classSetup() {
+ File javaHome = new File(System.getProperty("java.home"));
+ String toolsPath =
+ javaHome.getName().equalsIgnoreCase("jre") ? "../lib/tools.jar" : "lib/tools.jar";
+ toolsJar = new File(javaHome, toolsPath);
+ }
+
+ @Before
+ public void setup() {
+ locatorName = "locator-" + nameGenerator.generate('-');
+ serverName = "server-" + nameGenerator.generate('-');
+ locatorPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
+ }
+
+ @Test
+ @Parameters(
+ value = {"status locator --port=-10", "status locator --pid=-1", "status server --pid=-1"})
+ public void statusCommandWithInvalidOptionValueShouldFail(String cmd) {
+ GfshScript.of(cmd).withName("test-frame").awaitAtMost(1, MINUTES)
+ .expectExitCode(ExitCode.FATAL.getValue()).execute(gfsh);
+ }
+
+
+ @Test
+ @Parameters(value = {"status locator --host=somehostname", "status locator --port=10334",
+ "status locator --dir=.", "status server --dir=.", "status locator --name=some-locator-name",
+ "status server --name=some-server-name", "status locator --pid=-1", "status server --pid=-1"})
+ public void statusCommandWithValidOptionValueShouldFailWithNoMembers(String cmd) {
--- End diff --
this test failed for me when I run it locally:
[INFO 10:33:30.713 PDT] (test-frame): (1) Executing - status server --dir=.
[INFO 10:33:30.713 PDT] (test-frame):
[INFO 10:33:30.743 PDT] (test-frame): Server in /private/var/folders/1r/mzzwlxp953ldf540k1cjpyvm0000gn/T/junit5151700688125605579/test-frame on null is currently not responding.
[INFO 10:33:30.743 PDT] (test-frame):
org.junit.ComparisonFailure:
Expected :1
Actual :0
<Click to see difference>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129714136
--- Diff: geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/gfsh/GfshRule.java ---
@@ -102,35 +107,52 @@ protected ProcessBuilder toProcessBuilder(GfshScript gfshScript, Path gfshPath,
commandsToExecute.add("-e " + command);
}
- return new ProcessBuilder(commandsToExecute).directory(workingDir);
+ ProcessBuilder p = new ProcessBuilder(commandsToExecute);
+ p.directory(workingDir);
+
+ // TODO PSRhomberg -- Input requested: Is this OS agnostic
+ List<String> extendedClasspath = gfshScript.getExtendedClasspath();
+ if (!extendedClasspath.isEmpty()) {
+ Map<String, String> m = p.environment();
+ String classpathKey = "CLASSPATH";
+ String existingJavaArgs = m.get(classpathKey);
+ String specified = String.join(":", extendedClasspath);
--- End diff --
Linux and Mac use ":" as path separator. Use this instead:
org.apache.commons.lang.SystemUtils.PATH_SEPARATOR
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129673457
--- Diff: geode-core/src/main/java/org/apache/geode/internal/ExitCode.java ---
@@ -0,0 +1,61 @@
+/*
+ * 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.geode.internal;
+
+import java.util.Arrays;
+
+import org.springframework.shell.core.ExitShellRequest;
+
+
+public enum ExitCode {
+
+ // The choice of redundant values here is to be consistent with existing behavior
+ // while allowing for future extensibility. JVM_TERMINATED_EXIT(99) exists for coverage of
+ // Spring's ExitShellRequest values in fromSpring.
+ NORMAL(0),
+ FATAL(1),
+ DEPENDENCY_GRAPH_FAILURE(-1),
+ COULD_NOT_EXECUTE_COMMAND(1),
+ INVALID_COMMAND(1),
--- End diff --
It's the exit code that matters. As long as you are not changing the code that each command gives when it's calling system.exit(), you are not changing existing behavior. Or Am I missing anything here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #652: Geode-2971: Introduce ExitCode to resolve inconsistency of...
Posted by PurelyApplied <gi...@git.apache.org>.
Github user PurelyApplied commented on the issue:
https://github.com/apache/geode/pull/652
Precheckin came back fully green. PR squashed and rebased.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129646761
--- Diff: geode-core/src/main/java/org/apache/geode/internal/ExitCode.java ---
@@ -0,0 +1,61 @@
+/*
+ * 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.geode.internal;
+
+import java.util.Arrays;
+
+import org.springframework.shell.core.ExitShellRequest;
+
+
+public enum ExitCode {
+
+ // The choice of redundant values here is to be consistent with existing behavior
+ // while allowing for future extensibility. JVM_TERMINATED_EXIT(99) exists for coverage of
+ // Spring's ExitShellRequest values in fromSpring.
+ NORMAL(0),
+ FATAL(1),
+ DEPENDENCY_GRAPH_FAILURE(-1),
+ COULD_NOT_EXECUTE_COMMAND(1),
+ INVALID_COMMAND(1),
--- End diff --
I would think for each enum, the value passed to it would be distinct. What's the reason for multiple enum value have the same shellExitCode? Would this be more concise?
enum ExitCode{
DEPENDENCY_GRAPH_FAILURE(-1),
NORMAL(0),
FATAL(1),
INSTALL_FAILURE(2),
}
I am hesitant to put the exit code used only for test in here. But that's debatable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #652: Geode-2971: Introduce ExitCode to resolve inconsistency of...
Posted by PurelyApplied <gi...@git.apache.org>.
Github user PurelyApplied commented on the issue:
https://github.com/apache/geode/pull/652
@jaredjstewart I don't know why I didn't think to put my questions here instead of imbedding them in my code, just to be removed later. I clearly wasn't on my A-game.
1. In my addition to `GfshRule` to extend the classpath, it was unclear to me whether using `ProcessBuilder`'s `environment` method was properly OS agnostic. (Looking at it again, it also appears that I left variable names `p` and `m` in place instead of more meaningful names. Ugh.)
2. In the `GfshExitCodeStatuscommandsDUnitTest`, I couldn't decide if the utility functions generating the actual `gfsh` command would look better inlined into the test function or as a function call.
3. Akin to the "Should `gfsh help` actually be returning non-zero", existing behavior is to exit with a zero when gfsh is interrupted (`Launcher.java:227`). Is an interruption assumed to be intentional and we should return a zero, or should that actually be returning a nonzero?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #652: Geode-2971: Introduce ExitCode to resolve inconsist...
Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/652#discussion_r129712861
--- Diff: geode-assembly/src/test/java/org/apache/geode/management/internal/cli/shell/GfshExitCodeStatusCommandsDUnitTest.java ---
@@ -0,0 +1,396 @@
+/*
+ * 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.geode.management.internal.cli.shell;
+
+import static java.util.concurrent.TimeUnit.MINUTES;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.internal.AvailablePort;
+import org.apache.geode.internal.ExitCode;
+import org.apache.geode.internal.process.PidFile;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.management.internal.cli.util.ThreePhraseGenerator;
+import org.apache.geode.test.dunit.rules.gfsh.GfshExecution;
+import org.apache.geode.test.dunit.rules.gfsh.GfshRule;
+import org.apache.geode.test.dunit.rules.gfsh.GfshScript;
+import org.apache.geode.test.junit.categories.AcceptanceTest;
+
+// Originally created in response to GEODE-2971
+
+@Category(AcceptanceTest.class)
+@RunWith(JUnitParamsRunner.class)
+public class GfshExitCodeStatusCommandsDUnitTest {
+ private static File toolsJar;
+ private final static ThreePhraseGenerator nameGenerator = new ThreePhraseGenerator();
--- End diff --
Correct keyword ordering (according to the Google style guide we're using) is "static final"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode issue #652: Geode-2971: Introduce ExitCode to resolve inconsistency of...
Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on the issue:
https://github.com/apache/geode/pull/652
To answer one of your `//TODO` questions, I think `gfsh help` should return a normal exit code. Can you collect the other questions you have and post them in this PR? It will be much easier to make sure I've answered them all than to dig through the changes looking for TODOs. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---