----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18593/#review36274 -----------------------------------------------------------
bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment67233> But you actually have a very nice testWrapper that has a setup string as an argument. Do you think that same method could be leveraged to make this test order independent? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment67231> Not sure I understand. I see the string " contents" in the assert statement. Where is that being generated from? Do we need that? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hdfs/TestFuseDFS.groovy <https://reviews.apache.org/r/18593/#comment67230> I was more suggesting a test named testCp should test if the copy actually happened - say by diffing the source and destination files. Do you think an assertion on that would make more sense here? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hcfs/TestFuseHCFS.groovy <https://reviews.apache.org/r/18593/#comment67234> Do you think it'd make sense to not always assume that mkdir will pass, since this is a test after all? Would you consider only moving ahead if the exit code of this command is zero? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hcfs/TestFuseHCFS.groovy <https://reviews.apache.org/r/18593/#comment67235> Jay, can you please help me understand what contains("hcfs") assert does here? In particular, what's the relevance of asserting for the presence of word "hcfs" here? Since this is testCd, do you think it would make sense to verify the pwd returned ${testdir}? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hcfs/TestFuseHCFS.groovy <https://reviews.apache.org/r/18593/#comment67236> I still think this error is misleading. The exit code for cdh && pwd is 0. Not cd, nor pwd alone. Also, the error string is printed when the assertion fails. Do you think the string should be "exit code is non-zero" instead? Reference: http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals(java.lang.String, long, long) Given the above, what do you think about "Non-zero exit code returned in testCd()" as the error string argument? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hcfs/TestFuseHCFS.groovy <https://reviews.apache.org/r/18593/#comment67238> About taking out the cat test because it was dependent, I still respectfully differ. I think what you have here is a really nicely done re-structuring of the code here. You introduced a testWrapper that takes a setup string as an argument. Instead of just getting rid of test, I'd encourage you to consider re-introducing it back and using your setup string argument to make it non-order dependent. bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hcfs/TestFuseHCFS.groovy <https://reviews.apache.org/r/18593/#comment67237> Do we need to close this? bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hcfs/TestFuseHCFS.groovy <https://reviews.apache.org/r/18593/#comment67232> The string argument is an error message. Can we please word it like an error message instead of a log message? How about something like "verification failed" + ... bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/hcfs/TestFuseHCFS.groovy <https://reviews.apache.org/r/18593/#comment67239> I was saying more along the lines of the test is called testCp because it's testing copying of a file. To do so, would you say testing that the file got copied by comparing the contents would be a good test? How about using diff against the 2 files or comparing that their checksums are equal? Just because a file exists and ls confirms it, doesn't mean that the copy succeeded. - Mark Grover On March 5, 2014, 2:33 a.m., jay vyas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18593/ > ----------------------------------------------------------- > > (Updated March 5, 2014, 2:33 a.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/hcfs/TestFuseHCFS.groovy > PRE-CREATION > > 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 > >
