> On March 2, 2017, 6:39 p.m., Kirk Lund wrote: > > geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityIntegrationTest.java > > Lines 66 (patched) > > <https://reviews.apache.org/r/57242/diff/1/?file=1653572#file1653572line66> > > > > 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).
Good idea. Will enhance this more in the next round. > On March 2, 2017, 6:39 p.m., Kirk Lund wrote: > > geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseDataExportTest.java > > Line 80 (original), 80 (patched) > > <https://reviews.apache.org/r/57242/diff/1/?file=1653576#file1653576line80> > > > > I would override startLocatorVM to have a flavor that doesn't require > > new Properties. > > ```java > > locator = lsRule.startLocatorVM(0); > > ``` good idea. It was added at one point. > On March 2, 2017, 6:39 p.m., Kirk Lund wrote: > > 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/diff/1/?file=1653578#file1653578line399> > > > > 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). We usually wouldn't need to do this. This is an old tests that is not using LocatorServerStarterRule, and I need a quick a way to start a server in the vm. I will add a note in the test saying this is not a recommended usage. > On March 2, 2017, 6:39 p.m., Kirk Lund wrote: > > geode-core/src/test/java/org/apache/geode/management/internal/security/DeployCommandsSecurityTest.java > > Lines 59 (patched) > > <https://reviews.apache.org/r/57242/diff/1/?file=1653581#file1653581line64> > > > > 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). Good idea. Will consider this in the next round of refactoring. > On March 2, 2017, 6:39 p.m., Kirk Lund wrote: > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorStarterRule.java > > Lines 67 (patched) > > <https://reviews.apache.org/r/57242/diff/1/?file=1653589#file1653589line67> > > > > 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. We were doing that before, but rules inside rules are not behaving correctly. We need to manually call the before and after the rules we used. Will look into this more. > On March 2, 2017, 6:39 p.m., Kirk Lund wrote: > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java > > Lines 76 (patched) > > <https://reviews.apache.org/r/57242/diff/1/?file=1653592#file1653592line76> > > > > 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. Agree, right now, there is no fixed way how how Geode code is handling relative paths. It was a nightmare trying to figure this out, and we do need a consistent way of doing that both in our test code and in our production code. This is only a refactoring of the old LocatorServerStartupRule where it was doing the same thing before (setting user.dir in each of the VMs when starting up locator/server in the VM). I am simply moving it to here so that when using ServerStartRule alone, we have a notion of where its workingdir is for testing purpose. We should file a separate JIRA ticket to do the way you are suggesting. - Jinmei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57242/#review167694 ----------------------------------------------------------- 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 > >
