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