[GitHub] commons-lang pull request #376: Test cleanup

[mureinik <gi...@git.apache.org>]

GitHub user mureinik opened a pull request:

    https://github.com/apache/commons-lang/pull/376

    Test cleanup

    Following up after the migration of the test suite to JUnit Jupiter, some test code could be cleaned up to make it clearer and easier to maintain.
    This PR contains several patches around that area:
    
    General fixes:
    - In order to test an expected exception, `assertThrows` was used instead of a cumbersome `if`-`fail`-`catch` construct.
    - In order to fail a test on an unexpected exception, the exception is thrown outside the method, instead of the cumbersome construct of catching it and calling `fail`
    - Redundant `throws` clauses were removed
    - `assertEquals`, `assertNotEquals`, `assertSame`, `assertNotSame`, `assertNull` and `assertNotNull` were used instead of reimplementing them with conditions in `if` statements or `assertTrue`s.
    
    Specific improvements:
    - `ConverstionTest#assertBinaryEquals` was removed in favor of the built-in `Assertions#assertArraysEquals(boolean[], boolean[])`.
    - `LocaleUtilsTest#parseAllLocales` was rewritten as a parameterized test instead of implementing it manually with a loop.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/mureinik/commons-lang test-cleanup

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/commons-lang/pull/376.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #376
    
----
commit c9ee985fce7d14d673d9b6bace824560eb4d40fc
Author: Allon Mureinik <mu...@...>
Date:   2018-10-12T15:12:34Z

    Remove ConversionTest#assertBinaryEquals
    
    JUnit Jupiter's Assertions has an
    assertArraysEuqals(boolean[], boolean[]) method, so there's no longer a
    need for the assertBinaryEquals method.

commit cf44c603b105f154b6b57604fe9abd589b7dbd2b
Author: Allon Mureinik <mu...@...>
Date:   2018-10-12T16:14:01Z

    Make LocaleUtilsTest#parseAllLocales parameterized
    
    This patch converts testParseAllLocales to a @ParameterizedTest instead
    of iterating over the locales inside the method's body.
    This changes allows using standard asserts for each case individually
    instead of having to count failures and print the problematic locales
    to System.out.

commit 15daf92088ad6d8868cd157ca9666721a59ce705
Author: Allon Mureinik <mu...@...>
Date:   2018-10-13T14:16:54Z

    Remove double stop() test in StopWatchTest
    
    StopWatchTest#testBadStates has the same block of code testing
    StopWatch#stop copy-pasted.
    This patch cleans it up by removing one of those blocks.

commit 8507e5c81a8d3fedc655c02c93d4cf9dd4418ff6
Author: Allon Mureinik <mu...@...>
Date:   2018-10-13T14:08:48Z

    Clean up testing of exceptions
    
    Now that the entire project is ported to JUnit Jupiter, there are more
    elegant ways to test for exceptions, which this patch applies
    throughtout the code base.
    
    If throwing an exception is supposed to fail a test, just throwing it
    outside of the method cleans up the code and makes it more elegant,
    instead of catching it and calling Assertions#fail.
    
    If an exception is supposed to be thrown, calling
    Assertions#assertThrows is a more elegant option than calling
    Assertions#fail in the try block and then catching and ignoring the
    expected exception.
    Note that assertThrows uses a lambda block, so the variables inside it
    should be final or effectively final. Reusing variables is a common
    practice in the tests, so where needed new final variables were
    introduced, or the variables used were inlined.

commit 3c6141d401233176d2e424640bffe0369592349e
Author: Allon Mureinik <mu...@...>
Date:   2018-10-13T16:38:01Z

    Use assertTrue/assertFalse instead of reimplementing them
    
    Use assertTrue and assertFalse instead of reimplemeting their logic by
    having an if statement conditionalling call fail.

commit 8ee1a558b821f28313b8b538b5d4b0de1b0e7044
Author: Allon Mureinik <mu...@...>
Date:   2018-10-13T16:49:52Z

    Clean up redundant throws clauses

commit 591bebc111bca4f0681560b70fcc3d20cda40577
Author: Allon Mureinik <mu...@...>
Date:   2018-10-13T17:05:40Z

    Clean up assertions
    
    Use built-in assertion methods provides by
    org.junit.jupiter.api.Assertions instead of reimplementing the same
    logic with assertTrue and assertFalse.
    
    Note that JUnit Jupiter 5.3.1 does not support deltas of 0 for
    assertEquals(double, double, double) and
    assertEquals(float, float, float), so these usages of assertTrue were
    left untouched, and will be addressed when JUnit Jupiter 5.4, that
    addresses this isssue, will be available (see
    https://github.com/junit-team/junit5/pull/1613 for details).

----


---

[GitHub] commons-lang issue #376: Test cleanup

[coveralls <gi...@git.apache.org>]

Github user coveralls commented on the issue:

    https://github.com/apache/commons-lang/pull/376
  
    
    [![Coverage Status](https://coveralls.io/builds/19506146/badge)](https://coveralls.io/builds/19506146)
    
    Coverage decreased (-0.02%) to 95.233% when pulling **142a40b9188b3710a4a15298f82460ab51bc90dd on mureinik:test-cleanup** into **46ea7e5e963b09b63d6a54cddd7fe391d0d5b6f4 on apache:master**.



---

[GitHub] commons-lang issue #376: Test cleanup

[mureinik <gi...@git.apache.org>]

Github user mureinik commented on the issue:

    https://github.com/apache/commons-lang/pull/376
  
    > Looks great, thank you very much! 👍
    > 
    > There are just some checkstyle errors related to unused imports left to fix (see failed travis build).
    
    Missed a couple of unused import statements after I cleaned up the usages of `assertTrue` and `assertFalse`. I removed them, and the build should pass now.


---

[GitHub] commons-lang issue #376: Test cleanup

[PascalSchumacher <gi...@git.apache.org>]

Github user PascalSchumacher commented on the issue:

    https://github.com/apache/commons-lang/pull/376
  
    Looks great, thank you very much! 👍 
    
    There are just some checkstyle errors related to unused imports left to fix (see failed travis build).


---

[GitHub] commons-lang pull request #376: Test cleanup

[asfgit <gi...@git.apache.org>]

Github user asfgit closed the pull request at:

    https://github.com/apache/commons-lang/pull/376


---

[GitHub] commons-lang issue #376: Test cleanup

[PascalSchumacher <gi...@git.apache.org>]

Github user PascalSchumacher commented on the issue:

    https://github.com/apache/commons-lang/pull/376
  
    Thanks! 👍 


---