Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add java21 support #2640

Merged
merged 2 commits into from
Sep 25, 2024
Merged

chore: add java21 support #2640

merged 2 commits into from
Sep 25, 2024

Conversation

Vovchyk
Copy link
Contributor

@Vovchyk Vovchyk commented Jul 24, 2024

Description

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Tests for the changes have been added (for bug fixes / features)
  • Requires Activation Code (Hard Fork)

fed:vovchyk/java21-support

@Vovchyk Vovchyk force-pushed the vovchyk/java21-support branch 5 times, most recently from 482470b to 026ce0b Compare July 24, 2024 14:34
- name: Setup Java JDK
uses: actions/setup-java@v3
- name: Setup Java & Gradle
uses: actions/setup-java@v4
with:
java-version: '17'
Copy link
Contributor

Choose a reason for hiding this comment

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

should not be updated to 21 aswell?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting question.. I guess we should be targeting java17 as more stable one. But perhaps it'd make sense to run unit tests against both 17 and 21. Added that in this 1ddcfd7

asoto-iov
asoto-iov previously approved these changes Jul 25, 2024

class MaxSizeHashMapTest {

@Test
void maxSizeMap_Test() {
int maxSize = 50_000;
void maxSizeMapTest() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this test updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are couple of reasons: first, this test tries to "test" the feature not as a "blackbox", but an internal state of the instance of a system class. Secondly and mosts importantly, this test uses reflection to obtain an internal field of the system class (via field.setAccessible(true)), which is prohibited for non-open java packages in recent versions of junit. There's a workaround by opening system package (via --add-opens java.base/java.util=ALL-UNNAMED), but I'm not sure that worth it

Copy link

sonarcloud bot commented Jul 30, 2024

@Vovchyk Vovchyk changed the base branch from vovchyk/min-java-ver-bump to master September 25, 2024 11:56
@Vovchyk Vovchyk dismissed asoto-iov’s stale review September 25, 2024 11:56

The base branch was changed.

Copy link

sonarcloud bot commented Sep 25, 2024

@Vovchyk Vovchyk merged commit 2a0d333 into master Sep 25, 2024
11 checks passed
@Vovchyk Vovchyk deleted the vovchyk/java21-support branch September 25, 2024 13:04
@aeidelman aeidelman added this to the Arrowhead 6.4.0 milestone Oct 30, 2024
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.

3 participants