[GR-60258] Refactor JUnit feature #693

dnestoro · 2025-02-12T16:40:43Z

Problem:

Currently, we are running test discovery and generate test plan at buildtime.
Then, we are storing launcher and test plan in the ImageSingleton, in order to run it at runtime.
This approach requires many classes to be initialized at buildtime.
The negative effect of this approach is that we have to maintain multiple lists of classes that should be initialized at buildtime (here, here, and here).

Idea:

Move test plan generation and tests execution to runtime.

Solution:

PREREQUISITE: We are using test-ids generated before test compilation to discover tests (currently on master as well)
We should run test discovery at buildtime to only collect classes needed for reflection registration
Then at runtime, we will have everything necessary for reflection, and the only thing left is to run test discovery with test-ids to find tests and execute them
Test-ids location should be passed to the NativeImageJUnitLauncher class, so we simply set systemProperty with this location in order to read it at runtime (we do this automatically for both gradle and maven)

This PR:

removes hardcoded lists for initializeAtBuildtime (here, here, and here)
removes launcher and testPlan from ImageSingletons (here) => therefore we don't need classes to be initialized at buildtime
computes testPlan at runtime

In addition:

registers interfaces for reflection to solve Interfaces implemented by test classes are not registered for reflection #636
adds support for @FieldSource annotations to solve Add support for @FieldSource for @ParameterizedTest methods #638
Adds support for org.junit.rules.ExpectedException (code originally taken from this PR cc @tzezula)
Fixes JUnit tests that fail when updating JUnitJupiter from 5.10.0 to 5.11.0 (see this)
Adds functional tests for both gradle and maven

melix

Thanks David. Can you give a description of the changes that this makes? For example you mention that the tests are discovered at runtime but in practice you are still using the test-ids.

There are changes which need to be reverted on the samples: build plugins versions and maven repositories.

dnestoro · 2025-02-13T15:28:34Z

Thanks David. Can you give a description of the changes that this makes? For example you mention that the tests are discovered at runtime but in practice you are still using the test-ids.

There are changes which need to be reverted on the samples: build plugins versions and maven repositories.

Hey @melix the PR is not ready yet (it is in draft state). I will update PR description and revert unnecessary changes after I finish testing.

olpaw · 2025-02-24T13:18:07Z

Please clean up the commit history: https://github.com/graalvm/native-build-tools/pull/693/commits

E.g. fold
[Fix checkstyle errors] commits into the commit that introduced the style error.

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

dnestoro · 2025-02-24T14:25:28Z

gradle/libs.versions.toml

@@ -12,7 +12,7 @@ mavenResolver = "1.9.22"
 graalvm = "23.0.2"
 openjson = "1.0.13"
 junitPlatform = "1.10.0"
-junitJupiter = "5.10.0"
+junitJupiter = "5.11.0"


I updated this in order to provide support for @FieldSource annotation introduced in 5.11.0 (see this)

Please note that we recently released JUnit 5.12.

https://github.com/junit-team/junit5/releases/tag/r5.12.0

So, it's probably a good idea to upgrade to that. I can create a separate GitHub issue for that if you like.

As a side note, JUnit 5.12 includes native-image.properties files (see release notes).

All affected JAR files now include native-image.properties files that contain the --initialize-at-build-time option to avoid breakages in GraalVM projects when updating to newer versions of JUnit.

So, that might affect your refactoring effort.

See also: Plan for JUnit 6 support #695

As a side note, JUnit 5.12 includes native-image.properties files (see release notes).

@dnestoro , that means we have to use --exclude-config ... to ensure we do not get those --initialize-at-build-time options for your new implementation that does not require any initialize-at-build-time classes.

If anything is incomplete in the out-of-the-box properties files in the JUnit 5 JARs, please let us (the JUnit Team) know, so that we can address that.

Hey @sbrannen, I tried JUnit 5.12 with both my implementation and the current master. My findings are the following:

If I bump JUnit version from 5.10 to 5.12 image build command contains multiple lines like:

-H:ClassInitialization@jar:.../.gradle/caches/modules-2/files-2.1/org.junit.platform/junit-platform-reporting/.../META-INF/native-image/org.junit.platform/junit-platform-reporting/native-image.properties+api=org.junit.platform.reporting.open.xml.OpenTestReportGeneratingListener...

So native-image.properties are really provided to the native image

But if I remove our own initializeAtBuildtime (either on master or in my implementation) I am facing the following error:

Fatal error: com.oracle.graal.pointsto.constraints.UnsupportedFeatureException: An object of type 'org.junit.platform.commons.support.scanning.DefaultClasspathScanner' was found in the image heap. This type, however, is marked for initialization at image run time for the following reason: classes are initialized at run time by default. This is not allowed for correctness reasons: All objects that are stored in the image heap must be initialized at build time. ...

So, it looks like properties files provided by JUnit are not complete

@marcphilipp, please take a look. Thanks!

I doubt that's the only class that's missing. I could copy the list of classes from the hard-coded lists in the feature implementation but I'd rather extend our existing tests so it exercises all relevant code.

But if I remove our own initializeAtBuildtime (either on master or in my implementation) I am facing the following error

@dnestoro What do I need run to reproduce this locally?

I am so confused now... I tried to reproduce this error again, but now everything works correctly on master (while on my branch it fails with different errors depending on the GraalVM version I use). I will double check what happened here (maybe something got cached) because I am 100% sure that it was failing on master as well.

@marcphilipp @sbrannen Sorry for the false alarm. I will investigate this deeper to see if something is still missing on master and give you a reproducer if something fails.

fniephaus · 2025-02-26T12:20:27Z

While not so concerning for testing, how much bigger do such test images get when launcher and testPlan are no longer ImageSingletons? Bigger or smaller? :)

melix

I didn't finish the review but left a few comments. I'm still confused and would appreciate a few more details:

you are saying that we should discover the test ids at runtime, however, we still run the tests in JVM mode to get the test ids?
if the test ids are not needed anymore, maybe we should change the name of the parameters which are used

melix · 2025-02-26T15:18:40Z

...e-plugin/src/functionalTest/groovy/org/graalvm/buildtools/gradle/JUnitFunctionalTests.groovy

+import java.nio.file.Files
+
+class JUnitFunctionalTests extends AbstractFunctionalTest {
+    @Unroll("test if JUint support works with various annotations, reflection and resources")


:nit: this is not an unrolled test. I guess you just wanted a display name, in which case you can put this directly in the test method name (also, there's a typo in JUnit)

native-gradle-plugin/src/main/java/org/graalvm/buildtools/gradle/NativeImagePlugin.java

...junit-platform-native/src/main/java/org/graalvm/junit/platform/NativeImageJUnitLauncher.java

dnestoro · 2025-02-26T15:48:57Z

While not so concerning for testing, how much bigger do such test images get when launcher and testPlan are no longer ImageSingletons? Bigger or smaller? :)

On a first look, the tests image size gets around 5% of increase. But I will run more tests and provide more accurate information soon

dnestoro · 2025-02-26T15:57:34Z

@melix

you are saying that we should discover the test ids at runtime, however, we still run the tests in JVM mode to get the test ids?

if the test ids are not needed anymore, maybe we should change the name of the parameters which are used

No, I am just saying that we should use test ids to compute test plan (to discover it with launcher.discover(request)) at runtime. With that approach, we don't have to compute testPlan at buildtime and store it in ImageSingletons in order to use it at runtime (as we currently do on master)

dnestoro · 2025-03-03T14:05:50Z

@fniephaus after deeper investigation, I came to the following results regarding image size:

On an image created from a single test:
- master: 25.47MB
- branch: 27.29MB
- diff: 1.82MB (6.6691% bigger image size)
On an image created from 400 tests:
- master: 26.58MB
- branch: 27.88MB
- diff: 1.3MB (4,6628% bigger image size)
On an image created from 1500 tests:
- master: 28.96MB
- branch: 29.31MB
- diff: 0.35MB (1.1941% bigger image size)
On an image created from 2800 tests, the master and the branch are equal
On an image, created from 10 000 tests:
- master: 48.81MB
- branch: 42.39MB
- diff: -6.42MB (13.15% smaller image size)

The more tests, the more benefits we get!

fniephaus · 2025-03-03T15:36:34Z

Thanks for checking, @dnestoro!

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

...junit-platform-native/src/main/java/org/graalvm/junit/platform/NativeImageJUnitLauncher.java

…g at buildtime

… JDKs in a separate file

…r earlier JDKs

…dtime

common/junit-platform-native/src/main/resources/initialize-at-buildtime

common/junit-platform-native/gradle/native-image-testing.gradle

native-gradle-plugin/src/main/java/org/graalvm/buildtools/gradle/NativeImagePlugin.java

common/junit-platform-native/gradle/native-image-testing.gradle

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

…ime list

marcphilipp

LGTM 👍

(I lack permissions to formally approve)

fniephaus

Thank you, @dnestoro!

marcphilipp

Good catch on the recursive type declarations! 👍

marcphilipp · 2025-05-01T07:57:06Z

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

+
+    private final Set<Class<?>> registeredClasses = new HashSet<>();


I don't think this should be a field. Instead, it should be possible to pass it as a paramter to registerTestClassForReflection and shouldRegisterClass.

Suggested change

private final Set<Class<?>> registeredClasses = new HashSet<>();

marcphilipp · 2025-05-01T07:57:26Z

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

        LauncherDiscoveryRequest request = LauncherDiscoveryRequestBuilder.request()
                .selectors(selectors)
                .build();

+        Launcher launcher = LauncherFactory.create();


Suggested change

Launcher launcher = LauncherFactory.create();

Set<Class<?>> registeredClasses = new HashSet<>();

Launcher launcher = LauncherFactory.create();

marcphilipp · 2025-05-01T07:57:38Z

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

@@ -143,14 +168,44 @@ private TestPlan discoverTestsAndRegisterTestClassesForReflection(Launcher launc
                .map(ClassSource.class::cast)
                .map(ClassSource::getJavaClass)
                .forEach(this::registerTestClassForReflection);


Suggested change

.forEach(this::registerTestClassForReflection);

.forEach(clazz -> registerTestClassForReflection(clazz, registeredClasses));

marcphilipp · 2025-05-01T07:57:52Z

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

+
+    private final Set<Class<?>> registeredClasses = new HashSet<>();
+
+    private boolean shouldRegisterClass(Class<?> clazz) {


Suggested change

private boolean shouldRegisterClass(Class<?> clazz) {

private boolean shouldRegisterClass(Class<?> clazz, Set<Class<?>> registeredClasses) {

marcphilipp · 2025-05-01T07:59:12Z

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

+        if (registeredClasses.contains(clazz)) {
+            return false;
+        }
+        registeredClasses.add(clazz);

-        return testPlan;
+        return true;


This can be simplified to the following, but your version may be more readable.

return registeredClasses.add(clazz);

marcphilipp · 2025-05-01T07:59:30Z

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

    private void registerTestClassForReflection(Class<?> clazz) {
+        if (!shouldRegisterClass(clazz)) {


Suggested change

private void registerTestClassForReflection(Class<?> clazz) {

if (!shouldRegisterClass(clazz)) {

private void registerTestClassForReflection(Class<?> clazz, Set<Class<?>> registeredClasses) {

if (!shouldRegisterClass(clazz, registeredClasses)) {

marcphilipp · 2025-05-01T07:59:54Z

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

+
+        Class<?>[] declaredClasses = clazz.getDeclaredClasses();
+        for (Class<?> declaredClass : declaredClasses) {
+            registerTestClassForReflection(declaredClass);


Suggested change

registerTestClassForReflection(declaredClass);

registerTestClassForReflection(declaredClass, registeredClasses);

marcphilipp · 2025-05-01T08:00:05Z

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

+
+        Class<?>[] interfaces = clazz.getInterfaces();
+        for (Class<?> inter : interfaces) {
+            registerTestClassForReflection(inter);


Suggested change

registerTestClassForReflection(inter);

registerTestClassForReflection(inter, registeredClasses);

marcphilipp · 2025-05-01T08:00:15Z

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java

+        for (Class<?> inter : interfaces) {
+            registerTestClassForReflection(inter);
+        }
+
        Class<?> superClass = clazz.getSuperclass();
        if (superClass != null && superClass != Object.class) {
            registerTestClassForReflection(superClass);


Suggested change

registerTestClassForReflection(superClass);

registerTestClassForReflection(superClass, registeredClasses);

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 12, 2025

melix requested changes Feb 13, 2025

View reviewed changes

dnestoro mentioned this pull request Feb 13, 2025

Add support for @FieldSource for @ParameterizedTest methods #638

Open

dnestoro force-pushed the dnestoro/refactor-junit-feature branch 3 times, most recently from 29c75fe to b80fd65 Compare February 21, 2025 01:56

olpaw reviewed Feb 24, 2025

View reviewed changes

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java Show resolved Hide resolved

dnestoro commented Feb 24, 2025

View reviewed changes

dnestoro force-pushed the dnestoro/refactor-junit-feature branch 2 times, most recently from 2448625 to e5d1f01 Compare February 25, 2025 16:46

dnestoro marked this pull request as ready for review February 26, 2025 11:41

dnestoro requested review from olpaw, melix, fniephaus and vjovanov February 26, 2025 11:41

linghengqian mentioned this pull request Feb 26, 2025

Clarify the buildArg of GraalVM Native Image associated with --initialize-at-build-time=org.junit.platform.engine.TestTag linghengqian/graalvm-trace-metadata-smoketest#3

Open

melix requested changes Feb 26, 2025

View reviewed changes

linghengqian mentioned this pull request Mar 2, 2025

Bump Junit5 to 5.12.0 linghengqian/hive-server2-jdbc-driver#26

Open

dnestoro changed the title ~~Refactor JUnit feature~~ [GR-60258] Refactor JUnit feature Mar 3, 2025

dnestoro requested review from sbrannen and melix March 3, 2025 14:09

olpaw reviewed Mar 3, 2025

View reviewed changes

common/junit-platform-native/src/main/java/org/graalvm/junit/platform/JUnitPlatformFeature.java Show resolved Hide resolved

olpaw reviewed Mar 3, 2025

View reviewed changes

...junit-platform-native/src/main/java/org/graalvm/junit/platform/NativeImageJUnitLauncher.java Outdated Show resolved Hide resolved

dnestoro added 4 commits April 16, 2025 12:42

Use several specific packages instead of one general when initializin…

dd4f497

…g at buildtime

Store all classes that should be initialized at buildtime for earlier…

82d3fbb

… JDKs in a separate file

Extract annottaion element fetching into a separate method

3364e05

Improve error message and rename function for initializing classes fo…

c890433

…r earlier JDKs

dnestoro force-pushed the dnestoro/refactor-junit-feature branch from 9637662 to c890433 Compare April 16, 2025 10:42

dnestoro marked this pull request as ready for review April 16, 2025 11:35

dnestoro requested a review from olpaw April 16, 2025 11:35

This was referenced Apr 17, 2025

Add support for JUnit's @ParameterizedClass #711

Closed

Remove native-image.properties files from published JARs junit-team/junit5#4488

Closed

dnestoro added 3 commits April 17, 2025 13:29

Adjust initialize-at-buildtime list with JUnit list

cae0ced

Add exclude config for JUnit versions that provide initialize-at-buil…

c3979f4

…dtime

Add exclude config in both Gradle and Maven

7f51603

dnestoro self-assigned this Apr 23, 2025

Use JUnit 5.11.0 version

04198fc

dnestoro requested review from marcphilipp, tzezula and melix April 24, 2025 10:48

Use try with resources to properly close BufferedReader

6f9748e

olpaw requested changes Apr 24, 2025

View reviewed changes

common/junit-platform-native/src/main/resources/initialize-at-buildtime Outdated Show resolved Hide resolved

marcphilipp reviewed Apr 28, 2025

View reviewed changes

dnestoro added 2 commits April 28, 2025 12:19

Register all class members of inner classes for reflection

847ef48

Remove unused java.beans.Introspector entry form initialize-at-buildt…

2c2ec24

…ime list

olpaw previously approved these changes Apr 28, 2025

View reviewed changes

marcphilipp reviewed Apr 29, 2025

View reviewed changes

Avoid infinite loops when registering test classes for reflection

5b8d5fe

dnestoro dismissed olpaw’s stale review via 5b8d5fe April 30, 2025 12:48

tzezula approved these changes Apr 30, 2025

View reviewed changes

olpaw approved these changes Apr 30, 2025

View reviewed changes

fniephaus approved these changes Apr 30, 2025

View reviewed changes

marcphilipp reviewed May 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GR-60258] Refactor JUnit feature #693

[GR-60258] Refactor JUnit feature #693

dnestoro commented Feb 12, 2025 •

edited

Loading

melix left a comment

dnestoro commented Feb 13, 2025

olpaw commented Feb 24, 2025

dnestoro Feb 24, 2025

sbrannen Feb 24, 2025

sbrannen Feb 24, 2025 •

edited

Loading

sbrannen Feb 24, 2025

olpaw Feb 24, 2025

sbrannen Feb 24, 2025

dnestoro Feb 26, 2025

sbrannen Mar 24, 2025

marcphilipp Mar 24, 2025

dnestoro Mar 27, 2025

fniephaus commented Feb 26, 2025

melix left a comment

melix Feb 26, 2025

dnestoro commented Feb 26, 2025 •

edited

Loading

dnestoro commented Feb 26, 2025 •

edited

Loading

dnestoro commented Mar 3, 2025

fniephaus commented Mar 3, 2025 •

edited

Loading

marcphilipp left a comment

fniephaus left a comment

marcphilipp left a comment

marcphilipp May 1, 2025

marcphilipp May 1, 2025

marcphilipp May 1, 2025

marcphilipp May 1, 2025

marcphilipp May 1, 2025

marcphilipp May 1, 2025

marcphilipp May 1, 2025

marcphilipp May 1, 2025

marcphilipp May 1, 2025


		private final Set<Class<?>> registeredClasses = new HashSet<>();

	Launcher launcher = LauncherFactory.create();
	Set<Class<?>> registeredClasses = new HashSet<>();
	Launcher launcher = LauncherFactory.create();

	.forEach(this::registerTestClassForReflection);
	.forEach(clazz -> registerTestClassForReflection(clazz, registeredClasses));


		private final Set<Class<?>> registeredClasses = new HashSet<>();

		private boolean shouldRegisterClass(Class<?> clazz) {

		private void registerTestClassForReflection(Class<?> clazz) {
		if (!shouldRegisterClass(clazz)) {

	registerTestClassForReflection(declaredClass);
	registerTestClassForReflection(declaredClass, registeredClasses);

	registerTestClassForReflection(inter);
	registerTestClassForReflection(inter, registeredClasses);

	registerTestClassForReflection(superClass);
	registerTestClassForReflection(superClass, registeredClasses);

[GR-60258] Refactor JUnit feature #693

Are you sure you want to change the base?

[GR-60258] Refactor JUnit feature #693

Conversation

dnestoro commented Feb 12, 2025 • edited Loading

melix left a comment

Choose a reason for hiding this comment

dnestoro commented Feb 13, 2025

olpaw commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbrannen Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fniephaus commented Feb 26, 2025

melix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnestoro commented Feb 26, 2025 • edited Loading

dnestoro commented Feb 26, 2025 • edited Loading

dnestoro commented Mar 3, 2025

fniephaus commented Mar 3, 2025 • edited Loading

marcphilipp left a comment

Choose a reason for hiding this comment

fniephaus left a comment

Choose a reason for hiding this comment

marcphilipp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnestoro commented Feb 12, 2025 •

edited

Loading

sbrannen Feb 24, 2025 •

edited

Loading

dnestoro commented Feb 26, 2025 •

edited

Loading

dnestoro commented Feb 26, 2025 •

edited

Loading

fniephaus commented Mar 3, 2025 •

edited

Loading