----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18593/#review36080 -----------------------------------------------------------
bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66992> Does it make more sense to do a isMountable variable that gets set if the HCFS_IMPLEMENTATION is one of those in a list of implementations? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66973> Dead code, please take it out. bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66995> Let's only go further if this succeeds i.e. check the exit code. bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66974> Same thing here, let's take out dead code. bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66996> Sorry, I don't get this contains("hcfs") assert. Shouldn't you be verifying that the pwd returned ${testdir}? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66979> This error is misleading since you checked for a cd and pwd, so please correct it. The error could be in either cd or pwd. bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66997> Perhaps, a longer name would be better? "a" can match anything. How about something like "non-trivial-file-name"? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66988> This doesn't fall on you, but I agree with Cos that we should use logger instead of println. If you have cycles, this would be great to fix. bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66998> What do you think of contains "${testdir}/dir1" instead? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66989> Why take out a test? What if someone else's code somewhere uses this test? Is this something that just applies to HDFS and not to other HCFSs? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66999> Do we need to close this? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment67000> Please no all caps:-) bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment67002> Where is "contents" coming from? Is it guaranteed to be there across file systems? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66991> Is this guaranteed? I haven't read the cp spec but I would think the only guaranteed thing would be the return code. A better assert here would be to check if the destination exists and diff it with the source. Checking strings from output is a pretty hard to maintain and arguably a very strict assert. bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment66987> Spelling error (nitpick) - Mark Grover On Feb. 27, 2014, 11:47 p.m., jay vyas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18593/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2014, 11:47 p.m.) > > > Review request for bigtop. > > > Repository: bigtop > > > Description > ------- > > this patch makes bigtop fuse tests awesome :) > > > Diffs > ----- > > > bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy > c42fb33 > > Diff: https://reviews.apache.org/r/18593/diff/ > > > Testing > ------- > > tested in a pseudodistributed VM. works. > > > Thanks, > > jay vyas > >
