Skip to content

Discover tests with ClassLoader other than ImageClassLoader - #445

Merged
melix merged 1 commit into
graalvm:masterfrom
ziyilin:discoverTestsWithDifferentClassLoader
Jul 12, 2023
Merged

Discover tests with ClassLoader other than ImageClassLoader#445
melix merged 1 commit into
graalvm:masterfrom
ziyilin:discoverTestsWithDifferentClassLoader

Conversation

@ziyilin

@ziyilin ziyilin commented Jun 2, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

JUnitPlatformFeature discovers tests before analysis. The discovery process will initialize some of the test classes. As they are loaded by the ImageClassloader, the initialization will cause eager class initialization error during native image building time.

This PR uses a different classloader than the ImageClassLoader in the JUnitPlatformFeature to make sure the test class initialization at discovery time won't affect the native image class initialization policy.

==updated July 7th 2023 ==
This PR launches a separated Java process to run the test discovery if -DisolateTestDiscovery=true is set (it is set by default), and goes to the original way otherwise. The reason is the initialization of test class may initialize some JDK classes that are loaded by null classloader. In this case, classloader isolation is not sufficient.

The attached demo shows the issue.
The GraalVM version is java11-22.3.1.
Simply unzip the file and run mvn -Pnative test and mvn -Pnative test -DisolateTestDiscovery=false to see the difference.

native-build-tool-demo.zip

@oracle-contributor-agreement oracle-contributor-agreement Bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jun 2, 2023
@vjovanov
vjovanov requested review from dnestoro and melix June 5, 2023 05:17
@melix

melix commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator

I am not sure this change is safe. Probably @sbrannen has more expertise than I do on this particular aspect. I'm in particular wondering about the use of the context classloader.

@ziyilin

ziyilin commented Jun 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Just found this commit is not enough to isolate more complicated class initializations during tests discovery. SubstrateVM's pointsto analysis scans constants and adds them into native image regardless how it is loaded. If such a constant's class should not be initialized at build time, but is initialized during test discovery. The build of native test will never succeed.
I'm trying to separate the tests discovery to run in another JVM, and return a file containing discovered test classes. In this case, code will be simpler and there is no need to set context classloader anymore.

I'm in particular wondering about the use of the context classloader.

For your question. SubstrateVM sets the context classloader to ImageClassloader which loads all classes that will be compiled into the native image. The initialization status of these classes is checked at building time. The junit platform uses the context classloader to load the tests. So I changed the context classloader to avoid loading and potential initialization of test classes.

@ziyilin
Discover tests with another Java process
3853830 
JUnitPlatformFeature discovers tests before analysis. The discovery
procsee will initialize some of the test classes when @RunWith,
@parameterized annotations are used. As they are loaded by
the ImageClassloader, the initialization may cause eager class
initialization error during native image building time.

This commit lauches the test discovery in a another Java process in
the JUnitPlatformFeature to make sure the test class initialization at
discovery time won't affect the native image class initialization
policy.
@ziyilin
ziyilin force-pushed the discoverTestsWithDifferentClassLoader branch from d4a6011 to 3853830 Compare June 7, 2023 09:35
@sbrannen sbrannen changed the title Discover tests with other classloader than ImageClassLoader Discover tests with ClassLoader other than ImageClassLoader Jun 16, 2023
@melix
melix merged commit 2988364 into graalvm:master Jul 12, 2023
@melix melix added this to the 0.9.24 milestone Jul 12, 2023
melix added a commit that referenced this pull request Jul 20, 2023
@melix
Revert "Discover tests with ClassLoader other than `ImageClassLoade…
2982d5d 
…r` (#445)"

This reverts commit 2988364.
@melix melix mentioned this pull request Jul 20, 2023
Merged
@melix

melix commented Jul 20, 2023

Copy link
Copy Markdown
Collaborator

We're going to have to revert this PR as it breaks builds with build time initialization happening with JUnit classes.

melix added a commit that referenced this pull request Jul 20, 2023
@melix
Revert "Discover tests with ClassLoader other than `ImageClassLoade…
7bdab7a 
…r`" (#470)

Revert "Discover tests with `ClassLoader` other than `ImageClassLoader` (#445)"

This reverts commit 2988364.
@ziyilin

ziyilin commented Jul 24, 2023

Copy link
Copy Markdown
Contributor Author

@melix Can you provide a test case？

@melix

melix commented Jul 24, 2023

Copy link
Copy Markdown
Collaborator

You simply have to run ./gradlew :native-gradle-plugin:funTest and every single test which uses nativeTest is going to fail.

@mend-for-github-com mend-for-github-com Bot mentioned this pull request Dec 8, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@melix melix Awaiting requested review from melix
@dnestoro dnestoro Awaiting requested review from dnestoro

Assignees

No one assigned

Labels

OCA Verified All contributors have signed the Oracle Contributor Agreement.

Projects

None yet

Milestone

0.9.24

Development

Successfully merging this pull request may close these issues.

2 participants

@ziyilin @melix