-----------------------------------------------------------
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
> 
>

Reply via email to