You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@geode.apache.org by "mhansonp (GitHub)" <gi...@apache.org> on 2018/10/18 16:53:30 UTC

[GitHub] [geode] mhansonp opened pull request #2654: Geode 5793: LocatorDUnitTest. testNonSSLLocatorDiesWhenConnectingToSSLLocator

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:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

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

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

- [ ] Does `gradlew build` run cleanly?

- [ ] 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.


[ Full content available at: https://github.com/apache/geode/pull/2654 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mhansonp commented on issue #2654: Geode 5793: LocatorDUnitTest. testNonSSLLocatorDiesWhenConnectingToSSLLocator

Posted by "mhansonp (GitHub)" <gi...@apache.org>.

This fix was originally part of a 5728 fix, which carried into a the 5793 fix.

[ Full content available at: https://github.com/apache/geode/pull/2654 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #2654: Geode 5793: LocatorDUnitTest. testNonSSLLocatorDiesWhenConnectingToSSLLocator

Posted by "kirklund (GitHub)" <gi...@apache.org>.

Does "getConnectedDistributedSystem" actually throw GemFireConfigException? If this test doesn't need that "catch (GemFireConfigException" then delete it. If this test is actually verifying that GemFireConfigException should be thrown then it needs some more help.

Either add a fail statement in just before the catch so the test fails if the expected exception is not thrown. Or rewrite the block to use catchThrowable:
```
import static org.assertj.core.api.Assertions.catchThrowable;

Throwable thrown = catchThrowable(() -> getConnectedDistributedSystem(props));
assertThat(thrown).isInstanceOf(GemFireConfigException.class).hasMessageContaining("Rejecting findCoordinatorRequest");
```
Lines like "vm.invoke(LocatorDUnitTest::stopLocator);" would typically be better in a tearDown() method rather than a finally block.

[ Full content available at: https://github.com/apache/geode/pull/2654 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #2654: Geode 5793: LocatorDUnitTest. testNonSSLLocatorDiesWhenConnectingToSSLLocator

Posted by "kirklund (GitHub)" <gi...@apache.org>.

Ignore if you want: If you change the caller of restartLocator to be a lambda call, then you can delete these catch blocks and change the restartLocator method to have "throws Exception":
```
locatorRestart0.invoke(() -> restartLocator(port0, logFile0, props));

protected void restartLocator(int port0, File logFile0, Properties props) throws Exception {
```

[ Full content available at: https://github.com/apache/geode/pull/2654 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal commented on issue #2654: Geode 5793: LocatorDUnitTest. testNonSSLLocatorDiesWhenConnectingToSSLLocator

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.

Can you please explain what this fix does? And what the behavior change is? It looks to me like you're causing the locator to exit early when it gets a certain exception, but this changes some of the later setup. Do you think that not rethrowing the exception was simply an error on the part of the original writers? 

[ Full content available at: https://github.com/apache/geode/pull/2654 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] galen-pivotal closed pull request #2654: Geode 5793: LocatorDUnitTest. testNonSSLLocatorDiesWhenConnectingToSSLLocator

Posted by "galen-pivotal (GitHub)" <gi...@apache.org>.

[ pull request closed by galen-pivotal ]

[ Full content available at: https://github.com/apache/geode/pull/2654 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mhansonp commented on pull request #2654: Geode 5793: LocatorDUnitTest. testNonSSLLocatorDiesWhenConnectingToSSLLocator

Posted by "mhansonp (GitHub)" <gi...@apache.org>.

Done. Cleaned up this whole file...

[ Full content available at: https://github.com/apache/geode/pull/2654 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mhansonp commented on pull request #2654: Geode 5793: LocatorDUnitTest. testNonSSLLocatorDiesWhenConnectingToSSLLocator

Posted by "mhansonp (GitHub)" <gi...@apache.org>.

@WireBaron @galen-pivotal Please review.

[ Full content available at: https://github.com/apache/geode/pull/2654 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mhansonp commented on issue #2654: Geode 5793: LocatorDUnitTest. testNonSSLLocatorDiesWhenConnectingToSSLLocator

Posted by "mhansonp (GitHub)" <gi...@apache.org>.

This fix basically causes a LocatorCancelException to be propogated up when you get an SSL based connection failure, because, it is not running SSL at the time. Previously the test was just failing, when it was testing for this condition. Now with the exception flowing out of the lower level code, this test can pass because it can see the exception.

[ Full content available at: https://github.com/apache/geode/pull/2654 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mhansonp commented on pull request #2654: Geode 5793: LocatorDUnitTest. testNonSSLLocatorDiesWhenConnectingToSSLLocator

Posted by "mhansonp (GitHub)" <gi...@apache.org>.

This is the fix...

[ Full content available at: https://github.com/apache/geode/pull/2654 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mhansonp commented on pull request #2654: Geode 5793: LocatorDUnitTest. testNonSSLLocatorDiesWhenConnectingToSSLLocator

Posted by "mhansonp (GitHub)" <gi...@apache.org>.

Done. Added a fail statement.

[ Full content available at: https://github.com/apache/geode/pull/2654 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org