> On May 7, 2015, 8:18 a.m., Ajay Yadava wrote: > > common/src/test/java/org/apache/falcon/entity/parser/ClusterEntityParserTest.java, > > line 296 > > <https://reviews.apache.org/r/33867/diff/3/?file=951662#file951662line296> > > > > Seems like cleanupClusterLocations is a good candidate for @AfterMethod.
Minority of the tests create the staging and working dirs. So having cleanupClusterLocations() in AfterMethod seems like overkill. > On May 7, 2015, 8:18 a.m., Ajay Yadava wrote: > > common/src/test/java/org/apache/falcon/entity/parser/ClusterEntityParserTest.java, > > line 416 > > <https://reviews.apache.org/r/33867/diff/3/?file=951662#file951662line416> > > > > Since only one argument is being passed, any particular reason to use > > isNoneEmpty here instead of isNotEmpty? Made change > On May 7, 2015, 8:18 a.m., Ajay Yadava wrote: > > common/src/test/java/org/apache/falcon/entity/parser/ClusterEntityParserTest.java, > > line 423 > > <https://reviews.apache.org/r/33867/diff/3/?file=951662#file951662line423> > > > > Same as Line:408 made change > On May 7, 2015, 8:18 a.m., Ajay Yadava wrote: > > common/src/test/java/org/apache/falcon/entity/parser/ClusterEntityParserTest.java, > > line 435 > > <https://reviews.apache.org/r/33867/diff/3/?file=951662#file951662line435> > > > > This method can be made more robust and generic by checking for both > > staging and working path so that tests for staging directory alone can also > > use it. made the change > On May 7, 2015, 8:18 a.m., Ajay Yadava wrote: > > common/src/test/java/org/apache/falcon/entity/parser/ClusterEntityParserTest.java, > > line 289 > > <https://reviews.apache.org/r/33867/diff/3/?file=951662#file951662line289> > > > > If the purpose of the catch block is to validate the error message then > > it can be done in better manner by leveraging > > expectedExceptionMessageRegExp parameter. You can pass the message string > > or a regular expression. > > > > > > http://testng.org/javadoc/org/testng/annotations/Test.html#expectedExceptionsMessageRegExp() > > > > Doing exact string match can cause unwanted coupling of the error > > messages in tests and source. True, I made the necessary change. - Balu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33867/#review82807 ----------------------------------------------------------- On May 6, 2015, 10:38 p.m., Balu Vellanki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33867/ > ----------------------------------------------------------- > > (Updated May 6, 2015, 10:38 p.m.) > > > Review request for Falcon, Sowmya Ramesh and Venkat Ranganathan. > > > Bugs: Falcon-1195 > https://issues.apache.org/jira/browse/Falcon-1195 > > > Repository: falcon-git > > > Description > ------- > > testClusterWithOnlyStaging fails intermittently due to race condition in > creating working dir for cluster. This can be fixed by creating a different > working/staging dir for each test case. > > > Diffs > ----- > > > common/src/main/java/org/apache/falcon/entity/parser/ClusterEntityParser.java > 4555cb0 > > common/src/test/java/org/apache/falcon/entity/parser/ClusterEntityParserTest.java > 4920d03 > > Diff: https://reviews.apache.org/r/33867/diff/ > > > Testing > ------- > > Ran unit tests multiple times to verify the changes. > > > Thanks, > > Balu Vellanki > >
