You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "PurelyApplied (GitHub)" <gi...@apache.org> on 2018/10/24 22:22:35 UTC
[GitHub] [geode] PurelyApplied opened pull request #2706: GEODE-5928:
Refactor / rewrite of WANSSLDunitTest
* Added robustness of test cleanup by way of newer ClusterStarterRule and the removal of many statics
* The actual test behavior should be unchanged, excepting the simplification
of logic surrounding the potential of a dirty testing environment.
* Parameterize test to run each test with 1, 3, and 5 dispatcher threads, rather than choosing one value at random.
* Grouped methods for readability.
* Additional refactoring is recommended to make use of the locator and server starter rules,
rather than invoking custom creation in otherwise-empty VMs.
* Address GEODE-5819 with respect to this test via changes proposed in PR#2702.
* * default.keystore file picked from that PR, sha b0d06ff125968b5284d2a3b136d414a8ee2897fc
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?
- [n/a] 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/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2706:
GEODE-5928: Refactor / rewrite of WANSSLDunitTest
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Another unexpected exception to not catch. If this is really problematic, you could do this instead:
```
try {
...
} catch (IOException e) {
throw new UncheckedIOException(e);
}
```
[ Full content available at: https://github.com/apache/geode/pull/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2706:
GEODE-5928: Refactor / rewrite of WANSSLDunitTest
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Ignore if you want: Using "setProperty" is actually more correct here. The "put" method is inherited from Map.
[ Full content available at: https://github.com/apache/geode/pull/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] balesh2 commented on pull request #2706: GEODE-5928:
Refactor / rewrite of WANSSLDunitTest
Posted by "balesh2 (GitHub)" <gi...@apache.org>.
Did you mean to leave this here? Would it be a more useful error if it were passed into the fail()? (I forget if fail takes an exception or not)
[ Full content available at: https://github.com/apache/geode/pull/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] balesh2 commented on pull request #2706: GEODE-5928:
Refactor / rewrite of WANSSLDunitTest
Posted by "balesh2 (GitHub)" <gi...@apache.org>.
You could inline this and the previous line to make it read nicely
[ Full content available at: https://github.com/apache/geode/pull/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2706:
GEODE-5928: Refactor / rewrite of WANSSLDunitTest
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Instead of catch unexpected exception, please change the method to "throws IOException" or rethrow the IOException as an UncheckedIOException:
```
try {
...
} catch (IOException e) {
throw new UncheckedIOException(e);
}
```
[ Full content available at: https://github.com/apache/geode/pull/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2706:
GEODE-5928: Refactor / rewrite of WANSSLDunitTest
Posted by "kirklund (GitHub)" <gi...@apache.org>.
You could change this to use try-resource instead of try-finally and also no need to specify class.getName():
```
import static org.apache.geode.test.dunit.IgnoredException.addIgnoredException;
try (IgnoredException ie1 = addIgnoredException(ForceReattemptException.class);
IgnoredException ie2 = addIgnoredException(InterruptedException.class);
IgnoredException ie3 = addIgnoredException(GatewaySenderException.class)) {
...
}
```
[ Full content available at: https://github.com/apache/geode/pull/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] balesh2 commented on pull request #2706: GEODE-5928:
Refactor / rewrite of WANSSLDunitTest
Posted by "balesh2 (GitHub)" <gi...@apache.org>.
It isn't strictly necessary, but you could refactor this and createFirstLocatorWithDSIdOne to be one method with parameters of DSID and remoteLocPort, which would just take -1 for the first one.
[ Full content available at: https://github.com/apache/geode/pull/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied commented on issue #2706: GEODE-5928:
Refactor / rewrite of WANSSLDunitTest
Posted by "PurelyApplied (GitHub)" <gi...@apache.org>.
See #2702 for keystore changes. The true scope of this ticket is in reforming `WANSSLDUnitTest` so that future developers do not have to struggle as much with `WANTestBase` / `DistributedTestCase` / `JUnit4DistributedTestCase` chasing.
The actual content of this rewrite is the result of inlining methods and pushing methods down from the parent class into this test class. Most methods and fields were renamed, statics turned to instance variables to avoid test environment leakage, and added parameterization on a previously-randomized configuration option.
Additional refactoring is certainly possible, notably that we should use the actual `MemberStarterRules` to create servers and locators, rather than these custom cache / member creation methods in otherwise empty VMs. Additionally, this test relies heavily on internals, particularly surrounding the `InternalDistributedSystem`.
[ Full content available at: https://github.com/apache/geode/pull/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on issue #2706: GEODE-5928:
Refactor / rewrite of WANSSLDunitTest
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
+0 Not sure if this is necessary if we are not actually rewriting the test using the rules. Looks like we just got rid of the parent class and duplicate a lot of the code?
[ Full content available at: https://github.com/apache/geode/pull/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] PurelyApplied closed pull request #2706: GEODE-5928:
Refactor / rewrite of WANSSLDunitTest
Posted by "PurelyApplied (GitHub)" <gi...@apache.org>.
[ pull request closed by PurelyApplied ]
[ Full content available at: https://github.com/apache/geode/pull/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] balesh2 commented on pull request #2706: GEODE-5928:
Refactor / rewrite of WANSSLDunitTest
Posted by "balesh2 (GitHub)" <gi...@apache.org>.
Is there a reason to not use Invoke.invokeInEveryVM here?
[ Full content available at: https://github.com/apache/geode/pull/2706 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org