> On Feb. 8, 2017, 7:49 p.m., Kirk Lund wrote: > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java, > > line 130 > > <https://reviews.apache.org/r/56425/diff/1/?file=1627169#file1627169line130> > > > > I didn't notice use of "gemfire.home" before but given this is geode > > code, we shouldn't use "gemfire.home" > > > > I thought Anthony and Dan had worked to remove all env vars and uses of > > gemfire.home or geode.home from our tests and code. > > Anthony Baker wrote: > I have no recollection of this :-) > > The only uses of `gemfire.home` are here: > > ``` > ~/code/incubator-geode (release/1.1.0)$ git grep -i gemfire.home > > geode-assembly/src/test/java/org/apache/geode/management/internal/AgentUtilJUnitTest.java: > // GEODE-958: We need to set gemfire.home to tell AgentUtil where to find > wars in case the env > > geode-assembly/src/test/java/org/apache/geode/management/internal/AgentUtilJUnitTest.java: > System.out.println("Setting gemfire.home to " + installDir); > > geode-assembly/src/test/java/org/apache/geode/management/internal/AgentUtilJUnitTest.java: > System.setProperty("gemfire.home", installDir.toString()); > > geode-core/src/main/java/org/apache/geode/management/internal/AgentUtil.java: > logger.info("Reading gemfire.home System Property -> {}", geodeHome); > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java: > // Setting gemfire.home to this value allows locators started by the rule > to run Pulse > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java: > System.out.println("Setting gemfire.home to " + geodeInstallDir); > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java: > System.setProperty("gemfire.home", geodeInstallDir); > ```
We did move all of the code that looked for files in System.getProperty(JTESTS) look up resources from the classpath instead. See TestUtils.getResourcePath. I don't think we did anything with gemfire.home. > On Feb. 8, 2017, 7:49 p.m., Kirk Lund wrote: > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java, > > line 172 > > <https://reviews.apache.org/r/56425/diff/1/?file=1627169#file1627169line172> > > > > Another use of setting "gemfire.home" system property. I don't know > > what the correct approach is but I think this is wrong. > > Jared Stewart wrote: > I think the correct thing to do is to let Gradle set GEODE_HOME for the > tests to pick up. > > We do this already apparently, although the value we set for GEODE_HOME > looks wrong to me. > > ``` > tasks.withType(Test){ > environment 'GEODE_HOME', > "$buildDir/install/${distributions.main.baseName}/lib" > } > ``` > > I would expect GEODE_HOME to not have the "/lib" portion on the end, and > AgentUtil (which finds the pulse war) is expecting the same thing. I talked with Kirk about this. Here's my 2 cents: I think anything that depends on the assembled product shouldn't be in geode-core. So this LocatorStartupRule definitely should not be in the business of fishing around on the filesystem for an assembled product. I think the tests in geode-assembly should have a standardized way of using the installed product. It looks like that probably is that GEODE_HOME environment variable. If that thing is set to the wrong through gradle, we should probably fix it. - Dan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56425/#review164751 ----------------------------------------------------------- On Feb. 8, 2017, 3:03 a.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56425/ > ----------------------------------------------------------- > > (Updated Feb. 8, 2017, 3:03 a.m.) > > > Review request for geode, Jared Stewart, Kevin Duling, and Kirk Lund. > > > Repository: geode > > > Description > ------- > > GEODE-2272: do not use a new method to start locator with pulse > > > Diffs > ----- > > > geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseDataExportTest.java > b5472909eec8d5ca124e7bfd5c6cb71864d9bbee > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > 1f0cd9e720e732c2ce06515c16601e1df173ff4f > > Diff: https://reviews.apache.org/r/56425/diff/ > > > Testing > ------- > > > Thanks, > > Jinmei Liao > >
