Skip to content

Commit

Permalink
Consistently use class literals for primitive types
Browse files Browse the repository at this point in the history 
To improve consistency and avoid confusion regarding primitive types
and their wrapper types, this commit ensures that we always use class
literals for primitive types.

For example, instead of using the `Void.TYPE` constant, we now
consistently use `void.class`.
  • Loading branch information
@sbrannen
sbrannen committed Feb 3, 2024
1 parent 6d75184 commit 10a5879
Show file tree
Hide file tree
Showing 7 changed files with 26 additions and 21 deletions.
4 changes: 2 additions & 2 deletions ...upiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/LifecycleMethodUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
package org.junit.jupiter.engine.descriptor;

import static org.junit.platform.commons.util.AnnotationUtils.findAnnotatedMethods;
import static org.junit.platform.commons.util.ReflectionUtils.returnsVoid;
import static org.junit.platform.commons.util.ReflectionUtils.returnsPrimitiveVoid;

import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
Expand Down Expand Up @@ -96,7 +96,7 @@ private static void assertNonStatic(Class<? extends Annotation> annotationType,
}

private static void assertVoid(Class<? extends Annotation> annotationType, Method method) {
if (!returnsVoid(method)) {
if (!returnsPrimitiveVoid(method)) {
throw new JUnitException(String.format("@%s method '%s' must not return a value.",
annotationType.getSimpleName(), method.toGenericString()));
}
Expand Down
10 changes: 5 additions & 5 deletions ...-engine/src/main/java/org/junit/jupiter/engine/discovery/predicates/IsTestableMethod.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import static org.junit.platform.commons.util.ReflectionUtils.isAbstract;
import static org.junit.platform.commons.util.ReflectionUtils.isPrivate;
import static org.junit.platform.commons.util.ReflectionUtils.isStatic;
import static org.junit.platform.commons.util.ReflectionUtils.returnsVoid;
import static org.junit.platform.commons.util.ReflectionUtils.returnsPrimitiveVoid;

import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
Expand All @@ -26,11 +26,11 @@
abstract class IsTestableMethod implements Predicate<Method> {

private final Class<? extends Annotation> annotationType;
private final boolean mustReturnVoid;
private final boolean mustReturnPrimitiveVoid;

IsTestableMethod(Class<? extends Annotation> annotationType, boolean mustReturnVoid) {
IsTestableMethod(Class<? extends Annotation> annotationType, boolean mustReturnPrimitiveVoid) {
this.annotationType = annotationType;
this.mustReturnVoid = mustReturnVoid;
this.mustReturnPrimitiveVoid = mustReturnPrimitiveVoid;
}

@Override
Expand All @@ -45,7 +45,7 @@ public boolean test(Method candidate) {
if (isAbstract(candidate)) {
return false;
}
if (returnsVoid(candidate) != this.mustReturnVoid) {
if (returnsPrimitiveVoid(candidate) != this.mustReturnPrimitiveVoid) {
return false;
}

Expand Down
10 changes: 8 additions & 2 deletions junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,8 +341,14 @@ public static boolean isInnerClass(Class<?> clazz) {
return !isStatic(clazz) && clazz.isMemberClass();
}

public static boolean returnsVoid(Method method) {
return method.getReturnType().equals(Void.TYPE);
/**
* Determine if the return type of the supplied method is primitive {@code void}.
*
* @param method the method to test; never {@code null}
* @return {@code true} if the method's return type is {@code void}
*/
public static boolean returnsPrimitiveVoid(Method method) {
return method.getReturnType() == void.class;
}

/**
Expand Down
6 changes: 3 additions & 3 deletions ...nit/platform/engine/support/hierarchical/ForkJoinPoolHierarchicalTestExecutorService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,9 @@ private ForkJoinPool createForkJoinPool(ParallelExecutionConfiguration configura
}

private static Optional<Constructor<ForkJoinPool>> sinceJava9Constructor() {
return Try.call(() -> ForkJoinPool.class.getDeclaredConstructor(Integer.TYPE, ForkJoinWorkerThreadFactory.class,
UncaughtExceptionHandler.class, Boolean.TYPE, Integer.TYPE, Integer.TYPE, Integer.TYPE, Predicate.class,
Long.TYPE, TimeUnit.class)) //
return Try.call(() -> ForkJoinPool.class.getDeclaredConstructor(int.class, ForkJoinWorkerThreadFactory.class,
UncaughtExceptionHandler.class, boolean.class, int.class, int.class, int.class, Predicate.class, long.class,
TimeUnit.class)) //
.toOptional();
}

Expand Down
3 changes: 1 addition & 2 deletions platform-tests/src/test/java/org/junit/platform/commons/util/ClassLoaderUtilsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,7 @@ void getLocationFromNullFails() {

@Test
void getLocationFromVariousObjectsArePresent() {
assertTrue(ClassLoaderUtils.getLocation(void.class).isPresent());
assertTrue(ClassLoaderUtils.getLocation(byte.class).isPresent());
assertTrue(ClassLoaderUtils.getLocation(getClass()).isPresent());
assertTrue(ClassLoaderUtils.getLocation(this).isPresent());
assertTrue(ClassLoaderUtils.getLocation("").isPresent());
assertTrue(ClassLoaderUtils.getLocation(0).isPresent());
Expand Down
10 changes: 5 additions & 5 deletions platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,13 +176,13 @@ void isNotFinal() throws Exception {
}

@Test
void returnsVoid() throws Exception {
void returnsPrimitiveVoid() throws Exception {
Class<?> clazz = ClassWithVoidAndNonVoidMethods.class;
assertTrue(ReflectionUtils.returnsVoid(clazz.getDeclaredMethod("voidMethod")));
assertTrue(ReflectionUtils.returnsPrimitiveVoid(clazz.getDeclaredMethod("voidMethod")));

assertFalse(ReflectionUtils.returnsVoid(clazz.getDeclaredMethod("methodReturningVoidReference")));
assertFalse(ReflectionUtils.returnsVoid(clazz.getDeclaredMethod("methodReturningObject")));
assertFalse(ReflectionUtils.returnsVoid(clazz.getDeclaredMethod("methodReturningPrimitive")));
assertFalse(ReflectionUtils.returnsPrimitiveVoid(clazz.getDeclaredMethod("methodReturningVoidReference")));
assertFalse(ReflectionUtils.returnsPrimitiveVoid(clazz.getDeclaredMethod("methodReturningObject")));
assertFalse(ReflectionUtils.returnsPrimitiveVoid(clazz.getDeclaredMethod("methodReturningPrimitive")));
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions ...m-tests/src/test/java/org/junit/platform/engine/support/descriptor/MethodSourceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ void getJavaMethodFromStringShouldFindVoidMethod() throws Exception {

@Test
void getJavaMethodFromStringShouldFindMethodWithParameter() throws Exception {
var testMethod = getClass().getDeclaredMethod("method3", Integer.TYPE);
var testMethod = getClass().getDeclaredMethod("method3", int.class);
var source = MethodSource.from(getClass().getName(), testMethod.getName(), testMethod.getParameterTypes());

assertThat(source.getJavaMethod()).isEqualTo(testMethod);
Expand All @@ -254,7 +254,7 @@ void getJavaMethodFromStringShouldThrowExceptionIfParameterTypesAreNotSupplied()

@Test
void getJavaMethodFromStringShouldThrowExceptionIfParameterTypesDoNotMatch() {
var source = MethodSource.from(getClass().getName(), "method3", Double.TYPE);
var source = MethodSource.from(getClass().getName(), "method3", double.class);

assertThrows(PreconditionViolationException.class, source::getJavaMethod);
}
Expand Down

7 comments on commit 10a5879

@kaipe
Copy link

@kaipe kaipe commented on 10a5879 Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this breaks backward compatibility. I.e. platform-1.11.0-M1 is no longer compatable with jupiter-5.10.2
I have a suite of regression tests to monitor modifications on junit-platform ...
mvn test -P jupiter_5.10.2 -P platform_latest
... where platform_latest has reached 1.11.0-M1. Now the test-suite is broken:

TestEngine with ID 'junit-jupiter' failed to discover tests
...
Caused by: java.lang.NoSuchMethodError: org.junit.platform.commons.util.ReflectionUtils.returnsVoid(Ljava/lang/reflect/Method;)Z
...

It would be great help if we let ReflectionUtils#returnsVoid(...) hang around as @Deprecated for a few more minor releases. Otherwise my test suite would also need to struggle with Jupiter milestone regressions (such as issue 3797) and the benefit of JUnit-5 modularization is diminished.

@marcphilipp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaipe ReflectionUtils is an internal class so we don't consider this a breaking change. Are you calling ReflectionUtils#returnsVoid(...) directly?

@kaipe
Copy link

@kaipe kaipe commented on 10a5879 Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dont call it all. My concern is junit-platform backward compatibility with junit-jupiter-engine.
I.e. version 1.11.0-M1 of junit-platform does not support junit-jupiter-engine-5.10.2, which is the most recent stable edition of junit-jupiter-engine.
Partly because of issue 3797 I was hoping to try most recent junit-platform without upgrading junit-jupiter-engine.

@pzygielo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under impression that junit-platform shares its lifecycle with other components, like junit-jupiter-engine. Thus the version of platform should match the one of engine. So for the most recent stable engine:5.10.2 it would be to stay at platform:1.10.2.

@marcphilipp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, those versions should always be aligned, e.g. by importing the JUnit BOM.

@kaipe
Copy link

@kaipe kaipe commented on 10a5879 Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I'm aware of the shared lifecycle but I have thought it concerns development and release. With JUnit-Platform on release-numbers 1.x and JUnit-Jupiter on 5.x I have previously thought of JUnit-Jupiter as one engine implementation among many - and that JUnit-Platform would maintain backward compatibility with all engines. It's with some surprise I now realize this maintained backward compatibility is limited to 3rd-party engines, whereas Jupiter (and Vintage?) utilize some platform internals that are frequently modified.

But what you say is indeed documented in the user guide and my original practical concerns are no longer valid after the quick fix for issue 3797. I will now reconfigure my regression test suites to make sure they no longer will have separate properties version.platform and version.jupiter - and instead rely on that JUnit BOM.

Thanks for your help!

@marcphilipp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's with some surprise I now realize this maintained backward compatibility is limited to 3rd-party engines, whereas Jupiter (and Vintage?) utilize some platform internals that are frequently modified.

Backwards compatibility is maintained for third-party engines as well but only for non-internal APIs such as ReflectionSupport, not internal ones such as ReflectionUtils.

Please sign in to comment.