Skip to content

Add capability to build and test with JDK up to version 17 (fixes #127)#128

Open
davoustp wants to merge 1 commit intomjiderhamn:masterfrom
davoustp:#127
Open

Add capability to build and test with JDK up to version 17 (fixes #127)#128
davoustp wants to merge 1 commit intomjiderhamn:masterfrom
davoustp:#127

Conversation

@davoustp
Copy link

This provides the capability to build and test against recent JDK versions (tested up with JDK 17).

  • JDK source and target are now set to 1.7 as this is the minimal version supported by JDK 17
  • All appropriate --add-opens have been added using a maven profile with activation based on JDK version (9+) so that it still compiles with prior versions (JDK 7 and 8)
  • Add support for org.junit.Assume in classloader-leak-test-framework to allow tests to be conditionally ignored based on JDK version
  • Add JDK version conditions for tests which leak condition have been fixed in later versions

Note that dependency from classloader-leak-prevention onto classloader-leak-test-framework is left as SNAPSHOT in this PR since this requires releasing a new version (which is obviously not possible using a PR).

@davoustp davoustp changed the title Add capability to build and test with JDK up to version 17 (fixed #127) Add capability to build and test with JDK up to version 17 (fixes #127) Jan 10, 2022
@Override
protected void triggerLeak() throws Exception {
// Leak does not occur any more with JDK11+
Assume.assumeTrue(JavaVersion.IS_JAVA_10_OR_EARLIER);
Copy link
Owner

Choose a reason for hiding this comment

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

Why isn't triggerLeakWithoutCleanup() overridden in this case?

Copy link
Author

Choose a reason for hiding this comment

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

Good question - I may have overlooked this one...

@Override
public void triggerLeakWithoutCleanup() throws Exception {
// Leak does not occur any more with JDK8+
Assume.assumeTrue(JavaVersion.IS_JAVA_1_6 || JavaVersion.IS_JAVA_1_7);
Copy link
Owner

Choose a reason for hiding this comment

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

Would it have made more sense to make the original triggerLeakWithoutCleanup() do something like Assume.assumeTrue(isRelevantForJavaVersion()) with a default implementation of isRelevantForJavaVersion() that returns true, and then override that method in the respective tests? Feels like it would be more readable?

Plus, isRelevantForJavaVersion() could be named the same for both CleanUp tests and PreInitiator tests.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed.

<configuration>
<argLine>
--add-opens java.base/sun.reflect.misc=ALL-UNNAMED
--add-opens java.base/sun.reflect.misc=ALL-UNNAMED
Copy link
Owner

Choose a reason for hiding this comment

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

Duplicate

Copy link
Author

Choose a reason for hiding this comment

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

True


forceGc(3);

if (assumptionFailed) {
Copy link
Owner

Choose a reason for hiding this comment

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

Do we want to do this before forcing garbage collection 3 times, for better performance?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, makes sense.

<dependency>
<groupId>org.geotools</groupId>
<artifactId>gt-metadata</artifactId>
<version>2.6.2</version>
Copy link
Owner

Choose a reason for hiding this comment

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

Is this actualy working for you, @davoustp ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it is.
But the gt-metadata artifact has moved to another repo, and not available in public Maven: http://maven.geo-solutions.it/
I struggled quite a bit to find this one, to be honest. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants