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
>> > >
>> >
>>
>
