geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ken Howe <kh...@pivotal.io>
Subject Re: Review Request 58388: GEODE-2730: refactor rules
Date Wed, 12 Apr 2017 21:34:35 GMT


> On April 12, 2017, 9:10 p.m., Ken Howe wrote:
> > geode-core/src/test/java/org/apache/geode/test/dunit/rules/MemberStarterRule.java
> > Lines 110 (patched)
> > <https://reviews.apache.org/r/58388/diff/2/?file=1690839#file1690839line110>
> >
> >     I find this method name confusing because the member it just sets a boolean
to start (or not) later in the before() method. My interpretation is that it's an "auto start"
flag for each test.
> 
> Jinmei Liao wrote:
>     probably renamed to autoStart()?
> 
> Jared Stewart wrote:
>     Maybe startAutomatically()

That makes it more readable to me. I made the comment to see if there's consensus or strong
feelings either way. If the method name is changed, then proably also want to rename the boolean
as well.


- Ken


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58388/#review171788
-----------------------------------------------------------


On April 12, 2017, 3:08 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, 3:08 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> * 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
9a39298a21a4593e8939ddfcac11664508d20df3 
>   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
381b7515c48b82cdd231d54323b0b6db3228fc41 
>   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/2/
> 
> 
> Testing
> -------
> 
> precheckin successful.
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message