----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52911/#review152835 -----------------------------------------------------------
sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestHdfsOperations.java (line 65) <https://reviews.apache.org/r/52911/#comment221957> U"rkorszak :D sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestHdfsOperations.java (lines 103 - 114) <https://reviews.apache.org/r/52911/#comment221956> Nagyon tetszik a BDD megkozelites! Mindenesetre a @Before-t raraknam, es az osszes @Test method-bol kivennem. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (lines 206 - 207) <https://reviews.apache.org/r/52911/#comment221962> Either rename `e` to `expected`, or use `@Rule ExpectedException`, but drop that comment in any case. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 211) <https://reviews.apache.org/r/52911/#comment221973> Not needed w/ `@Rule ExpectedException`. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (lines 224 - 225) <https://reviews.apache.org/r/52911/#comment221969> Either rename `e` to `expected`, or use `@Rule ExpectedException`, but drop that comment in any case. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 229) <https://reviews.apache.org/r/52911/#comment221976> Not needed w/ `@Rule ExpectedException`. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (lines 314 - 315) <https://reviews.apache.org/r/52911/#comment221964> Either rename `e` to `expected`, or use `@Rule ExpectedException`, but drop the comment in any case. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 319) <https://reviews.apache.org/r/52911/#comment221974> Not needed w/ `@Rule ExpectedException`. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (lines 334 - 335) <https://reviews.apache.org/r/52911/#comment221965> Either rename `e` to `expected`, or use `@Rule ExpectedException`, but drop the comment in any case. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 339) <https://reviews.apache.org/r/52911/#comment221975> Not needed w/ `@Rule ExpectedException`. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 341) <https://reviews.apache.org/r/52911/#comment221990> http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertFalse(java.lang.String, boolean) sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (lines 346 - 347) <https://reviews.apache.org/r/52911/#comment221960> Let's call it `configureMocksForHappyPath()` and drop the comment line, then. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 369) <https://reviews.apache.org/r/52911/#comment221961> I'd extract method and call 4 times. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 388) <https://reviews.apache.org/r/52911/#comment221967> Either use `OozieActionResult` instead of `boolean`, or use always variables where this method is called like that: `final boolean background = true; assertSuccessfulExecution(background);` or `final OozieActionResult resultOnNotification = OozieActionResult.RUNNING; assertSuccessfulExecution(resultOnNotification);` sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 401) <https://reviews.apache.org/r/52911/#comment221968> This can also be put simpler if you go for using `OozieActionResult` instead of `boolean`. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 405) <https://reviews.apache.org/r/52911/#comment221971> Rename to `assertActionOutputDataPresentAndCorrect()`. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (lines 412 - 415) <https://reviews.apache.org/r/52911/#comment221979> Use instead> http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals(java.lang.String, java.lang.Object, java.lang.Object) sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 418) <https://reviews.apache.org/r/52911/#comment221972> Rename to `assertActionOutputDataNotPresent()`. sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (lines 425 - 428) <https://reviews.apache.org/r/52911/#comment221980> Instead: http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertNull(java.lang.String, java.lang.Object) sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 431) <https://reviews.apache.org/r/52911/#comment221970> Way too many parameters here... have you considered an own `@Rule` that `extends ExpectedException` or uses that? sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 443) <https://reviews.apache.org/r/52911/#comment221985> http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertNotNull(java.lang.String, java.lang.Object) sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 444) <https://reviews.apache.org/r/52911/#comment221988> http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertTrue(java.lang.String, boolean) sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 446) <https://reviews.apache.org/r/52911/#comment221981> Instead: http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertNull(java.lang.String, java.lang.Object) sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 451) <https://reviews.apache.org/r/52911/#comment221986> http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertNotNull(java.lang.String, java.lang.Object) sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 452) <https://reviews.apache.org/r/52911/#comment221989> http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertTrue(java.lang.String, boolean) sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 454) <https://reviews.apache.org/r/52911/#comment221982> http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertNull(java.lang.String, java.lang.Object) sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 459) <https://reviews.apache.org/r/52911/#comment221987> http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertNotNull(java.lang.String, java.lang.Object) sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 461) <https://reviews.apache.org/r/52911/#comment221983> http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertNull(java.lang.String, java.lang.Object) sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java (line 475) <https://reviews.apache.org/r/52911/#comment221984> http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertNull(java.lang.String, java.lang.Object) - András Piros On Oct. 15, 2016, 7:16 p.m., Peter Bacsko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52911/ > ----------------------------------------------------------- > > (Updated Oct. 15, 2016, 7:16 p.m.) > > > Review request for oozie, András Piros, Attila Sasvari, Peter Cseh, and > Robert Kanter. > > > Repository: oozie-git > > > Description > ------- > > New unit tests for LauncherAM class. > > These tests already provide good coverage of the class (around ~80%). Some > code paths are missing but the most important scenarios are covered. > > > Diffs > ----- > > > sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/LauncherAMTestMainClass.java > PRE-CREATION > > sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestHdfsOperations.java > PRE-CREATION > > sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52911/diff/ > > > Testing > ------- > > > Thanks, > > Peter Bacsko > >
