Annotation lookup on @Nested constructor parameter fails if test class is compiled with JDK 8 #1345
Comments
|
I should note that this exception is not thrown for annotations on parameters of test methods defined in a nested test class, nor does it throw on annotations on fields in a nested class. |
sbrannen
changed the title
|
Thanks for raising the issue. We'll look into it! |
|
Thanks to your example, I am able to reproduce this behavior against So that gives us a sound foundation to track down the bug. Cheers! |
|
Well, I've got some good news and some bad news. It took me quite a bit of debugging... that lead me to believe it was actually a bug in the JDK. That turned out to be true.
Related JDK bugs: The place where it actually blows up is in public Annotation[] getDeclaredAnnotations() {
return executable.getParameterAnnotations()[index];
}The problem is that JDK 8 compiles the wrong annotation arrays into the byte code. So there's an "off by one" error when indexing the array returned by the Through a bit of debugging, I determined that the annotation is present on the invisible (synthetic) first argument to the constructor for the inner class which is actually the parameter for the outer instance. Crazy! So with that in mind, I'm afraid we'll have to close this bug report and leave it as is. |
|
Or.... @junit-team/junit-lambda / @TimvdLippe ... Anyone have any ideas for a robust way to handle this? |
|
By the way, here's the technical explanation for the bug in JDK 8 (copied from the above linked bug issue):
|
|
@sbrannen Thanks for the detailed investigation! The options are limited I fear, but maybe the following would work? Introduce a new interface Essentially this would interface would receive a Class it would manually look in the annotations on |
|
FWIW, I actually tested the example in this issue against JDK 9.0.4, and the nested test passes. |
|
Yep, I totally agree that the options are limited for an issue like this (i.e., a bug in the compiled byte code). |
|
@TimvdLippe, I'm not so sure we'd want to introduce a new extension API just to handle this. But... I am considering adding an additional method to |
|
But... if we do address this somehow, extension authors would have to be made aware of the problem, and it would never be intuitive to be forced to use the convenience method only when resolving a constructor parameter for a |
|
TBH, I don't even know how easy it would be for JUnit to check if the test class was compiled on JDK 8. |
|
OK, I think I have a means for determining if the byte code is messed up. I added the following to the System.err.format("%d: %s --> %s%n", 0, parameter, Arrays.toString(parameter.getAnnotations()));
System.err.format(">>> executable.getParameters().length: %d%n", executable.getParameters().length);
System.err.format(">>> executable.getParameterAnnotations().length: %d%n", executable.getParameterAnnotations().length);And these are the results... JDK 8JDK 9+So it looks like comparing the lengths of the |
|
If we do that and then do this within the for-loop... Parameter parameter = (outerInstance == null) ? parameters[i] : parameters[i - 1];... the test then passes on JDK 8. Except... that we have to comment out the invocation of Which means we'd have to hack that algorithm as well... so that it actually checks the resolved type against the Starting to sound very hacky to me...... |
|
Of course, we wouldn't actually want to provide a I was just investigating if we could even find the correct information and make it available to extension authors via a convenience method that does not use the |
|
Update: changed label from |
sbrannen
changed the title
This commit introduces @nested tests to verify support for constructor injection when using the SpringExtension. One of the tests is disabled on Java 8 due to a bug in javac that was first fixed in JDK 9. See junit-team/junit5#1345 for details.
|
FYI: I verified that the |
Due to a bug in javac on JDK versions prior to JDK 9, looking up annotations directly on a java.lang.reflect.Parameter will fail for inner class constructors. Specifically, the parameter annotations array in the compiled byte code for the user's test class excludes an entry for the implicit enclosing instance parameter for an inner class constructor. This commit introduces a workaround for this off-by-one error by helping extension authors to access annotations on the preceding Parameter object (i.e., index - 1). This workaround is made available to extension authors via the following new convenience methods in the ParameterContext API. - boolean isAnnotated(Class<? extends Annotation> annotationType); - Optional<A> findAnnotation(Class<A> annotationType); - List<A> findRepeatableAnnotations(Class<A> annotationType); Issue: #1345
|
Merged feature branch into |
ghost
removed
the
|
Reopening since the team has not yet decided on this:
|
|
Added team discussion label to address backporting |
|
Awesome thanks. Looking forward to the new release 🎉 |
|
Per mockito/mockito#1350 (comment) I would like to voice my opinion regarding a backport. We are not eager to maintain potential workarounds with shipping new code, therefore having a backport would enable us to ship JUnit support sooner. Personally I would rather hold off with a feature if it requires a workaround, if I know the workaround will be removed "soon". |
|
I'm in favor of backporting it, too. |
|
Fine with me. Let's bundle and ship 5.1.1 soon. |
|
okie dokie! @marcphilipp, thanks for creating the 5.1.1 milestone and reassigning this issue to it. 👍 |
|
I'll backport it..... |
|
in progress |
Due to a bug in javac on JDK versions prior to JDK 9, looking up annotations directly on a java.lang.reflect.Parameter will fail for inner class constructors. Specifically, the parameter annotations array in the compiled byte code for the user's test class excludes an entry for the implicit enclosing instance parameter for an inner class constructor. This commit introduces a workaround for this off-by-one error by helping extension authors to access annotations on the preceding Parameter object (i.e., index - 1). This workaround is made available to extension authors via the following new convenience methods in the ParameterContext API. - boolean isAnnotated(Class<? extends Annotation> annotationType); - Optional<A> findAnnotation(Class<A> annotationType); - List<A> findRepeatableAnnotations(Class<A> annotationType); Issue: #1345
|
Backported to 5.1.1 in commit a01c112. |
This commit introduces a @nested test to verify support for constructor injection via a ParameterResolver that looks up an annotation on the Parameter supplied via the ParameterContext. The test is @DisabledOnJre(JAVA_8) due to a bug in javac that was first fixed in JDK 9. Note, however, that the test executes and passes on both JDK 9 and JDK 10. Issue: junit-team#1345
Due to a bug in javac on JDK versions prior to JDK 9, looking up annotations directly on a java.lang.reflect.Parameter will fail for inner class constructors. Specifically, the parameter annotations array in the compiled byte code for the user's test class excludes an entry for the implicit enclosing instance parameter for an inner class constructor. This commit introduces a workaround for this off-by-one error by helping extension authors to access annotations on the preceding Parameter object (i.e., index - 1). This workaround is made available to extension authors via the following new convenience methods in the ParameterContext API. - boolean isAnnotated(Class<? extends Annotation> annotationType); - Optional<A> findAnnotation(Class<A> annotationType); - List<A> findRepeatableAnnotations(Class<A> annotationType); Issue: junit-team#1345
Bug Report
Version:
junit-jupiter-api:5.1.0Given the following
Extensionwhich supports a custom@Parameterizedannotation:And the following usage of the extension, by annotating the constructor parameters:
Then when executing this class file, the test defined in
ExampleTestwill pass, but the test inNestedClasswill fail with the following exception:As such, resolution of a parameter in constructor of a
@Nestedtest class fails, while the same parameter resolved on the root class constructor works just fine.I encountered this issue while testing out the capabilities of the
MockitoExtensionI was developing.Deliverables
ParameterContext.The text was updated successfully, but these errors were encountered: