----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58388/#review171938 -----------------------------------------------------------
geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityWithSSLTest.java Lines 48 (patched) <https://reviews.apache.org/r/58388/#comment244960> It would be better for this to be non-static and set within a @Before method -- this is just a standard JUnit best practice for all test state. Also, if it's static then it gets set early during class-load time. By the time JUnit goes to execute any of the tests, the port may no longer be available: ```java private int restPort; @Before public void before() throws Exception { restPort = AvailablePortHelper.getRandomAvailableTCPPort(); } ``` geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/EmbeddedPulseRule.java Lines 51 (patched) <https://reviews.apache.org/r/58388/#comment244961> This should have a null-check. In theory Repository.get() could return a null or throw an Exception. The before() will then throw but after() is still called and then after() will throw NPE which will cause the original Exception from before() to be suppressed. geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterIntegrationTest.java Line 108 (original), 94 (patched) <https://reviews.apache.org/r/58388/#comment244962> Is this @Ignore represented by any GEODE-nn tickets? We should file one (if it doesn't already exist) and add the ticket # here. geode-core/src/test/java/org/apache/geode/management/internal/security/DataCommandsSecurityTest.java Line 58 (original), 58 (patched) <https://reviews.apache.org/r/58388/#comment244964> I liked your use of RuleChain on the 1st page of reviewed code better than using @ClassRule and @Rule in this way. The problem is that it presumably finds a randomly available port during class-load and then finally uses it later during test runtime. Also, in general we should prefer @Rule over @ClassRule -- a clean environment for each test is more important than shaving a few seconds off by starting one server for all tests. geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java Line 37 (original), 36 (patched) <https://reviews.apache.org/r/58388/#comment244966> Why is not ok to use @ClassRule here but it's ok in the other tests? I'd much prefer to see a consistent pattern applied. geode-core/src/test/java/org/apache/geode/test/dunit/rules/MemberStarterRule.java Lines 110 (patched) <https://reviews.apache.org/r/58388/#comment244967> I really like this change! Or as Ken and you said "withAutoStart" would be ok too. - Kirk Lund On April 12, 2017, 11:32 p.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58388/ > ----------------------------------------------------------- > > (Updated April 12, 2017, 11:32 p.m.) > > > Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > ------- > > Finally the reviewboard is behaving now. This is good for review now. I will > close the PR. > > * consolidated the two sets of rules. > * do not allow member start up at test initialization time. > * validate properties in @Before > * use provider in the chained rules to get the appropriate ports in @Before > > Jared and I tried to use ServerLauncher/LocatorLauncher to start the > respective member in the rules, but precheckin yields many failures. Looks > like it has two problems: > 1) by passing in the workingDir in the launcher alone does not make all the > logs go there. I still have to set the user.dir env variable to make some > tests pass. > 2) launcher.stop does not stop the cache/locator/server cleanly. Subsequent > tests fail due to insufficient cleanup. > Due to the above two reasons, I reverted back to use the old way to start > server/locator. This looks like a lean and mean way to get what we needed. > > We also investigated the use of Builder in the rules. At least for now, it > doesn't buy us much since we need to do validation/startup servers in @Before > of the rules. And it yeilds some duplication code and make the usage not that > intuitive. > > As for using AvailablePort.Keeper, Jared and I found out it doesn't really > keep the ports, so revert that back as well. > > > Diffs > ----- > > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityIntegrationTest.java > cbd83e382116dcfc06b0199b69bf91486be22f06 > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityPostProcessorTest.java > e93561c0b5fc4fa7f89c03ff4055515d6bbb21be > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityWithSSLTest.java > 14113c06532d42833ee67f4fbf97ba24d535f2d3 > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestServersJUnitTest.java > 2a3a036782fbde22f8969337973ca5794a7172b0 > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/SwaggerVerificationTest.java > a8ab19c0ac7f5cb94148fff725093c5f07858e93 > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/EmbeddedPulseRule.java > PRE-CREATION > > geode-assembly/src/test/java/org/apache/geode/test/dunit/rules/HttpClientRule.java > 49351d9bba854eef08c20d47b99eb3ce467cabaa > > geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseDataExportTest.java > 10ca43b8fc171f75887519deb72700f464f24149 > > geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseVerificationTest.java > 5a300a15b06b948603b5e82acba1190e1905b41a > > geode-core/src/test/java/org/apache/geode/cache/ConnectionPoolDUnitTest.java > 2ac5120a673ed53f7a8f4b1ed077902566e7865d > geode-core/src/test/java/org/apache/geode/cache/ProxyJUnitTest.java > 05228addb74b070a4eac13bd61e84b055916a503 > > geode-core/src/test/java/org/apache/geode/cache/partition/PartitionRegionHelperDUnitTest.java > c57ce5beebc7bc4e4d7b826d8795d5a8e367f245 > > geode-core/src/test/java/org/apache/geode/cache/query/BaseLineAndCompareQueryPerfJUnitTest.java > de2f8d3e159ed199c1bd4b740ce7dd8ed5b82a5f > > geode-core/src/test/java/org/apache/geode/cache/query/dunit/QueryIndexDUnitTest.java > d121fe9993e13e07dcea4817a173b80a8342e169 > > geode-core/src/test/java/org/apache/geode/cache/query/dunit/RemoteQueryDUnitTest.java > 5f48dae54cd14beb2affeae8a8f3387f3e7bfae0 > > geode-core/src/test/java/org/apache/geode/cache/query/functional/IndexWithSngleFrmAndMultCondQryJUnitTest.java > d5195a6b2c8f1b62955ddd127e28ee74bc63ab21 > > geode-core/src/test/java/org/apache/geode/cache/query/functional/LimitClauseJUnitTest.java > 1661cc8d5ba42b782e08251338605c001c526c97 > > geode-core/src/test/java/org/apache/geode/cache/query/internal/QueryUtilsJUnitTest.java > 85305560669544d5fb3493a88f7bf93d86b8ac91 > > geode-core/src/test/java/org/apache/geode/cache/query/internal/index/IndexMaintenanceJUnitTest.java > b7b590ac2918e3176f2aa9625f4bbe7d06113c87 > > geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRQueryDUnitHelper.java > e07990d88061d174f2a7a69e4158abb1ed3ac679 > geode-core/src/test/java/org/apache/geode/cache30/CacheLoaderTestCase.java > 164221776bcb187fc5d2d1fe199d40cfd37a0c63 > geode-core/src/test/java/org/apache/geode/cache30/CacheXml66DUnitTest.java > 738ef3f778da58375d495bb1ad294f07161133db > geode-core/src/test/java/org/apache/geode/cache30/DiskRegionDUnitTest.java > c3553b7ece3b91ce142ad5e0acd4ecc1845e9869 > geode-core/src/test/java/org/apache/geode/cache30/DiskRegionTestImpl.java > 301c23267995dd0201566bac9472d4b7db3c60fd > > geode-core/src/test/java/org/apache/geode/cache30/DistributedAckRegionCCEDUnitTest.java > dcb6cf37a5458e40d4638b944c1394fb63a52da2 > > geode-core/src/test/java/org/apache/geode/cache30/DistributedMulticastRegionDUnitTest.java > f22886b7a9b5c89d2023ef5cafbb1332f1fe08b3 > > geode-core/src/test/java/org/apache/geode/cache30/GlobalRegionCCEDUnitTest.java > 6c3f952581cdc2760f6d07ae5e8b7355bc9583c6 > > geode-core/src/test/java/org/apache/geode/cache30/MultiVMRegionTestCase.java > f57bc96f4410ce9925b28370cd125237fbc5a13d > geode-core/src/test/java/org/apache/geode/cache30/RegionTestCase.java > c4cca348cb183ed0613b9a863eea5b5daa7579ab > > geode-core/src/test/java/org/apache/geode/distributed/internal/Bug40751DUnitTest.java > 008c03e8d66332725d7b7b28dfe144c672014f7f > geode-core/src/test/java/org/apache/geode/disttx/DistTXDebugDUnitTest.java > 7415f4070eedfd8ca505dfe8fcc97f8cbc63a616 > > geode-core/src/test/java/org/apache/geode/internal/cache/CacheAdvisorDUnitTest.java > 6ac85d9d0d829d45a67d01649d791942736245aa > > geode-core/src/test/java/org/apache/geode/internal/cache/DeltaSizingDUnitTest.java > a879b774a1f670a057b0ad4f37b93151f162ec6b > > geode-core/src/test/java/org/apache/geode/internal/cache/EventTrackerDUnitTest.java > 219949bc7577cc1a0a507f97b2983e6a6863fe21 > geode-core/src/test/java/org/apache/geode/internal/cache/PRTXJUnitTest.java > 1bca936ea40529873cc3839f21438dbcf2633c6a > > geode-core/src/test/java/org/apache/geode/internal/cache/PartitionListenerDUnitTest.java > 9c389483df3bd31e4bbbe8ea1cc1de0729ff55d3 > > geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionHADUnitTest.java > 2b3a5a95ded7758be50eb9a74cfc96e31a8929f8 > > geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionRedundancyZoneDUnitTest.java > 404bf27720397962761694e82cf1702c81f0fc2f > > geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionStatsDUnitTest.java > ce09d11f49fb0e825dace186bbd5ebff57ac7cd7 > > geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionTestUtilsDUnitTest.java > 358bece025100dc379a2a89bb091fe94ec3f90b3 > > geode-core/src/test/java/org/apache/geode/internal/cache/control/RebalanceOperationDUnitTest.java > 694fc61699a207736df4976517aab9c2db0625fa > > geode-core/src/test/java/org/apache/geode/internal/cache/ha/HARQueueNewImplDUnitTest.java > 83f3b3f2fa0509e2d224dc2e15b485221ccef9ab > > geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/Bug39356DUnitTest.java > 1d412914d67e0060a8ec73a1fe44fe71d3aaa0a7 > > geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/ShutdownAllDUnitTest.java > 2cdd23bd3a81581fd3dca82b96ecd3e21a0b5e91 > > geode-core/src/test/java/org/apache/geode/internal/cache/rollingupgrade/RollingUpgrade2DUnitTest.java > 6d964716a4074e6e328ca0eedc13323fed3db7bc > > geode-core/src/test/java/org/apache/geode/internal/cache/rollingupgrade/RollingUpgradeDUnitTest.java > 0fffb130c72db5374dc9ea67e88588222fbaf2a2 > > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/Bug36829DUnitTest.java > d90d41d2ae6f16cdfd4868376fd238125d5532bb > > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/DataSerializerPropogationDUnitTest.java > 4a67e384c16a140198e873b2acc329a2141556b7 > > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/HAStartupAndFailoverDUnitTest.java > 6aea5096cdd08ec477848366d5b0e274443334d0 > > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/InstantiatorPropagationDUnitTest.java > 698f79525d20db0d6f8a81553405c7c0494c809e > > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/RedundancyLevelTestBase.java > 4a9829869b5d36ead3ea8714354d3ba202e66249 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java > 3c53dfed1c1fbf8a5cb83eaac16f5bfc96d44a0a > > geode-core/src/test/java/org/apache/geode/management/internal/cli/functions/ExportLogsFunctionIntegrationTest.java > af9a1e60cde38e5f358878d86ef8bf7d99beed59 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/shell/GfshMultilineCommandTest.java > 0d33058961930207120bd990453e9a89f5c783aa > > geode-core/src/test/java/org/apache/geode/management/internal/cli/util/LogExporterIntegrationTest.java > a831ac1e649e97cd2cf659def3151e190665b2dc > > geode-core/src/test/java/org/apache/geode/management/internal/security/AccessControlMBeanJUnitTest.java > d992e9351c2075fea9c2a4d0cc03d75573466d2b > > geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanAuthenticationJUnitTest.java > 97436e01076bdde71f76e0b3b9ad71d8b293303b > > geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanAuthorizationJUnitTest.java > c9e35531ac4d8a3221650ef805dffc35124d6e99 > > geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanShiroJUnitTest.java > 91efeace6a5f7adc3b27d959f958807523a8beae > > geode-core/src/test/java/org/apache/geode/management/internal/security/CliCommandsSecurityTest.java > f1321639a392490003a3bc93dfbe3c684b2b9e5a > > geode-core/src/test/java/org/apache/geode/management/internal/security/DataCommandsSecurityTest.java > f6b5a2e04d14e311d5a94a9c8c2e45b1383a3db5 > > geode-core/src/test/java/org/apache/geode/management/internal/security/DeployCommandsSecurityTest.java > b6190320811b282f03368278f5086c0cafad67f0 > > geode-core/src/test/java/org/apache/geode/management/internal/security/DiskStoreMXBeanSecurityJUnitTest.java > feb00d4507dc37df0d7bcc3b4767a6e77b367b89 > > geode-core/src/test/java/org/apache/geode/management/internal/security/GatewayReceiverMBeanSecurityTest.java > e7fe1d6b1a51466e3570ba51667848f9a30b3dca > > geode-core/src/test/java/org/apache/geode/management/internal/security/GatewaySenderMBeanSecurityTest.java > 67cb1ccefd9800c7d5e979fcee7b00a90f499a43 > > geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsPostProcessorTest.java > 6f2754f46c703074661b89a8005531c5c0a00b43 > > geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java > 3fe83144077d829473dbe48c555ab3be5bf67550 > > geode-core/src/test/java/org/apache/geode/management/internal/security/JavaRmiServerNameTest.java > 66e2702d5a10ba7b7ba5aa4652476a58da91c8c8 > > geode-core/src/test/java/org/apache/geode/management/internal/security/LockServiceMBeanAuthorizationJUnitTest.java > d2f0cae36d6bd8bb00615275942d764bcf6d7a8f > > geode-core/src/test/java/org/apache/geode/management/internal/security/MBeanSecurityJUnitTest.java > 9dcba9465b64f55a190494cc405b6c073228a707 > > geode-core/src/test/java/org/apache/geode/management/internal/security/ManagerMBeanAuthorizationJUnitTest.java > b7447c9216a401ea44e1624407ec86678265d739 > > geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java > 11e202637817d8718ec42ea96730c695974cb907 > geode-core/src/test/java/org/apache/geode/security/ClientAuthDUnitTest.java > 76efae50f7e5d01668481c672e1282ba2e3aa038 > > geode-core/src/test/java/org/apache/geode/security/ClientContainsKeyAuthDUnitTest.java > 0366673df77d7db70bc9370f478adace4315a686 > > geode-core/src/test/java/org/apache/geode/security/ClientDestroyInvalidateAuthDUnitTest.java > 841d98eba9d531d881eb641d5bbff3f7d83c0e42 > > geode-core/src/test/java/org/apache/geode/security/ClientDestroyRegionAuthDUnitTest.java > 5ea65d80be6eeceffeba4832de8ea84a57291a96 > > geode-core/src/test/java/org/apache/geode/security/ClientExecuteFunctionAuthDUnitTest.java > 2b4c27c335346a88fbbd1622d7975c21dfbfae56 > > geode-core/src/test/java/org/apache/geode/security/ClientExecuteRegionFunctionAuthDUnitTest.java > a018f4ccaa8d31765cedff9bd167af734345f68c > > geode-core/src/test/java/org/apache/geode/security/ClientGetAllAuthDUnitTest.java > 92eda186ea609c41f7942464ccc7c7628a742d6b > > geode-core/src/test/java/org/apache/geode/security/ClientGetEntryAuthDUnitTest.java > fad77f59fd46c2051a340f61c2468f2b8661ea46 > > geode-core/src/test/java/org/apache/geode/security/ClientGetPutAuthDUnitTest.java > a816a125b73bc55761208c0e9a2d0724c42d7eb7 > > geode-core/src/test/java/org/apache/geode/security/ClientRegionClearAuthDUnitTest.java > 99a77b679bd23e7b567dcc2905bbd66cd89a5000 > > geode-core/src/test/java/org/apache/geode/security/ClientRegisterInterestAuthDUnitTest.java > 9c24d42f2998c82d10b1073fb8b407e8131efec8 > > geode-core/src/test/java/org/apache/geode/security/ClientRemoveAllAuthDUnitTest.java > 44887d974f1309732fcc64030e6c147eeccc674e > > geode-core/src/test/java/org/apache/geode/security/ClientUnregisterInterestAuthDUnitTest.java > e62aa6ab7d36149c69b368256ebe687e5406aeb8 > > geode-core/src/test/java/org/apache/geode/security/NoShowValue1PostProcessorDUnitTest.java > c6b2735a7d488fd802350c8ec62b1d1b95f50084 > > geode-core/src/test/java/org/apache/geode/security/PDXPostProcessorDUnitTest.java > c735313adf1a2c5c0b001ed36712addf48624ecc > > geode-core/src/test/java/org/apache/geode/security/PostProcessorDUnitTest.java > 89a675246536a64b4c0ab5be267e77a12ae8da54 > > geode-core/src/test/java/org/apache/geode/security/SecurityClusterConfigDUnitTest.java > 4f84f7b7fda8968973f043b153085d9e475e4e92 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java > 02bad30e97f0a3550eac0bf00ce0c6833d761dae > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocalLocatorStarterRule.java > 21be585e648b3f25944f542499837727c45f5aa7 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocalServerStarterRule.java > 3106948196b39500a795c106d1cfb14cbc30dd80 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > 7350241538112c9bc1dfa8e4dfc9a82f18de8033 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorStarterBuilder.java > 169d41e0730aa3b4874bf105c9d495db7aba0387 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorStarterRule.java > 097adb787422aa0c7741ddc147a6d1a8c28f6c03 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/MBeanServerConnectionRule.java > 9c6f81e2bdeb5078330cecc41493522feefccd11 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/MemberStarterRule.java > 3fda85a4814f87995e29aea0ce2c64baad86ac7a > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterBuilder.java > 5d8947c7a1a635865cf61b31972df5abf56f8430 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java > fea2e9db1e57727336edd486aeddff884d481c3b > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/test/MemberStarterRuleTest.java > PRE-CREATION > > geode-cq/src/test/java/org/apache/geode/cache/query/dunit/QueryMonitorDUnitTest.java > 49d7c28663c48581c7b92628351d35479e9e92ee > > geode-cq/src/test/java/org/apache/geode/internal/cache/ha/CQListGIIDUnitTest.java > 7a100467ba45a743d2e30de9c4a89ff5318d8fdd > > geode-cq/src/test/java/org/apache/geode/management/CacheServerManagementDUnitTest.java > ae5570f994c26fde82b940ed1fa14809df714acb > geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDUnitTest.java > 68e4c69fd218b9af0b94473c766a3b6063b09d0e > > geode-cq/src/test/java/org/apache/geode/security/CQPDXPostProcessorDUnitTest.java > 86d467107a4d0fe1ede6059bde7e1d95139de505 > > geode-cq/src/test/java/org/apache/geode/security/CQPostProcessorDunitTest.java > 935c48c6b6d2f9c36aff59a1fdc3bdcf6e0a5a2a > > geode-cq/src/test/java/org/apache/geode/security/ClientQueryAuthDUnitTest.java > aea543d4d2a539a2f3f1fa66c897c9ce863a98a1 > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java > 86deb27f8094e8fc2fd5968ffd2705fdb7a6b890 > > geode-web/src/test/java/org/apache/geode/management/internal/security/GfshCommandsOverHttpSecurityTest.java > 7fd839c967314e20897b305270de9b72f27149f3 > > > Diff: https://reviews.apache.org/r/58388/diff/3/ > > > Testing > ------- > > precheckin successful. > > > Thanks, > > Jinmei Liao > >
