BCEL-343 JUnit Assertion improvement #69
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix a typo in FieldAnnotationsTestCase#checkValue: "Didnt" to "Didn't".
Simplify assertTrue calls with negative conditions to assertFalse calls which are easier to understand.
garydgregory
requested changes
Oct 7, 2020
There was a problem hiding this comment.
@mureinik
Thank you for your PR; I just have a few comment (see each file).
src/test/java/org/apache/bcel/generic/GeneratingAnnotatedClassesTestCase.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/bcel/generic/FieldAnnotationsTestCase.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/bcel/generic/FieldAnnotationsTestCase.java
Outdated
Show resolved
Hide resolved
| @@ -81,7 +80,8 @@ private void compare(final JavaClass jc, final InputStream inputStream, final St | |||
| int i = 0; | |||
| for (final int out : baos.toByteArray()) { | |||
| final int in = src.read(); | |||
| assertEquals(in, out & 0xFF, name + ": Mismatch at " + i); | |||
| int j = i; | |||
There was a problem hiding this comment.
Yes - values in a lambda expression must be final or effectively final, and i isn't, so I assigned it to j, which is (since it's scoped in the loop's body and is never modified).
Alternatively, we can leave the message as is and not replace it with a Supplier.
src/test/java/org/apache/bcel/EnclosingMethodAttributeTestCase.java
Outdated
Show resolved
Hide resolved
Clean up assertions code by using the built-in assertions in org.junit.jupiter.api.Assertions instead of reinventing the wheel. assertTrue(x.equals(y)) and assertTrue(primitiveX == primitiveY) are simplified to assertEquals(x, y). The main gain by this patch is cleaning up the code and making it easier to maintain. A side bonus is that many assertions simplified to assertEquals calls had messages that were manually constructed to show the difference between the two values being compared, and these messages were constructed regardless of the assertion success or failure. By using assertEquals, these string concatenations are avoided as long as the assert succeeds.
Clean up assertions code by using the built-in assertions in
org.junit.jupiter.api.Assertions instead of reinventing the wheel.
This patch simplifies the idiom of using an if to explicitly check for
equality:
```
if (!a.equals(b)) {
fail("a should equal b");
}
```
with the build it assertEquals(a, b, "a should equal b").
Replace the idiom of catching an exception and then explicitly calling org.junit.jupiter.api.Assertions with just allowing the error to be thrown from the test method and leaving the it up to the runner to handle the failure.
In the cases that assertions have non-constant failure messages, these messages are replaced with Suppliers that produce them in order to avoid producing the calculated string unconditionally.
mureinik
force-pushed
the
BCEL-343-assertions
branch
from
b469c23
to
721e427
Compare
|
@garydgregory Fixed the formatting issues and answered wrt the usage of |
garydgregory
changed the title
asfgit
pushed a commit
that referenced
this pull request
Oct 7, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As a followup to #68 (BCEL-342), now that the tests are migrated to JUnit Jupiter, it's a good opportunity to go over the assertions and clean them up.
The main gain from this PR is improving the readability and maintainability of the tests. There is a theoretical performance improvement here too, but my benchmarking it on my limited resources, I could not observe a meaningful difference.
This PR offers the following improvements:
assertTrue(!condition, message)toassertTrue(condtion, message)in order to make the tests easier to understandassertTrue(x.equals(y), message)toassertEquals(x, y, message)in order to make the tests easier to understand. As a side bonus, most of these messages were constructed dynamically with the values ofxandy, and usingassertEqualsallowed relying on JUnit's existing functionality to display them in case of failure instead of reinventing the wheel. This not only shortens the code by removing boilerplate logical, but also theoritically improves performance as this string needs to be constructed only in case of a test failure.assertTrue(primitiveX == primitiveY, message)toassertEquals(x, y, message), for similar benefits as the previous point.if(!x.equals(y)) fail(message)toassertEquals(x, y, message), for similar benefits as the previous point.Supplier<String>that supplies the same message so that it's only evaluated in case of an assertion failure.