+1 to not modifying user.dir
We should remove the unit test Rules that are doing this.
On 1/22/18 10:00 AM, Kirk Lund wrote:
I started filing subtasks for GEODE-4180, but I came to the conclusion that
I don't really want to be involved in this. I'd rather see tests be changed
to not mutate "user.dir". I think the test authors should find a different
way to test Geode.
I've renamed the test summary:
GEODE-4180: Tests should not mutate system property "user.dir"
I'm not planning to work on this bug any further. If anyone else feels like
taking it up, please go ahead.
For the tests that I'm involved with, I'll be modifying DUnit test
framework to fork new JVMs in targeted user dirs. I think that's a much
better strategy for end-to-end cluster tests.
Thanks,
Kirk
On Tue, Jan 9, 2018 at 8:02 AM, Kirk Lund <kl...@apache.org> wrote:
Looks like everyone supports this change. I’m going to change the Jira
ticket into an epic and create a few sub-tasks. I’ll also specify going
through some centralized mechanism that determines the parent dir.
Thanks for all the input!
On Mon, Jan 8, 2018 at 11:47 AM Darrel Schneider <dschnei...@pivotal.io>
wrote:
Should geode have a single method it uses to create File instances?
That way the code that determines the parent dir could be in one place.
On Mon, Jan 8, 2018 at 11:26 AM, Patrick Rhomberg <prhomb...@pivotal.io>
wrote:
The ClusterStartupRule, LocatorStarterRule, and ServerStarterRule rules
all
have withTempWorkingDir methods that should take care of this, as well.
These temporary directories should be removed as part of the rules
@After
invocation.
On Mon, Jan 8, 2018 at 10:23 AM, Jinmei Liao <jil...@pivotal.io> wrote:
+1 for always using absolute path in our product code.
Also the server/locator launchers should be able to take a
working-dir as
parameter to store all the stat/logs/config files.
On Fri, Jan 5, 2018 at 3:49 PM, Kirk Lund <kl...@apache.org> wrote:
The from should be:
this.viewFile = new File("locator" + server.getPort() +
"view.dat");
On Fri, Jan 5, 2018 at 3:48 PM, Kirk Lund <kl...@apache.org> wrote:
Just to help facilitate the discussion, here's a pull request that
changes
GMSLocator from:
this.viewFile = new File(System.getProperty("user.dir"),
"locator" +
server.getPort() + "view.dat");
...to:
this.viewFile = new File(System.getProperty("user.dir"),
"locator" +
server.getPort() + "view.dat");
To allow the new test LocatorViewFileTemporaryFolderDUnitTest to
redirect
the locator view dat file to a JUnit TemporaryFolder.
The only other way I can think of to this is to introduce a new
Geode
property for "current-directory" which a test could specify. That
property
value would have to be pushed down to every class that creates a
new
File.
Pull request: https://github.com/apache/geode/pull/1243
On Fri, Jan 5, 2018 at 3:32 PM, Kirk Lund <kl...@apache.org>
wrote:
Any calls such as:
File file = new File("myfile");
...results in creating a file in the current working directory of
IntelliJ or Gradle when executing the test.
I previously made a change to Gfsh so that tests can pass in a
parent
directory which will be used instead. This allowed me to change
all
of
the
Gfsh tests to write to a JUnit TemporaryFolder.
This allows us to clean up ALL file artifacts produced from a
test
and
also allows us to avoid file-based pollution from one test to
another.
I'd like to propose that we either always pass a parent directory
into a
component that produces file artifacts or change all of our code
from
using
the constructor File(String pathname) to using the constructor
File(String
parent, String child).
That 2nd approach would involve changing lines like this:
File file = new File("myfile");
...to:
File file = new File(System.getProperty("user.dir"),
"myfile");
Then if you add this line to a test:
System.setProperty("user.dir", temporaryFolder.getRoot().getA
bsolutePath());
...you're able to redirect all file artifacts to the JUnit
TemporaryFolder.
If we don't make this change to product code, then I really don't
think
we should be manipulating "user.dir" in ANY of our tests because
the
results are rather broken.
If we don't like using "user.dir" then we could devise our own
Geode
system property for "the current working directory." Honestly, I
don't
care
what system property we use or don't use, but I really want to
have
ALL
file artifacts properly cleaned and deleted after every test.
And I
really
want to prevent file-based test pollution.
--
Cheers
Jinmei
