You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/10/07 10:50:18 UTC
[GitHub] [commons-bcel] mureinik opened a new pull request #69: BCEL-343 Assertion improvement
mureinik opened a new pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69
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:
1. Remvoes unused methods from test classes
1. Removes unnescesary casts from test classes
1. Simplifies the idiom of `assertTrue(!condition, message)` to `assertTrue(condtion, message)` in order to make the tests easier to understand
1. Simplifies the idiom of `assertTrue(x.equals(y), message)` to `assertEquals(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 of `x` and `y`, and using `assertEquals` allowed 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.
1. Simplifies the idiom of `assertTrue(primitiveX == primitiveY, message)` to `assertEquals(x, y, message)`, for similar benefits as the previous point.
1. Simplifies the idiom of `if(!x.equals(y)) fail(message)` to `assertEquals(x, y, message)`, for similar benefits as the previous point.
1. Simplifies the idiom of catching an exception and explicitly failing to allowing the test methods to throw those exceptions and have the test runner handle it.
1. In the remaining cases where an assertion had a non-constant string message it was replaced with a `Supplier<String>` that supplies the same message so that it's only evaluated in case of an assertion failure.
1. Some minor typos in assertion messages were fixed where noticed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory commented on pull request #69: BCEL-343 Assertion improvement
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69#issuecomment-704908154
https://issues.apache.org/jira/browse/BCEL-343
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory commented on a change in pull request #69: BCEL-343 Assertion improvement
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69#discussion_r500974895
##########
File path: src/test/java/org/apache/bcel/classfile/JDKClassDumpTestCase.java
##########
@@ -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;
Review comment:
Do we really need `j`?
##########
File path: src/test/java/org/apache/bcel/generic/GeneratingAnnotatedClassesTestCase.java
##########
@@ -352,16 +327,14 @@ public void testTransformComplexClassToClassGen()
final ClassGen cgen = new ClassGen(jc);
// Check annotations are correctly preserved
final AnnotationEntryGen[] annotations = cgen.getAnnotationEntries();
- assertTrue(annotations.length == 1,
- "Expected one annotation but found " + annotations.length);
+ assertEquals(1, annotations.length,"Wrong number of annotations");
Review comment:
Oops, add space after ","
##########
File path: src/test/java/org/apache/bcel/generic/FieldAnnotationsTestCase.java
##########
@@ -139,38 +137,10 @@ public void checkAnnotatedField(final JavaClass clazz, final String fieldname,
private void checkAnnotationEntry(final AnnotationEntry a, final String name, final String elementname,
final String elementvalue)
{
- assertTrue(a.getAnnotationType().equals(name),
- "Expected AnnotationEntry to have name " + name
- + " but it had name " + a.getAnnotationType());
- assertTrue(a.getElementValuePairs().length == 1,
- "Expected AnnotationEntry to have one element but it had "
- + a.getElementValuePairs().length);
+ assertEquals(name, a.getAnnotationType(), "Wrong AnnotationEntry name");
+ assertEquals(1, a.getElementValuePairs().length,"Wrong number of AnnotationEntry elements");
Review comment:
Format.
##########
File path: src/test/java/org/apache/bcel/generic/FieldAnnotationsTestCase.java
##########
@@ -115,9 +115,7 @@ public void testFieldAnnotationModification()
System.err.println("With AnnotationEntrys: "
+ dumpAnnotationEntries(f.getAnnotationEntries()));
}
- assertTrue(f.getAnnotationEntries().length == 2,
- "Should be 2 AnnotationEntrys on this field, but there are "
- + f.getAnnotationEntries().length);
+ assertEquals(2, f.getAnnotationEntries().length,"Wrong number of AnnotationEntries");
Review comment:
Format.
##########
File path: src/test/java/org/apache/bcel/EnclosingMethodAttributeTestCase.java
##########
@@ -89,8 +81,7 @@ public void testAttributeSerializtion() throws ClassNotFoundException,
final JavaClass clazz = getTestClass(PACKAGE_BASE_NAME+".data.AttributeTestClassEM02$1");
final ConstantPool pool = clazz.getConstantPool();
final Attribute[] encMethodAttrs = findAttribute("EnclosingMethod", clazz);
- assertTrue(encMethodAttrs.length == 1,
- "Expected 1 EnclosingMethod attribute but found " + encMethodAttrs.length);
+ assertEquals(1, encMethodAttrs.length,"Wrong number of EnclosingMethod attributes");
Review comment:
Format (comma)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] mureinik commented on a change in pull request #69: BCEL-343 Assertion improvement
Posted by GitBox <gi...@apache.org>.
mureinik commented on a change in pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69#discussion_r500995873
##########
File path: src/test/java/org/apache/bcel/generic/FieldAnnotationsTestCase.java
##########
@@ -115,9 +115,7 @@ public void testFieldAnnotationModification()
System.err.println("With AnnotationEntrys: "
+ dumpAnnotationEntries(f.getAnnotationEntries()));
}
- assertTrue(f.getAnnotationEntries().length == 2,
- "Should be 2 AnnotationEntrys on this field, but there are "
- + f.getAnnotationEntries().length);
+ assertEquals(2, f.getAnnotationEntries().length,"Wrong number of AnnotationEntries");
Review comment:
Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] mureinik commented on a change in pull request #69: BCEL-343 Assertion improvement
Posted by GitBox <gi...@apache.org>.
mureinik commented on a change in pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69#discussion_r500995547
##########
File path: src/test/java/org/apache/bcel/EnclosingMethodAttributeTestCase.java
##########
@@ -89,8 +81,7 @@ public void testAttributeSerializtion() throws ClassNotFoundException,
final JavaClass clazz = getTestClass(PACKAGE_BASE_NAME+".data.AttributeTestClassEM02$1");
final ConstantPool pool = clazz.getConstantPool();
final Attribute[] encMethodAttrs = findAttribute("EnclosingMethod", clazz);
- assertTrue(encMethodAttrs.length == 1,
- "Expected 1 EnclosingMethod attribute but found " + encMethodAttrs.length);
+ assertEquals(1, encMethodAttrs.length,"Wrong number of EnclosingMethod attributes");
Review comment:
Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory merged pull request #69: BCEL-343 JUnit Assertion improvement
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] mureinik commented on pull request #69: BCEL-343 Assertion improvement
Posted by GitBox <gi...@apache.org>.
mureinik commented on pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69#issuecomment-704923986
@garydgregory Fixed the formatting issues and answered wrt the usage of `j` (see inline)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] mureinik commented on a change in pull request #69: BCEL-343 Assertion improvement
Posted by GitBox <gi...@apache.org>.
mureinik commented on a change in pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69#discussion_r500995426
##########
File path: src/test/java/org/apache/bcel/generic/FieldAnnotationsTestCase.java
##########
@@ -139,38 +137,10 @@ public void checkAnnotatedField(final JavaClass clazz, final String fieldname,
private void checkAnnotationEntry(final AnnotationEntry a, final String name, final String elementname,
final String elementvalue)
{
- assertTrue(a.getAnnotationType().equals(name),
- "Expected AnnotationEntry to have name " + name
- + " but it had name " + a.getAnnotationType());
- assertTrue(a.getElementValuePairs().length == 1,
- "Expected AnnotationEntry to have one element but it had "
- + a.getElementValuePairs().length);
+ assertEquals(name, a.getAnnotationType(), "Wrong AnnotationEntry name");
+ assertEquals(1, a.getElementValuePairs().length,"Wrong number of AnnotationEntry elements");
Review comment:
Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] mureinik commented on a change in pull request #69: BCEL-343 Assertion improvement
Posted by GitBox <gi...@apache.org>.
mureinik commented on a change in pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69#discussion_r500988579
##########
File path: src/test/java/org/apache/bcel/classfile/JDKClassDumpTestCase.java
##########
@@ -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;
Review 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`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] mureinik commented on a change in pull request #69: BCEL-343 Assertion improvement
Posted by GitBox <gi...@apache.org>.
mureinik commented on a change in pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69#discussion_r500988579
##########
File path: src/test/java/org/apache/bcel/classfile/JDKClassDumpTestCase.java
##########
@@ -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;
Review 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).
Alternatively, we can leave the message as is and not replace it with a `Supplier`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] mureinik commented on a change in pull request #69: BCEL-343 Assertion improvement
Posted by GitBox <gi...@apache.org>.
mureinik commented on a change in pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69#discussion_r500988579
##########
File path: src/test/java/org/apache/bcel/classfile/JDKClassDumpTestCase.java
##########
@@ -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;
Review 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 scopped in the loop's body).
Alternatively, we can leave the message as is and not replace it with a `Supplier`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] mureinik edited a comment on pull request #69: BCEL-343 Assertion improvement
Posted by GitBox <gi...@apache.org>.
mureinik edited a comment on pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69#issuecomment-704923986
@garydgregory Fixed the formatting issues and answered wrt the usage of `j` (see inline - left that comment unresolved for your decision)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] mureinik commented on a change in pull request #69: BCEL-343 Assertion improvement
Posted by GitBox <gi...@apache.org>.
mureinik commented on a change in pull request #69:
URL: https://github.com/apache/commons-bcel/pull/69#discussion_r500995185
##########
File path: src/test/java/org/apache/bcel/generic/GeneratingAnnotatedClassesTestCase.java
##########
@@ -352,16 +327,14 @@ public void testTransformComplexClassToClassGen()
final ClassGen cgen = new ClassGen(jc);
// Check annotations are correctly preserved
final AnnotationEntryGen[] annotations = cgen.getAnnotationEntries();
- assertTrue(annotations.length == 1,
- "Expected one annotation but found " + annotations.length);
+ assertEquals(1, annotations.length,"Wrong number of annotations");
Review comment:
Fixed, sorry about that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org