7902847: Class directory of a test case should be always used to compile a library

[lmesnik]

The classes from test libraries can be compiled implicitly as a test dependency or explicitly with @build tag.

For first case
Test library used as a source path during test compilation and library classes as well as test classes are compiled into class directory.

For 2nd case
The library classes are compiled using @build tag and library classes are placed into some shared location.

These 2 cases might be mixed in the single test and can have part of library classes compiled into shared directory and part compiled into test classes directory.

jtreg uses classfiles to check if source code should be compiled. Let we have 2 classes LibA and Lib that extends LibA.
So if class LibA might be compiled into test class directory while LibB is compiled into library directory. Later jtreg can try to use LibB class without and compilation because LibB already exists. Thus the CNFE will be thrown.

The another possible problem, is that the library code might include test code for libraries like "/" and "/vmTestbase"
so any test classes with same packages would be compiled into the same location because they are treated as library classes. So for tree like this:
test1/Test.java
test2/Test.java
with both tests having

\@library "/"
\@build Test

we are going just to have one Test class in library directory.

The only reliable fix would be don't use shared class directory at all and compile all libraries for each test. Although it looks like huge performance overhead, the impact is low. The libraries are not often compiled using build tag.

Times for tier1 execution with fix:
test-results/jtreg_test_lib_test_tier1/text/timeStats.txt:Total elapsed time 0m 28s
test-results/jtreg_test_hotspot_jtreg_tier1/text/timeStats.txt:Total elapsed time 18m 14s
test-results/jtreg_test_jdk_tier1/text/timeStats.txt:Total elapsed time 15m 8s
test-results/jtreg_test_langtools_tier1/text/timeStats.txt:Total elapsed time 7m 59s

and before fix
test-results/jtreg_test_lib_test_tier1/text/timeStats.txt:Total elapsed time 0m 32s
test-results/jtreg_test_hotspot_jtreg_tier1/text/timeStats.txt:Total elapsed time 17m 51s
test-results/jtreg_test_jdk_tier1/text/timeStats.txt:Total elapsed time 14m 49s
test-results/jtreg_test_langtools_tier1/text/timeStats.txt:Total elapsed time 7m 56s

The full fix might require more testing and adding testcase.
Please note that there are plans to work on the
https://bugs.openjdk.org/browse/CODETOOLS-7903882
and
https://bugs.openjdk.org/browse/JDK-8346058
So the libraries like '/test/lib' might be precompiled during build without tests and be used as classpath during test execution.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• CODETOOLS-7902847: Class directory of a test case should be always used to compile a library (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jtreg.git pull/256/head:pull/256
$ git checkout pull/256

Update a local copy of the PR:
$ git checkout pull/256
$ git pull https://git.openjdk.org/jtreg.git pull/256/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 256

View PR using the GUI difftool:
$ git pr show -t 256

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jtreg/pull/256.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back lmesnik! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 12: Full (cd30631a)
• 11: Full - Incremental (34372797)
• 10: Full - Incremental (271d58b3)
• 09: Full - Incremental (9dfaf822)
• 08: Full - Incremental (997c061e)
• 07: Full - Incremental (d0b28bff)
• 06: Full - Incremental (4d275a77)
• 05: Full - Incremental (cfe6aecb)
• 04: Full - Incremental (42b188e6)
• 03: Full - Incremental (532981df)
• 02: Full - Incremental (07a65a2d)
• 01: Full - Incremental (d5aca4dc)
• 00: Full (6079e8f2)

[jaikiran]

Hello Leonid,

Not a code review, but a general review about the proposal.

The only reliable fix would be don't use shared class directory at all and compile all libraries for each test.

By all libraries, do you mean all classes that reside under the directory tree of each of the @library that been declared on a specific test definition?

Although it looks like huge performance overhead, the impact is low.

The before/after numbers that you posted for the JDK repo look reasonable. JDK isn't the sole user of jtreg, but yes I think it's the largest user, so we might have to consider if this change needs to be configurable in some way to allow select projects (like the JDK) to opt-in to this new behaviour.

The libraries are not often compiled using build tag.

I have a contradicting understanding of that. In fact, the jtreg documentation here https://openjdk.org/jtreg/tag-spec.html for @library tag strongly recommends using @build for library classes used in the test:

In general, classes in library directories are not automatically compiled as part of a compilation command explicitly naming the source files containing those classes. A test that relies upon library classes should contain appropriate @build directives to ensure that the classes will be compiled. It is strongly recommended that tests do not rely on the use of implicit compilation by the Java compiler. Such an approach is generally fragile, and may lead to incomplete recompilation when a test or library code has been modified.

[lmesnik]

Hello Leonid,

Not a code review, but a general review about the proposal.

The only reliable fix would be don't use shared class directory at all and compile all libraries for each test.

By all libraries, do you mean all classes that reside under the directory tree of each of the @library that been declared on a specific test definition?

Yes,

Although it looks like huge performance overhead, the impact is low.

The before/after numbers that you posted for the JDK repo look reasonable. JDK isn't the sole user of jtreg, but yes I think it's the largest user, so we might have to consider if this change needs to be configurable in some way to allow select projects (like the JDK) to opt-in to this new behaviour.

Hmm, just curious, what are the other users.

The libraries are not often compiled using build tag.

I have a contradicting understanding of that. In fact, the jtreg documentation here https://openjdk.org/jtreg/tag-spec.html for @library tag strongly recommends using @build for library classes used in the test:

In general, classes in library directories are not automatically compiled as part of a compilation command explicitly naming the source files containing those classes. A test that relies upon library classes should contain appropriate @build directives to ensure that the classes will be compiled. It is strongly recommended that tests do not rely on the use of implicit compilation by the Java compiler. Such an approach is generally fragile, and may lead to incomplete recompilation when a test or library code has been modified.

This part of documentation needs to be updated. The main issues is that requirement to use @build directive for each package or class used from library by test is overkill. Engineers prefer to rely on implicit compilation. So we have thousands of implicit usage of libraries in our tests. The explicit build is used usually only if test source doesn't depend on the library classes during compilation. Usually, if they are used by classes running with ProcessTools.

Might be all would work if jtreg disable implicit compilation library dependency and require to cover all classes by build but it overcomplicate tests.

Yes, it is a known and existing problem that "implicit" dependency doesn't call recompilation if library changes only. However, nothing can be done with it right now.

[sormuras]

I also like to see an opt-out switch for this change.

Having at least a system property to enable the old and well-known behaviour, using absBaseClsDir, would be a good fall-back. Such a boolean switch could later be enhanced by either an option on the @library tag, and/or defined in a TEST.ROOT or TEST.properties configuration files.

[mlbridge]

Mailing list message from Jonathan Gibbons on jtreg-dev:

On 3/25/25 10:52 AM, Leonid Mesnik wrote:

The classes from test libraries can be compiled implicitly as a test dependency or explicitly with @build tag.

For first case
Test library used as a source path during test compilation and library classes as well as test classes are compiled into class directory.

Implicit compilation of the library classes into the test class
directory is deplorable and should not be encourages.

For 2nd case
The library classes are compiled using @build tag and library classes are placed into some shared location.

Generally, library classes should be independent of any test classes and
test class directory. As such, @build is the recommended way to build
libraries.

Class directory of a test case should be always used to compile a library

That would be an anti-pattern to be discouraged. Libraries are supposed
to be shared between tests and as such, should not be beholden to any
individual test class directory.

-- Jon

[mlbridge]

Mailing list message from Jonathan Gibbons on jtreg-dev:

It would be a worthwhile audit to examine test class directories after a
test run to see whether any library clases have leaked into those
directories.

A different approach would be to compile test classes with
`-implicit:none` to ensure library classes do not leak into test class
directories.
See
https://docs.oracle.com/en/java/javase/17/docs/specs/man/javac.html#option-implicit

-- Jon

On 4/1/25 12:04 PM, Jonathan Gibbons wrote:

Class directory of a test case should be always used to compile a
library
That would be an anti-pattern to be discouraged. Libraries are
supposed to be shared between tests and as such, should not be
beholden to any individual test class directory.

-- Jon

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/jtreg-dev/attachments/20250401/0c9fea5d/attachment.htm>

[lmesnik]

Thank you for your feedback. See my comments inline.

Mailing list message from Jonathan Gibbons on jtreg-dev:

On 3/25/25 10:52 AM, Leonid Mesnik wrote:

The classes from test libraries can be compiled implicitly as a test dependency or explicitly with @build tag.
For first case
Test library used as a source path during test compilation and library classes as well as test classes are compiled into class directory.

Implicit compilation of the library classes into the test class directory is deplorable and should not be encourages.

Most of tests are doing this already. The people are not going to add build tags if test works without them. I don't think there is a way to encourage people to do this except by disabling implicit compilation. Even I do this once, it will be always the problematic to expect from jtreg test developers to add the build tags in new and update tests.

For 2nd case
The library classes are compiled using @build tag and library classes are placed into some shared location.

Generally, library classes should be independent of any test classes and test class directory. As such, @build is the recommended way to build libraries.

Class directory of a test case should be always used to compile a library

That would be an anti-pattern to be discouraged. Libraries are supposed to be shared between tests and as such, should not be beholden to any individual test class directory.

That's also that is broken for a lot of cases and since it was not restricted, the library tag might refer to mix of library and test code.

So currently we have 2 type of libraries.

1. Shared, test-independent libraries like /test/lib that could be shared as expected
   There is an RFE to support such type of libraries separately. It might be decrease performance of single test execution for single test first time. However, it allow to better control libraries, support projects like Valhalla and also have fully incremented build.
   https://bugs.openjdk.org/browse/CODETOOLS-7903882

2)The test-mixed libraries, like "/" or "vmTesbase/"
that mix test and library code, and might even have same classes into different directories.

Ideally, we'll get rid of them and switch to using only N known libraries, but so far need to support them without any test failures.
So this fix is intended to do. Having feature that allow user to break the execution is not the good idea, better to have performance/disk penalty.
Requirement to always add @build for library classes is going to waste more developers time that I also prefer to avoid.

-- Jon

[RogerRiggs]

Suggested change

	those classes. The library directory is used as additional sourcepath for test compilation.
	So jtreg implicitly compiles all test dependencies that are located in library directory.
	A test that relies upon library classes might contain appropriate
	<code>@build</code> directives to explictily point classes to be compiled.
	This might be useful when test depends on library classes during runtime only.
	those classes. The library directory is used as an additional sourcepath for test compilation.
	So jtreg implicitly compiles all test dependencies that are located in a library directory.
	A test that relies upon library classes might contain appropriate
	<code>@build</code> directives to explicitly compile classes.
	This might be useful when a test depends on library classes during runtime only.

[RogerRiggs]

Where is the "shareLibraries" property defined? I'm expecting to see it defined in the TEST.ROOT or TEST.properties table like "enablePreview", etc.

Also, I think it should not be plural. It applies to this particular library.

Maybe I misunderstand.

[lmesnik]

The flags should be located in TEST.ROOT or TEST.properties for all libraries that are used by tests. It is not specific for any particular library.

This flag is requested by @sormuras to restore original behavior if something goes wrong with new mode.

I would like to keep temporary for a couple of releases and remove if it is not needed. I don't like to maintain different execution modes if it is not needed.

[RogerRiggs]

I was expecting this text in the tag-spec to describe both behavior(s) controlled by shareLibraries.
And mention the property and clearly define the behavior of each case.

That does not seem to be intended. Instead only the changed behavior is documented and the property will not be documented. I understood the feature to control the destination directory for compiled classes but from this description it controls the source path, not the destination.

Since the behavior is being changed somewhat incompatibly, the text should clearly describe the new behavior and call out the differences and how to get the previous behavior if needed.

It would be clearer the property and its behaviors were documented in Locations.getLibLcn .
The name of the property should more clearly indicate it is for compatibility and the behavior when it is set.

The words "might" and "may" are red flags in any specification.

[lmesnik]

The property controls only to where compile put compiled classes for @build tag. There are no changes related to source paths. It remains completely the same and change shouldn't break any test.

The might in this part

A test that relies upon library classes might contain appropriate
<code>@build</code> directives to explicitly compile classes.
This might be useful when a test depends on library classes during runtime only.

just describes when @build tag still is useful useful. It might be not evident that testlibrary classes are not compiled if javac can't understand that they are used.
Also, redundant @build is not an error, but not useful at all.

Does it looks better?

A test that relies upon library classes that  are not implicitly resolved by during compilation should have appropriate
<code>@build</code> directives to explicitly compile required classes.
This is required when a test depends on library classes during runtime only.  

[RogerRiggs]

The only text below that described a change mentions "sourcepath" and does not mention the directories the files are compiled into.

I'm not sure what is being described below except to dispel the idea that @build is necessary.

I don't have better words at the moment but would probably mention classes that were referenced using Reflection or explicitly loaded. But being direct about when @build is necessary will be clearer.

[lmesnik]

I see, the changes in this sentence are done just to more confident describe how the compilation with implicit library works. The actual behavior is not changed.
The library is used as a sourcepath for javac allowing it to pick and compile classes required to compile test source code or build/compiles instructions.
It works exactly as described in the:
https://docs.oracle.com/en/java/javase/17/docs/specs/man/javac.html#additional-source-files
So I just mention how exactly commands are mapped to javac command.

[openjdk]

⚠️ @lmesnik This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[bridgekeeper]

@lmesnik This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[sormuras]

/touch

[openjdk]

@sormuras The pull request is being re-evaluated and the inactivity timeout has been reset.

[sormuras]

Please update this branch to the latest of openjdk/jtreg to include bug fixes and features introduced by lmesnik/jtreg@7902847...openjdk:jtreg:master