----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57242/#review167694 -----------------------------------------------------------
geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityIntegrationTest.java Lines 66 (patched) <https://reviews.apache.org/r/57242/#comment239601> I think it would best if you can keep this self-contained as a ClassRule instead of invoking a rule method in BeforeClass: ```java @ClassRule public static ServerStarterRule serverStarter = new ServerStarterRule().startServer(properties); ``` Or (this matches the new pattern than JUnit folks are pushing for complex rules): ```java @ClassRule public static ServerStarterRule serverStarter = ServerStarterRule.builder().startServer(properties).build(); ``` And then change startServer to return this (ServerStarterRule). geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseDataExportTest.java Line 80 (original), 80 (patched) <https://reviews.apache.org/r/57242/#comment239602> I would override startLocatorVM to have a flavor that doesn't require new Properties. ```java locator = lsRule.startLocatorVM(0); ``` geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java Line 399 (original), 399 (patched) <https://reviews.apache.org/r/57242/#comment239603> I know I mentioned it before, but I really don't like seeing a Rule used as not-a-Rule. It would be much cleaner to create a ServerStarter class. Then use ServerStarter directly where you don't need a Rule and use ServerStarterRule only where you need a Rule (ie, when JUnit lifecycle is responsible for invoking before and after instead of invoking directly like this). geode-core/src/test/java/org/apache/geode/management/internal/security/DeployCommandsSecurityTest.java Lines 59 (patched) <https://reviews.apache.org/r/57242/#comment239605> This could all be moved into a builder on the Rule. See org.junit.rules.Timeout and org.apache.geode.management.ManagementTestRule: ```java @ClassRule public static ServerStarterRule serverRule = ServerStarterRule.builder() .setProperty(SECURITY_MANAGER, SimpleTestSecurityManager.class.getName()) .setProperty(JMX_MANAGER_PORT, jmxManagerPort + "") .setProperty(JMX_MANAGER_PORT, jmxManagerPort + "") .startServer() .build(); ``` This is the pattern/style recommended for complex rules (it's not my invention). geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorStarterRule.java Lines 67 (patched) <https://reviews.apache.org/r/57242/#comment239607> Wouldn't it be better to just Chain TemporaryFolder or DistributedTemporaryFolder with this Rule? Or maybe pass TemporaryFolder.getRoot() into the constructor on this Rule, and then let TemporaryFolder handle the before/after for itself. Now we'll have two rules that provide temporary directories. geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java Lines 76 (patched) <https://reviews.apache.org/r/57242/#comment239610> I've this behave differently on different operating systems and misbehave in general. It creates the illusion to the next developer that this system property is mutable, and then it can be nightmare to debug when they use a JDK API that theoretically uses "user.dir" but still ends up using the original "user.dir" -- I would do everything we can to eliminate usage of "user.dir" or back it into a corner (one single line of code in all of Geode). We can alter any Geode code that relies on relative paths to have absolute paths always passed into them. Then this problem goes away. Or at the very least it pushes getProperty("user.dir") into a single method that finds the root directory for the member. Then you can override this or mock it to have it return temporaryFolder.getRoot() instead of getProperty("user.dir"). ```java public class WorkingDirectory { // override or mock me public File getWorkingDirectory() { return System.getProperty("user.dir"); } ``` Or: ```java public class WorkingDirectory { private File dir; public WorkingDirectory(String path) { this.dir = new File(path); } public File getWorkingDirectory() { return this.dir; } ``` Then alter our Geode code to use the above. Immutable system properties and static code is a nightmare in tests. - Kirk Lund On March 2, 2017, 3:30 p.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57242/ > ----------------------------------------------------------- > > (Updated March 2, 2017, 3:30 p.m.) > > > Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, and Kirk > Lund. > > > Repository: geode > > > Description > ------- > > GEODE-2267: Enhance server/locator startup rules to include workingDir > > * This batch is mostly test code change, I am enhancing the > ServerStartupRules and LocatorStartupRules to have a notion of workingDir > itself. > * There are only 2 product code change: one is the assembly's build.gradle to > correctly set the gemfire.home/geode.home, another one is to use an absolute > path to check for File's exsistance. Without that, the tests always fail in > CI pipeline. > > > Diffs > ----- > > geode-assembly/build.gradle 1c95927f19888b15e771b3cbbc92f120787868c4 > > geode-assembly/src/test/java/org/apache/geode/management/internal/AgentUtilJUnitTest.java > 3a29e8a435f5d5362810f5e05ea92a1260e3fbfc > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityIntegrationTest.java > f0b6bb327f0424f2e4afa2e057d8e38bd0ef1b39 > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityPostProcessorTest.java > 160f6349d94646b12e3114da14e5bff569eed5a9 > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestServersJUnitTest.java > 10ce515df8c3630cbf01fc314be454d3e2adfe2c > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/SwaggerVerificationTest.java > c7d0e7341fcf13bf34a597405d2b42b608004215 > > geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseDataExportTest.java > b5472909eec8d5ca124e7bfd5c6cb71864d9bbee > > geode-core/src/main/java/org/apache/geode/distributed/DistributedSystem.java > 20c948f8f16240aef5c8eae313164693125a99ca > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java > 05f2c0cb2115ee574666ed3da943c26e8c38c9f2 > > geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanShiroJUnitTest.java > 3183aafd9907ecb15714c2038f2b8c3af31e6e06 > > geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerStartupRule.java > ea72cfab26f828af9a09f08755a9a7f9ed121654 > > geode-core/src/test/java/org/apache/geode/management/internal/security/DeployCommandsSecurityTest.java > e89bd62b59b8218c30c8f724b4febf2b3ae7721b > > geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsPostProcessorTest.java > 0980c18de1b504f545355a0f7d650f46c92cc670 > > geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java > 2d32905d45496fe69e5dc189cd94b82ea163c3b5 > > geode-core/src/test/java/org/apache/geode/management/internal/security/JavaRmiServerNameTest.java > 2ae61402d330ab6426c0bc0e0193771ce5dc9c70 > > geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorDUnitTest.java > 52afab40990ab3a18fed04fcb001ec9dbe7d045e > > geode-core/src/test/java/org/apache/geode/security/PeerSecurityWithEmbeddedLocatorDUnitTest.java > e87a285d088c923326aa82ee406f543ffc5e0593 > geode-core/src/test/java/org/apache/geode/test/dunit/rules/Locator.java > fe26e8364798689c1ece487554d99085f3f26f23 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > 68447201875ef31a73cfb888d346292e3f084ae8 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorStarterRule.java > c3e493e1c176974ed757c71d5c195ff5964dbb57 > geode-core/src/test/java/org/apache/geode/test/dunit/rules/Member.java > 7cc1eea2e26b176b570111bccdb8914e19528ecf > geode-core/src/test/java/org/apache/geode/test/dunit/rules/Server.java > 4aa2c69389dce4650cf476f4e8decebddfc36736 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java > b5ddee666c52fe39deb015c6e2aa57e5b21e4f20 > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/QueryNamesOverHttpDUnitTest.java > 1a19e5e5ca90c248655eb40e68aca3c7ed858770 > > > Diff: https://reviews.apache.org/r/57242/diff/1/ > > > Testing > ------- > > precheckin successful > > > Thanks, > > Jinmei Liao > >
