You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jinmei Liao <ji...@pivotal.io> on 2016/10/28 21:49:36 UTC
Review Request 53274: GEODE-1955: fix flaky tests by properly closing
the gfsh instance in ConnectToLocatorSSLDUnitTest
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53274/
-----------------------------------------------------------
Review request for geode, Kevin Duling and Kirk Lund.
Repository: geode
Description
-------
GEODE-1955: fix flaky tests by properly closing the gfsh instance in ConnectToLocatorSSLDUnitTest
Diffs
-----
geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java 8426c6f52129472c9c0def172aeefd10f1491935
geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsPostProcessorTest.java d3390ba6ce2990d05516b701f82d5f176e6f4d49
geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java b2dc6fe334499456184e1d0cbc0297063d4c99ce
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java 25780e67dd3f7920e53162ef9400d2845e4c958b
Diff: https://reviews.apache.org/r/53274/diff/
Testing
-------
Thanks,
Jinmei Liao
Re: Review Request 53274: GEODE-1955: fix flaky tests by properly
closing the gfsh instance in ConnectToLocatorSSLDUnitTest
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53274/#review154300
-----------------------------------------------------------
Use of the Rule should be cleaned up so everything is contained inside the Rule. Otherwise there's no value in trying to package this up as a Rule.
geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java (line 65)
<https://reviews.apache.org/r/53274/#comment223785>
This Rule isn't being used as a Rule so I'm confused?
This would be correct:
@Rule
public GfshShellConnectionRule gfshConnector = new GfshShellConnectionRule();
The before and after should be built into the Rule so lines #78-80 in after() wouldn't be necessary either.
If this is not being used as a Rule because some tests require different scaffolding from other tests, then the tests should be moved into a different test class.
geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java (line 84)
<https://reviews.apache.org/r/53274/#comment223787>
You should keep these as "throws Exception" -- if you're calling something that requires "throws Throwable" then we should change that class/method to "throws Exception"
- Kirk Lund
On Oct. 28, 2016, 9:49 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53274/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2016, 9:49 p.m.)
>
>
> Review request for geode, Kevin Duling and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1955: fix flaky tests by properly closing the gfsh instance in ConnectToLocatorSSLDUnitTest
>
>
> Diffs
> -----
>
> geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java 8426c6f52129472c9c0def172aeefd10f1491935
> geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsPostProcessorTest.java d3390ba6ce2990d05516b701f82d5f176e6f4d49
> geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java b2dc6fe334499456184e1d0cbc0297063d4c99ce
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java 25780e67dd3f7920e53162ef9400d2845e4c958b
>
> Diff: https://reviews.apache.org/r/53274/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 53274: GEODE-1955: fix flaky tests by properly
closing the gfsh instance in ConnectToLocatorSSLDUnitTest
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53274/#review154301
-----------------------------------------------------------
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java (line 34)
<https://reviews.apache.org/r/53274/#comment223789>
I really, really recommend not using the Java primitive wrappers as trinary variables.
A Boolean should be a true/false boolean as an object without this broken use of null. Boolean is by definition a binary. If you really have to have a tinary then please create a new enum or class:
public enum Source {
Locator, HTTP, RMI
}
Or whatever this is supposed to represent. The problem here is maintainability and clarity of purpose. I don't understand what you're doing or why without studying this code very closely. Everything should be very obvious and simple. GFSH and related code is already obfuscated well enough.
- Kirk Lund
On Oct. 28, 2016, 9:49 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53274/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2016, 9:49 p.m.)
>
>
> Review request for geode, Kevin Duling and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1955: fix flaky tests by properly closing the gfsh instance in ConnectToLocatorSSLDUnitTest
>
>
> Diffs
> -----
>
> geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java 8426c6f52129472c9c0def172aeefd10f1491935
> geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsPostProcessorTest.java d3390ba6ce2990d05516b701f82d5f176e6f4d49
> geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java b2dc6fe334499456184e1d0cbc0297063d4c99ce
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java 25780e67dd3f7920e53162ef9400d2845e4c958b
>
> Diff: https://reviews.apache.org/r/53274/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 53274: GEODE-1955: fix flaky tests by properly
closing the gfsh instance in ConnectToLocatorSSLDUnitTest
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53274/
-----------------------------------------------------------
(Updated Nov. 3, 2016, 7:38 p.m.)
Review request for geode, Kevin Duling and Kirk Lund.
Repository: geode
Description
-------
GEODE-1955: fix flaky tests by properly closing the gfsh instance in ConnectToLocatorSSLDUnitTest
Diffs
-----
geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java 8426c6f52129472c9c0def172aeefd10f1491935
geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsPostProcessorTest.java d3390ba6ce2990d05516b701f82d5f176e6f4d49
geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java b2dc6fe334499456184e1d0cbc0297063d4c99ce
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java 25780e67dd3f7920e53162ef9400d2845e4c958b
geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java 4e10b0acae6c6aac88b6039560e4cf65b0a012cb
geode-web/src/test/java/org/apache/geode/management/internal/security/GfshCommandsOverHttpSecurityTest.java d4775659c0ff2704fd55a3840fc3751e66886d26
Diff: https://reviews.apache.org/r/53274/diff/
Testing (updated)
-------
precheckin successful
Thanks,
Jinmei Liao
Re: Review Request 53274: GEODE-1955: fix flaky tests by properly
closing the gfsh instance in ConnectToLocatorSSLDUnitTest
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53274/
-----------------------------------------------------------
(Updated Nov. 1, 2016, 5:19 p.m.)
Review request for geode, Kevin Duling and Kirk Lund.
Repository: geode
Description
-------
GEODE-1955: fix flaky tests by properly closing the gfsh instance in ConnectToLocatorSSLDUnitTest
Diffs (updated)
-----
geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java 8426c6f52129472c9c0def172aeefd10f1491935
geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsPostProcessorTest.java d3390ba6ce2990d05516b701f82d5f176e6f4d49
geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java b2dc6fe334499456184e1d0cbc0297063d4c99ce
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java 25780e67dd3f7920e53162ef9400d2845e4c958b
geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java 4e10b0acae6c6aac88b6039560e4cf65b0a012cb
geode-web/src/test/java/org/apache/geode/management/internal/security/GfshCommandsOverHttpSecurityTest.java d4775659c0ff2704fd55a3840fc3751e66886d26
Diff: https://reviews.apache.org/r/53274/diff/
Testing
-------
Thanks,
Jinmei Liao
Re: Review Request 53274: GEODE-1955: fix flaky tests by properly
closing the gfsh instance in ConnectToLocatorSSLDUnitTest
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53274/
-----------------------------------------------------------
(Updated Nov. 1, 2016, 5:18 p.m.)
Review request for geode, Kevin Duling and Kirk Lund.
Repository: geode
Description
-------
GEODE-1955: fix flaky tests by properly closing the gfsh instance in ConnectToLocatorSSLDUnitTest
Diffs (updated)
-----
geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java 8426c6f52129472c9c0def172aeefd10f1491935
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java 25780e67dd3f7920e53162ef9400d2845e4c958b
Diff: https://reviews.apache.org/r/53274/diff/
Testing
-------
Thanks,
Jinmei Liao
Re: Review Request 53274: GEODE-1955: fix flaky tests by properly
closing the gfsh instance in ConnectToLocatorSSLDUnitTest
Posted by Kevin Duling <kd...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53274/#review154187
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Duling
On Oct. 28, 2016, 2:49 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53274/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2016, 2:49 p.m.)
>
>
> Review request for geode, Kevin Duling and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-1955: fix flaky tests by properly closing the gfsh instance in ConnectToLocatorSSLDUnitTest
>
>
> Diffs
> -----
>
> geode-core/src/test/java/org/apache/geode/management/ConnectToLocatorSSLDUnitTest.java 8426c6f52129472c9c0def172aeefd10f1491935
> geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsPostProcessorTest.java d3390ba6ce2990d05516b701f82d5f176e6f4d49
> geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java b2dc6fe334499456184e1d0cbc0297063d4c99ce
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java 25780e67dd3f7920e53162ef9400d2845e4c958b
>
> Diff: https://reviews.apache.org/r/53274/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>