[GitHub] geode pull request #664: Fix for GEODE-3292

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/geode/pull/664


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jujoramos]

Github user jujoramos commented on a diff in the pull request:

https://github.com/apache/geode/pull/664#discussion_r131378087
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/AbstractPulseConnectivityTest.java
 ---
@@ -15,26 +15,31 @@
 
 package org.apache.geode.tools.pulse;
 
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
 import static org.assertj.core.api.Assertions.assertThat;
 
-import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
-import org.apache.geode.test.dunit.rules.HttpClientRule;
-import org.apache.geode.test.dunit.rules.LocatorStarterRule;
-import org.apache.geode.test.junit.categories.IntegrationTest;
-import org.apache.geode.tools.pulse.internal.data.Cluster;
+import java.io.File;
+import java.net.InetAddress;
+import java.util.Properties;
+
 import org.apache.http.HttpResponse;
 import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
+import org.junit.BeforeClass;
 
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.HttpClientRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
 
-@Category(IntegrationTest.class)
-public class PulseVerificationTest {
+public abstract class AbstractPulseConnectivityTest {
--- End diff --

Thanks for the tips, I couldn't find anything related to this intention 
within 
[Writing 
Tests](https://cwiki.apache.org/confluence/display/GEODE/Writing+tests) but I'm 
okay anyway.
I'll give it one more try in my next commit later today, change things as 
you please :-).


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jujoramos]

Github user jujoramos commented on a diff in the pull request:

https://github.com/apache/geode/pull/664#discussion_r131367740
  
--- Diff: 
geode-pulse/src/test/java/org/apache/geode/tools/pulse/internal/PulseAppListenerTest.java
 ---
@@ -0,0 +1,91 @@
+/*
+ * 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.tools.pulse.internal;
+
+import javax.servlet.ServletContextEvent;
+import static org.mockito.Mockito.*;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TestRule;
+
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.apache.geode.tools.pulse.internal.data.PulseConstants;
+import org.apache.geode.tools.pulse.internal.data.Repository;
+
+@Category(UnitTest.class)
+public class PulseAppListenerTest {
+  private Repository repository;
+  private PulseAppListener appListener;
+
+  @Rule
+  public final TestRule restoreSystemProperties = new 
RestoreSystemProperties();
+
+  @Before
+  public void setUp() {
+repository = Repository.get();
+appListener = new PulseAppListener();
+  }
+
+  @Test
+  public void embeddedModeDefaultPropertiesRepositoryInitializationTest() {
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, 
"true");
+appListener.contextInitialized(mock(ServletContextEvent.class));
+
+Assert.assertEquals(false, repository.getJmxUseLocator());
+Assert.assertEquals(false, repository.isUseSSLManager());
+Assert.assertEquals(false, repository.isUseSSLLocator());
+Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_PORT, 
repository.getPort());
+Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_HOST, 
repository.getHost());
+
+  }
+
+  @Test
+  public void 
embeddedModeNonDefaultPropertiesRepositoryInitializationTest() {
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, 
"true");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_PORT, "");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_HOST, 
"nonDefaultBindAddress");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_MANAGER,
+Boolean.TRUE.toString());
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_LOCATOR,
+Boolean.TRUE.toString());
+
+appListener.contextInitialized(mock(ServletContextEvent.class));
+
+Assert.assertEquals(false, repository.getJmxUseLocator());
+Assert.assertEquals(true, repository.isUseSSLManager());
+Assert.assertEquals(true, repository.isUseSSLLocator());
+Assert.assertEquals("", repository.getPort());
+Assert.assertEquals("nonDefaultBindAddress", repository.getHost());
+  }
+
+  @After
+  public void tearDown() {
+if (repository != null) {
--- End diff --

Yes, I thought about using the existing rule but it implies adding a 
dependency from `geode-web` to `geode-assembly` as the rule is defined there, 
or duplicating the rule source code, that's why I didn't do it. Which approach 
would be best?.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/664#discussion_r131182576
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/AbstractPulseConnectivityTest.java
 ---
@@ -15,26 +15,31 @@
 
 package org.apache.geode.tools.pulse;
 
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
 import static org.assertj.core.api.Assertions.assertThat;
 
-import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
-import org.apache.geode.test.dunit.rules.HttpClientRule;
-import org.apache.geode.test.dunit.rules.LocatorStarterRule;
-import org.apache.geode.test.junit.categories.IntegrationTest;
-import org.apache.geode.tools.pulse.internal.data.Cluster;
+import java.io.File;
+import java.net.InetAddress;
+import java.util.Properties;
+
 import org.apache.http.HttpResponse;
 import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
+import org.junit.BeforeClass;
 
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.HttpClientRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
 
-@Category(IntegrationTest.class)
-public class PulseVerificationTest {
+public abstract class AbstractPulseConnectivityTest {
--- End diff --

We would like to get rid of the usage of abstract test classes in favor of 
rules and parameterized tests. I see further improvements on these tests. But 
if you don't mind, I can pull your changes in and make modifications on top of 
yours.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/664#discussion_r131182711
  
--- Diff: 
geode-pulse/src/test/java/org/apache/geode/tools/pulse/internal/PulseAppListenerTest.java
 ---
@@ -0,0 +1,91 @@
+/*
+ * 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.tools.pulse.internal;
+
+import javax.servlet.ServletContextEvent;
+import static org.mockito.Mockito.*;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TestRule;
+
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.apache.geode.tools.pulse.internal.data.PulseConstants;
+import org.apache.geode.tools.pulse.internal.data.Repository;
+
+@Category(UnitTest.class)
+public class PulseAppListenerTest {
+  private Repository repository;
+  private PulseAppListener appListener;
+
+  @Rule
+  public final TestRule restoreSystemProperties = new 
RestoreSystemProperties();
+
+  @Before
+  public void setUp() {
+repository = Repository.get();
+appListener = new PulseAppListener();
+  }
+
+  @Test
+  public void embeddedModeDefaultPropertiesRepositoryInitializationTest() {
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, 
"true");
+appListener.contextInitialized(mock(ServletContextEvent.class));
+
+Assert.assertEquals(false, repository.getJmxUseLocator());
+Assert.assertEquals(false, repository.isUseSSLManager());
+Assert.assertEquals(false, repository.isUseSSLLocator());
+Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_PORT, 
repository.getPort());
+Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_HOST, 
repository.getHost());
+
+  }
+
+  @Test
+  public void 
embeddedModeNonDefaultPropertiesRepositoryInitializationTest() {
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, 
"true");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_PORT, "");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_HOST, 
"nonDefaultBindAddress");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_MANAGER,
+Boolean.TRUE.toString());
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_LOCATOR,
+Boolean.TRUE.toString());
+
+appListener.contextInitialized(mock(ServletContextEvent.class));
+
+Assert.assertEquals(false, repository.getJmxUseLocator());
+Assert.assertEquals(true, repository.isUseSSLManager());
+Assert.assertEquals(true, repository.isUseSSLLocator());
+Assert.assertEquals("", repository.getPort());
+Assert.assertEquals("nonDefaultBindAddress", repository.getHost());
+  }
+
+  @After
+  public void tearDown() {
+if (repository != null) {
--- End diff --

this looks familiar, can you use EmbeddedPulseRule for this?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jujoramos]

Github user jujoramos commented on a diff in the pull request:

https://github.com/apache/geode/pull/664#discussion_r131071385
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
 ---
@@ -270,6 +271,7 @@ private void startHttpService(boolean isServer) {
   }
 
   System.setProperty(PULSE_EMBEDDED_PROP, "true");
+  System.setProperty(PULSE_HOST_PROP, "" + 
config.getJmxManagerBindAddress());
--- End diff --

I'll rewrite the tests and push the changes again, thanks for the 
suggestions!.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jujoramos]

Github user jujoramos commented on a diff in the pull request:

https://github.com/apache/geode/pull/664#discussion_r131071365
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java
 ---
@@ -0,0 +1,110 @@
+/*
+ * 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.tools.pulse;
+
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
+@Category(IntegrationTest.class)
+public class PulseConnectivityTest {
+  @ClassRule
+  public static LocatorStarterRule locator = new 
LocatorStarterRule().withJMXManager();
+
+  // Test Connectivity for Default Configuration
+  @Category(IntegrationTest.class)
+  public static class DefaultBindAddressTest {
+@Rule
+public EmbeddedPulseRule pulse = new EmbeddedPulseRule();
+
+@BeforeClass
+public static void beforeClass() throws Exception {
--- End diff --

True, but I'm not using `withAutoStart()` for a good reason: the two inner 
classes need to setup the locator differently (the `jmx-manager-bind-address` 
value changes). 
I thought it's easier to read a single functional test class annotated with 
`@RunWith(Enclosed.class)` internally split in different inner classes than 
write a new test class to test exactly the same but changing a single locator 
property.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/664#discussion_r131050219
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java
 ---
@@ -0,0 +1,110 @@
+/*
+ * 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.tools.pulse;
+
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
--- End diff --

the test you removed is slightly different from the one in 
PulseSecurityTest, there it's using a securityManager, but here, it doesn't, 
it's making sure the default username/password (admin/admin) works.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/664#discussion_r131049554
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java
 ---
@@ -0,0 +1,110 @@
+/*
+ * 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.tools.pulse;
+
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
+@Category(IntegrationTest.class)
+public class PulseConnectivityTest {
+  @ClassRule
+  public static LocatorStarterRule locator = new 
LocatorStarterRule().withJMXManager();
+
+  // Test Connectivity for Default Configuration
+  @Category(IntegrationTest.class)
+  public static class DefaultBindAddressTest {
+@Rule
+public EmbeddedPulseRule pulse = new EmbeddedPulseRule();
+
+@BeforeClass
+public static void beforeClass() throws Exception {
+  locator.startLocator();
+}
+
+@Test
+public void testConnectToJmx() throws Exception {
+  pulse.useJmxPort(locator.getJmxPort());
+  Cluster cluster = pulse.getRepository().getCluster("admin", null);
+  assertThat(cluster.isConnectedFlag()).isTrue();
+  assertThat(cluster.getServerCount()).isEqualTo(0);
+}
+
+@Test
+public void testConnectToLocator() throws Exception {
+  pulse.useLocatorPort(locator.getPort());
+  Cluster cluster = pulse.getRepository().getCluster("admin", null);
+  assertThat(cluster.isConnectedFlag()).isTrue();
+  assertThat(cluster.getServerCount()).isEqualTo(0);
+}
+
+@AfterClass
+public static void afterClass() throws Exception {
--- End diff --

this is not needed. The rule guarantees that this is called.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/664#discussion_r131049401
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java
 ---
@@ -0,0 +1,110 @@
+/*
+ * 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.tools.pulse;
+
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
--- End diff --

I am not sure I've seen this pattern before. It looks like you are adding 
another set of test to the original PulseVerificationTest. I believe another 
separate test class would read better. I would leave the original test alone 
(since it has a test that uses httpClient to try to connect with a wrong 
password). 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/664#discussion_r131050061
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
 ---
@@ -270,6 +271,7 @@ private void startHttpService(boolean isServer) {
   }
 
   System.setProperty(PULSE_EMBEDDED_PROP, "true");
+  System.setProperty(PULSE_HOST_PROP, "" + 
config.getJmxManagerBindAddress());
--- End diff --

+1 the product code change is good to go. Just some test re-org is needed.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/664#discussion_r131049994
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/EmbeddedPulseRule.java
 ---
@@ -38,6 +38,12 @@ protected void before() throws Throwable {
 repository.setHost("localhost");
   }
 
+  public void useJmxManager(String jmxHost, int jmxPort) {
--- End diff --

+1, very nice addition


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

[jujoramos]

GitHub user jujoramos opened a pull request:

https://github.com/apache/geode/pull/664

Fix for GEODE-3292

Embedded PULSE Connection Failure When jmx-manager-bind-address != localhost

- ManagementAgent sets the 'pulse.host' system property whenever a 
'jmx-manager-bind-address' is configured.
- PULSE gets the hostname of the local JMX Manager through the 'pulse.host' 
system property, reverting to 'localhost' by default.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in 
the commit message?

- [X] Has your PR been rebased against the latest commit within the target 
branch (typically `develop`)?

- [X] Is your initial contribution a single, squashed commit?

- [X] Does `gradlew build` run cleanly?

- [X] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build 
issues and
submit an update to your PR as soon as possible. If you need help, please 
send an
email to dev@geode.apache.org.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jujoramos/geode feature/GEODE-3292

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/geode/pull/664.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 #664


commit 0c3e15f619ddd08f02ba8e2a8818f22d308c8bde
Author: Juan Jose Ramos Cassella 
Date:   2017-07-28T09:55:20Z

GEODE-3292: Embedded PULSE Connection Failure When jmx-manager-bind-address 
!= localhost

- ManagementAgent sets the 'pulse.host' system property whenever a 
'jmx-manager-bind-address' is configured.
- PULSE gets the hostname of the local JMX Manager through the 'pulse.host' 
system property, reverting to 'localhost' by default.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---