Exclude enum methods "values" and "valueOf" from reports #491
Conversation
Godin
force-pushed
the
issue-491
branch
4 times, most recently
from
d2e92e2
to
7def60f
Compare
| final String superClassName, final int access, final String name, | ||
| final String desc) { | ||
| if ("java/lang/Enum".equals(superClassName) | ||
| && ("values".equals(name) || name.equals("valueOf") && desc |
There was a problem hiding this comment.
@Godin Shouldn't we also check the signature of the values method? For readability I would prefer braces or a separate statement for each method. Maybe also some inline comments what we're going to filter here.
There was a problem hiding this comment.
Good catch 👍 Don't know why my mind decided that signature check not needed for values method, while needed for valueOf 😆
| @@ -87,4 +87,24 @@ public void testMethodFilter_Lambda() { | |||
| assertEquals(1, coverage.getMethods().size()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testMethodFilter_EnumValues() { | |||
There was a problem hiding this comment.
@Godin What about a negative test for different signature?
| } | ||
|
|
||
| @Test | ||
| public void testMethodFilter_EnumValueOf() { |
There was a problem hiding this comment.
@Godin Same here: Negativ test for different signature.
Godin
force-pushed
the
issue-491
branch
2 times, most recently
from
539e4de
to
a038712
Compare
| final String desc) { | ||
| if ("java/lang/Enum".equals(superClassName)) { | ||
| // filter out methods that compiler creates for enums | ||
| if ("values".equals(name) |
There was a problem hiding this comment.
We mix two different comparisons styles here: "expected".equals(actual) vs. actual.equals("expected"). I would prefer to use the first one only as it is NPE safe.
There was a problem hiding this comment.
Umm, below there is anyway name.startsWith(...) that isn't NPE safe. Also according to my knowledge of ASM API - desc can't be null. For the NPE safety IMO far better to ensure that parameters of methods are not null by default and nullability should be stated explicitly.
But ok, I'll swap arguments, hoping that you won't say that concatenation on left hand side looks awkward 😆
| analyzer.visit(Opcodes.V1_5, Opcodes.ACC_PUBLIC, "Foo", null, | ||
| "java/lang/Enum", null); | ||
|
|
||
| MethodProbesVisitor mv = analyzer.visitMethod(0, "values", "()[LFoo;", |
There was a problem hiding this comment.
Shouldn't that be two test methods? Took me a short moment to realize that this tests two setups.
There was a problem hiding this comment.
I hesitated to create separate, because then IMO not so easy to catch difference between them. But ok, I'll split.
I propose to do a baby step in story of filtering by excluding enum methods
valuesandvalueOffrom reports.