> 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
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 - Piyush ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55110/#review160317 ----------------------------------------------------------- On Dec. 31, 2016, 10:47 a.m., Piyush Sharma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55110/ > ----------------------------------------------------------- > > (Updated Dec. 31, 2016, 10:47 a.m.) > > > Review request for DataFu. > > > 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 > >
