Mailing-List: contact dev-help@datafu.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@datafu.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============1804233032255498643=="
MIME-Version: 1.0
Subject: Re: Review Request 55110: DATAFU-106 Test files are currently created
 in the
 subdirectory folder (e.g. datafu-pig/input*). For better organization,
 they should be created in a subdirectory.
From: Piyush  Sharma <ps26oct@gmail.com>
To: Matthew Hayes <matthew.terence.hayes@gmail.com>
Cc: DataFu <dev@datafu.incubator.apache.org>,
 Piyush  Sharma <ps26oct@gmail.com>, Eyal Allweil <eyal_allweil@yahoo.com>
Date: Tue, 17 Jan 2017 21:50:53 -0000
Message-ID: <20170117215053.28415.3611@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: Piyush  Sharma <noreply@reviews.apache.org>
References: <20170102092943.5940.12327@reviews.apache.org>
In-Reply-To: <20170102092943.5940.12327@reviews.apache.org>
Reply-To: Piyush  Sharma <ps26oct@gmail.com>
archived-at: Tue, 17 Jan 2017 21:50:59 -0000

--===============1804233032255498643==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On Jan. 2, 2017, 9:29 a.m., Eyal Allweil wrote:
> > Hi Piyush,
> > 
> > Thank you for your patch! It looks to me that it works fine - I ran our tests on Ubuntu, both from Eclipse and from the command line.
> > 
> > I have two comments, one "real" and one just a typo:
> > 
> > 1) From [this question](http://stackoverflow.com/a/840229/150992), it appears that changing the *user.dir* property in this way can be unpredictable - that it won't affect FileOutputStreams, for example. So although this works for our current tests (the FileOutputStream used in datafu.pig.linkanalysis.PageRank uses *File.createTempFile* instead of the working dir, so it's fine) I worry that this might not work in the future. Maybe add a comment about this in the *beforeClass* method?
> > 
> > 2) "Location" is spelled as "loaction" in PigTests.java
> > 
> > Cheers,
> > Eyal
> 
> Piyush  Sharma wrote:
>     Hi Eyal,
>     
>     Thank you for reviewing it.
>     
>     1) I've actually tried doing it without changing user.dir. The pigtests don't actually work that way (for eg: LOAD 'input'... searches for the file in the working directory and it apparently deciphers it to be the same as that of the jvm). Hence, this was the only possible way to solve the issue that I could think of. 
>     
>     I also went through that stackoverflow question whilst creating the patch to resolve my suspicions about the same. Actually, as even the accepted answer says it does affect the subsequent file creations, which happens via a call to writeLinesToFile defined in the PigTests.java which always creates a new file, deleting the existing one if necessary, and then writes to it. Now since writeLinesToFile also implicitly uses FileOutputStream (via a FileWriter object) to write to files just created in the new user.dir, it seemed like changing the working directory did actually affect the FileOutPutStreams, atleast as far as our project goes it did. Moreover the afterMethod resets the user.dir so hopefully there aren't any consequences at all. 
>     
>     I can add some comment like "//Only some classes reflect the changes in working directory" or something like "//Developers are requested to just work with absoluteFiles and absolutePaths to avoid IOExceptions and such" but I feel it throws up a red flag unnecessarily, so should I ?
>     
>     2) I will surely correct the typo, thank you.
>     
>     Cheerio,
>     Piyush
> 
> Eyal Allweil wrote:
>     I think this is fine - after all, all existing tests pass. One more very minor comment - I think I'd just put the test in the main java/datafu/test dir - I don't think there's a need for a subdirectory for it.

Yeah sure, I've transferred the unit test to the datafu-pig/src/test/java/datafu/test directory and also corrected the typo in PigTests.java in the new patch that I've uploaded.

Cheerio


- Piyush


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55110/#review160317
-----------------------------------------------------------


On Jan. 6, 2017, 8:51 p.m., Piyush  Sharma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55110/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2017, 8:51 p.m.)
> 
> 
> Review request for DataFu and Matthew Hayes.
> 
> 
> Repository: datafu
> 
> 
> Description
> -------
> 
> DATAFU-106: Test files are currently created in the subdirectory folder (e.g. datafu-pig/input*). For better organization, they should be created in a subdirectory. This also makes it easier to exclude them all with gitignore. 
> (issue: https://issues.apache.org/jira/browse/DATAFU-106)
> 
> 
> Diffs
> -----
> 
>   datafu-pig/src/test/java/datafu/test/pig/PigTests.java d1d6fcc 
>   datafu-pig/src/test/java/datafu/test/pig/test_filesSubdir/TestFilesSubdirTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55110/diff/
> 
> 
> Testing
> -------
> 
> unit tests passed.
> 
> 
> Thanks,
> 
> Piyush  Sharma
> 
>


--===============1804233032255498643==--