----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57820/#review169639 -----------------------------------------------------------
A couple tests appear to still have a couple hardcoded ports (7070 and 1099). Please read what I have to say about chaining of @Before methods and having many subclasses extending super tests. These are the kinds of things I ran into that caused lots of problems when I untangled the worst dunit tests during the upgrade to JUnit 4. I want to avoid reintroducing anything similar to this. geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestServersJUnitTest.java Line 51 (original), 41 (patched) <https://reviews.apache.org/r/57820/#comment242125> 7070 is hardcoded geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestServersJUnitTest.java Line 67 (original), 56 (patched) <https://reviews.apache.org/r/57820/#comment242126> 7070 is hardcoded geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java Line 67 (original), 55 (patched) <https://reviews.apache.org/r/57820/#comment242128> 7070 and 1099 are both hardcoded geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanAuthorizationJUnitTest.java Line 33 (original), 34 (patched) <https://reviews.apache.org/r/57820/#comment242129> Did these changes fix GEODE-1953? geode-core/src/test/java/org/apache/geode/management/internal/security/JavaRmiServerNameTest.java Line 56 (original), 37 (patched) <https://reviews.apache.org/r/57820/#comment242135> Can we delete this line? Or move it into javadocs for testThatJavaRmiServerNameGetsSet? geode-core/src/test/java/org/apache/geode/security/IntegratedClientContainsKeyAuthDistributedTest.java Line 47 (original), 67 (patched) <https://reviews.apache.org/r/57820/#comment242131> It's not really part of this review, but the newer syntax for these join()/checkException() calls is: ```java @Test public void testContainsKey() throws Exception { ... ai1.await(); ai2.await(); } ``` So if you get the chance, use that new syntax. The await() even has a default timeout. geode-core/src/test/java/org/apache/geode/security/IntegratedClientDestroyInvalidateAuthDistributedTest.java Lines 46 (patched) <https://reviews.apache.org/r/57820/#comment242137> This is the most important part of this review and it's mostly because of what I've seen in previous DUnit tests. It's also because JUnit4 has well-defined ordering of invoking @Before/@After in super and sub classes that I think we should rely on. Please don't chain before/after or setup/teardown. Fixing up this sort of thing was a nightmare when I fixed up dunit and brought it up to junit4. First best practice is don't extend another test. Avoid this if possible. If you really have to subclass then minimize what's in either the super class or the sub class. For example, just set a System Property in the subclass to change some basic behavior. It's better to have redundant code than intertwine all the sub classes of the super class. Second best practice (if you really have to do extend a super class) is to give each super and sub class their own uniquely named before and don't chain them: ```java @Before public void beforeSuperClass() throws Exception { @Before public void beforeSubClass() throws Exception { ``` JUnit4 will handle the ordering like this: it will always execute the @Before method(s) in the super class first and then execute the @Before method(s) in the subclass. So if you're going to extend tests, please give them unique @Before methods and don't chain them with super.xxx. geode-core/src/test/java/org/apache/geode/security/IntegratedClientGetPutAuthDistributedTest.java Lines 51 (patched) <https://reviews.apache.org/r/57820/#comment242141> Please don't chain the before/after methods. See previous comments. geode-core/src/test/java/org/apache/geode/security/IntegratedClientRegionClearAuthDistributedTest.java Line 32 (original), 44 (patched) <https://reviews.apache.org/r/57820/#comment242145> Is there any chance the ServerStarterRule changes fix GEODE-1876? geode-core/src/test/java/org/apache/geode/security/IntegratedClientSizeAuthDistributedTest.java Line 32 (original), 44 (patched) <https://reviews.apache.org/r/57820/#comment242146> I'm really tempted to delete these @Ignored tests since we will never enable them. geode-core/src/test/java/org/apache/geode/security/NoShowValue1PostProcessorDUnitTest.java Lines 55 (patched) <https://reviews.apache.org/r/57820/#comment242147> Please don't chain the before/after methods. See previous comments. geode-core/src/test/java/org/apache/geode/security/PDXPostProcessorDUnitTest.java Line 152 (original), 151 (patched) <https://reviews.apache.org/r/57820/#comment242148> Did these changes fix GEODE-2204? geode-core/src/test/java/org/apache/geode/security/PostProcessorDUnitTest.java Lines 57 (patched) <https://reviews.apache.org/r/57820/#comment242150> Please don't chain the before/after methods. See previous comments. geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java Lines 59 (patched) <https://reviews.apache.org/r/57820/#comment242151> Please don't chain the before/after methods. See previous comments. geode-cq/src/test/java/org/apache/geode/security/CQPostProcessorDunitTest.java Lines 55 (patched) <https://reviews.apache.org/r/57820/#comment242154> Please don't chain the before/after methods. See previous comments. - Kirk Lund On March 21, 2017, 9:09 p.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57820/ > ----------------------------------------------------------- > > (Updated March 21, 2017, 9:09 p.m.) > > > Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, > and Patrick Rhomberg. > > > Repository: geode > > > Description > ------- > > * be able to configure the various aspects of the rules before starting the > server when declaring the rules. > * delete the unnecessary abstract test classes > * allowing tests to use default ports if needed > * created HttpClientRule to ease the connection to to pulse server > > > Diffs > ----- > > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityIntegrationTest.java > dee004ffdf9602969cf6f4c5acb59588eef9b03d > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityPostProcessorTest.java > ab21094b41fd0e5966142ced48ee831872a1cbdd > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityWithSSLTest.java > 09c3e355cf99905777fb92bf85a653258df2e05d > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestServersJUnitTest.java > d97bede0d432569d398ca1203ee64220b82df3d5 > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/SwaggerVerificationTest.java > 43960a8983138a368d4fc1657f8518342b6987d0 > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/HttpClientRule.java > PRE-CREATION > > geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseDataExportTest.java > b9e90b6d77f29875e63ba7d742ff4ee55947dc90 > > geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java > 57711258fbbc73570656e14ee8f05550ae32e891 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java > 55687438d1ceb319cedb705d720336e124818637 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java > 46a07ada5cb48d9090879937a28ac8fb285e535d > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOnServerManagerDUnit.java > c2fde4d3659cf86c4b9e6def810cd3f299efbd0e > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterIntegrationTest.java > 0df00b0340cbbcb8dace23e782490b30034f8404 > > geode-core/src/test/java/org/apache/geode/management/internal/security/AccessControlMBeanJUnitTest.java > f8aa0bd73a050ee46ac0cd631ee28b3ff4b5395f > > geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanAuthenticationJUnitTest.java > 31a4d77514f3a073af8a50da69dd1c116de5b149 > > geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanAuthorizationJUnitTest.java > 45a4b6d02a513297620b0f35af5ffb5f6e79deff > > geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanShiroJUnitTest.java > 31679319a49efe4dc3c6163b4f3a6ee6f34b4eda > > geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerStartupRule.java > 11077796b6c234eb63127d0c11c9d07c6bc4b0b5 > > geode-core/src/test/java/org/apache/geode/management/internal/security/CliCommandsSecurityTest.java > 1e092ff837da1af79e87f31bfe91e187b8e7d9a2 > > geode-core/src/test/java/org/apache/geode/management/internal/security/DataCommandsSecurityTest.java > f403c21554a93faaabdc075fdc20dce4379458d5 > > geode-core/src/test/java/org/apache/geode/management/internal/security/DeployCommandsSecurityTest.java > 88cbfe92f18bdc2ac27486e5f804728e012e8ffc > > geode-core/src/test/java/org/apache/geode/management/internal/security/DiskStoreMXBeanSecurityJUnitTest.java > 0187f802758a0efd9f2504f6297140aa61806ab0 > > geode-core/src/test/java/org/apache/geode/management/internal/security/GatewayReceiverMBeanSecurityTest.java > 7cc2f29b35c2bc3ce64e3164dfee44811c680786 > > geode-core/src/test/java/org/apache/geode/management/internal/security/GatewaySenderMBeanSecurityTest.java > 676243e70150e4015a043fc975a21c82c810d7a9 > > geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsPostProcessorTest.java > 62c0b854d351e0a2ac980a6f356367e8f74a51c6 > > geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java > 45d437a528b7554e7a71346807fb341b56008f33 > > geode-core/src/test/java/org/apache/geode/management/internal/security/JavaRmiServerNameTest.java > 070e9057d7594255c984aab9f0222776798f8e64 > > geode-core/src/test/java/org/apache/geode/management/internal/security/LockServiceMBeanAuthorizationJUnitTest.java > 97b9730e10fc6775e156007295aa351e85ef1b50 > > geode-core/src/test/java/org/apache/geode/management/internal/security/MBeanSecurityJUnitTest.java > 956111159e2f57ba6f27dd710dbbc4ec39a9c168 > > geode-core/src/test/java/org/apache/geode/management/internal/security/ManagerMBeanAuthorizationJUnitTest.java > e9e29fd2fbfe31eb89d62f64309cd692693ebc2f > > geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java > 002900c9f9e054d994ff5e3d3b876684e5726c5b > > geode-core/src/test/java/org/apache/geode/security/AbstractSecureServerDUnitTest.java > f8d90db1e018c28f2c5a4dd319e1b628e1f8f63d > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientAuthDUnitTest.java > a8ab5b750508f884842dc811ce552b28d58d35d6 > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientContainsKeyAuthDistributedTest.java > 9a34198c14e7cae5c64c3a445c6e9f6972d1082a > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientDestroyInvalidateAuthDistributedTest.java > 71a81560f62b3fea9de95a6446bd63ebebb7f5d2 > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientDestroyRegionAuthDistributedTest.java > 2366e3d883b5bc582f8b3bfba2d2d8369d3dfbfb > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientExecuteFunctionAuthDistributedTest.java > ad6b22627a620aec84e0d58e79995b173ba64af5 > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientExecuteRegionFunctionAuthDistributedTest.java > eb9de5df753e846c5d4593974bec0e053cdf9cf3 > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientGetAllAuthDistributedTest.java > cec4de643d55a42702cca381b3ae2e43fcd14edf > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientGetClientPRMetaDataAuthDistributedTest.java > f4aa9c929665285e600743ffad74ba40d389c59c > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientGetClientPartitionAttrCmdAuthDistributedTest.java > 706e168c262b43f086bdcbfbdd39b759a3d5d7e8 > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientGetEntryAuthDistributedTest.java > 57fc26356669e29ebd31481a6e67843f788a4de5 > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientGetPutAuthDistributedTest.java > 2d5bfdc1c0bc012a6c784d68e34eb7baf08d4e97 > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientRegionClearAuthDistributedTest.java > d5dbbdb1c26c4af26fa19f4466909f635871332d > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientRegisterInterestAuthDistributedTest.java > d74c7d058772b35868779f7de048158a45365d69 > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientRemoveAllAuthDistributedTest.java > ecb32a705191b45273081bb6d39a63aa062111dd > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientSizeAuthDistributedTest.java > 84c03997101fa51cf4b253824e4f107be9c96a60 > > geode-core/src/test/java/org/apache/geode/security/IntegratedClientUnregisterInterestAuthDistributedTest.java > 8b99d584f5f872bcc67cd257d2fe0147a4d43081 > > geode-core/src/test/java/org/apache/geode/security/NoShowValue1PostProcessorDUnitTest.java > f7a08852b6c49379758c24f885b2e73c02beed8b > > geode-core/src/test/java/org/apache/geode/security/PDXPostProcessorDUnitTest.java > f8b17bc8d24b82f0b1dd19a428e7981b85a30509 > > geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorDUnitTest.java > dd913c2b0be0837cd3042e6a6ec64b6f14058361 > > geode-core/src/test/java/org/apache/geode/security/PeerSecurityWithEmbeddedLocatorDUnitTest.java > 9cfb5ec5f5b10f3a2dd55470bffa82964d01f154 > > geode-core/src/test/java/org/apache/geode/security/PostProcessorDUnitTest.java > ed16f0cf06f9aec4562fcc906328fc2209918513 > > geode-core/src/test/java/org/apache/geode/security/SecurityWithoutClusterConfigDUnitTest.java > 52a4ce49e18fe829d14dac51993a330a627da54b > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > 01e346ae0b89911f26834238053bba91490ba902 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorStarterRule.java > 5d6a0bea3db2b02ef60c8f57b4fb3b7945b293c1 > geode-core/src/test/java/org/apache/geode/test/dunit/rules/Member.java > 6165c84533c2628a3db951828136532f29a200d0 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/MemberStarterRule.java > 75916161d81cac622930eadca74d47e50f04aacf > geode-core/src/test/java/org/apache/geode/test/dunit/rules/MemberVM.java > 988f5d4b3f6c430f10f0dec4c2c72e9447b97aac > geode-core/src/test/java/org/apache/geode/test/dunit/rules/Server.java > fb8630aaefebd034ddbf07fc6d786c9d0608b560 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java > 04543402d3775b2e7ff3a0e49da70349fba86c1b > geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java > ce1782b324ffbb3175efe53f54c33fa847613193 > > geode-cq/src/test/java/org/apache/geode/security/CQPDXPostProcessorDUnitTest.java > 0bafb6831ba9892fbcc8aef0eb023e4dcf7972a4 > > geode-cq/src/test/java/org/apache/geode/security/CQPostProcessorDunitTest.java > 8510dce91dde9d77343c2274afecb52eca52d0cd > > geode-cq/src/test/java/org/apache/geode/security/ClientQueryAuthDistributedTest.java > 9d82ae94dc36c5884cbeec7f22adff8c6b200ef1 > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpIntegrationTest.java > 420f2ddb552a6de6cd6b5258add446527abccd1b > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java > 22ca6e3df86d7bf9142eadb94b26847518fc1263 > > geode-web/src/test/java/org/apache/geode/management/internal/security/GfshCommandsOverHttpSecurityTest.java > 5449568bb19a3d09cf17cbe6614d643a0693a4de > > > Diff: https://reviews.apache.org/r/57820/diff/1/ > > > Testing > ------- > > precheckin running > > > Thanks, > > Jinmei Liao > >
