> On March 2, 2017, 6:44 p.m., Jared Stewart wrote: > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java > > Lines 154 (patched) > > <https://reviews.apache.org/r/57242/diff/1/?file=1653592#file1653592line166> > > > > I don't think that we ought to manually delete the workingDir. If > > someone is using a temporaryFolder (which I think is the normal use case), > > then the deletion will happen automatically. If they aren't using a > > temporary folder, then deleting the workingDir seems dangerous. > > Jinmei Liao wrote: > this is there so that when we are writing test using this rule alone, we > don't need to have the have a temperoaryFolder rule along side with it. The > rule itself handles creating the temeroray folder and delete it afterwards. > The deletion happens in the after() block, so it should be pretty safe. > > Jared Stewart wrote: > As a user of this rule, the following would seem like a reasonable thing > to do: > > ``` > private final File workingDir = new File("."); > > @Rule > ServerStarterRule serverStarterRule = new ServerStarterRule(workingDir); > ``` > > > Yet it would end up deleting part of the user's geode checkout. Perhaps > we should have separate constructors or factory methods for using this rule > alone vs. using it through LocatorServerStarterRule. That way, when used > alone, it can create its own temporary folder so that a user doesn't have to > worry about it. > > Jinmei Liao wrote: > The reason behind this refactoring is that the normal use case is that > user don't need to use a temporaryFolder with this rule. the rule handles the > create/deleting the temp folder for you, like this: > > @Rule > public ServerStarterRule serverStarterRule = new ServerStarterRule(); > > @Test > public void test(){ > Server server = serverStarterRule.startServer(); > File workingDir = server.getWorkingDir(); > // verfify the working dir > } > > And the rule will handle deleting the working dir created afterwards. > Maybe you are suggesting that if user passes in a workingDir, then you are > responsible of deleting it, the rule won't?
Yes, precisely. I think the creator of the directory should be the one responsible for deleting it. - Jared ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57242/#review167712 ----------------------------------------------------------- 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 > >
