datafu-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Piyush Sharma <ps26...@gmail.com>
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.
Date Tue, 17 Jan 2017 21:50:53 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message