Skip to content

Conversation

@rkampani
Copy link
Collaborator

@rkampani rkampani commented Apr 28, 2025

Fixes Issue

PR Branch
#ADD LINK TO THE PR BRANCH

Motivation and Context

Zerocode generates a Java 8 Jar, but issues when running under higher JDKs have been detected. This PR addresses this:

  • JDK Compatibility: Minor tweaks were made and maven profiles defined to prevent test and build failures.
  • Tested with all current JDK LTSs: 8, 11, 17, 21
  • Jukito to Guice: Swapped Jukito for Guice to ensure compatibility with newer JDKs and ease future upgrades.
  • Type Reference Fixes: Adjusted methods to handle stricter type enforcement in newer JDKs, preventing errors.
  • Gson Optional Support: Added a custom TypeAdapter for java.util.Optional to enable proper JSON serialization/deserialization.

Checklist:

  • New Unit tests were added

    • Covered in existing Unit tests
  • Integration tests were added

    • Covered in existing Integration tests
  • Test names are meaningful

  • Feature manually tested and outcome is successful

  • PR doesn't break any of the earlier features for end users

    • WARNING! This might break one or more earlier earlier features, hence left a comment tagging all reviewrs
  • Branch build passed in CI (tested with Java 8, 11, 17, 21)

  • No 'package.*' in the imports

  • Relevant DOcumentation page added or updated with clear instructions and examples for the end user

    • Not applicable. This was only a code refactor change, no functional or behaviourial changes were introduced
  • Http test added to http-testing module(if applicable) ?

    • Not applicable. The changes did not affect HTTP automation flow
  • Kafka test added to kafka-testing module(if applicable) ?

    • Not applicable. The changes did not affect Kafka automation flow

@authorjapps
Copy link
Owner

Hello Both @rkampani and @thanasissot,
you have been added as a collaborator now. You can join by accepting the invitation here.

This should allow you the trigger builds, review/approve other's PRs etc much more things. 👍

@rkampani rkampani force-pushed the feature/704-fixJunitsJdk24 branch from dd30856 to e6d901c Compare May 1, 2025 01:44
@rkampani
Copy link
Collaborator Author

rkampani commented May 3, 2025

@authorjapps : Here are changes that are addressed in the PR

  • JDK Compatibility: Tests run successfully with reports generated correctly. Minor tweaks were made to prevent test and build failures. However, JDK 9+ (JPMS) imposes stricter module boundaries, affecting reflection and class accessibility. Fully moving to higher JDKs will require updating or replacing legacy Reflection code for compatibility
  • Jukito to Guice: Swapped Jukito for Guice to ensure compatibility with newer JDKs and ease future upgrades.
  • Type Reference Fixes: Adjusted methods to handle stricter type enforcement in newer JDKs, preventing errors.
  • Gson Optional Support: Added a custom TypeAdapter for java.util.Optional to enable proper JSON serialization/deserialization.

if you notice anything that doesn't quite align with the requirements or could potentially introduce issues, please feel free to suggest improvements. If needed, or we can also consider closing it. and If there’s anything I’ve missed or any changes that might lead to issues, I’d really appreciate your feedback and suggestions.

@rkampani rkampani marked this pull request as ready for review May 3, 2025 15:14
@authorjapps
Copy link
Owner

Hello @rkampani , can you please resolve the conflicts as shown above ?
Thanks in advance!

image

@rkampani rkampani force-pushed the feature/704-fixJunitsJdk24 branch from 329f503 to f455ef0 Compare May 9, 2025 13:28
Copy link
Collaborator

@javiertuya javiertuya left a comment

Choose a reason for hiding this comment

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

Thanks @rkampani, great job. I'm happy with the code changes, but:

  • I was curious about the need to run the tests under Java 11 and tested it under Java 8. All tests passed.
  • Then, I tested it with Java 21 and tests failed.
  • You can check this in a temporary project: javiertuya/pr713-zerocode, its main branch contains a copy of this PR's branch. Actions have two runs: (3) with java 8 passes and (4) with java 21 does not pass

@rkampani
Copy link
Collaborator Author

rkampani commented May 12, 2025

@javiertuya: Yes, the tests would failed—based on my understanding, we should consider fully migrating to JPMS. Our test library makes extensive use of reflection, and with JDK 9+ introducing stricter module boundaries, we're encountering class accessibility issues. To address this, we’ll likely need to integrate the byte-buddy library (compatible with Mockito 5.x) and refactor parts of the reflection-based logic. and runtime libraries like Byte Buddy—used by Mockito 5.x—rely on JDK 11+ features (such as MethodHandles or module access), they may fail at runtime when working with classes compiled for JDK 8 due to incompatibility.

In addition, we should update the maven-compiler-plugin in our pom.xml to target a higher JDK version (e.g., JDK 17 or 21). Compiling with JDK 8 (with pom.xml java-compiler entry) while running on JDK 21 can result in bytecode incompatibilities, particularly when using bytecode-manipulating tools like Mockito and Byte Buddy.

I’m happy to take this forward, but wanted to check if there's already a plan in place to move to a newer JDK. and Since many corporate environments are still using JDK 8, I'm not sure whether upgrading the compiler to JDK 21 might have any unintended impacts.
Let me know your thoughts.

By the way, I’ve pushed the changes that should work with runtime JDK 21 or 24. However, I added the following entry in the pom.xml:

--add-opens java.base/java.lang=ALL-UNNAMED
--add-opens java.base/java.lang.reflect=ALL-UNNAMED

This was done to ensure the modules remain compatible with JDK 8 during compilation. In my opinion, this is more of a temporary workaround until we fully migrate to a higher JDK version, at which point there should be no runtime incompatibilities.

@javiertuya : pls re-review and let me know your thoughts.

@javiertuya
Copy link
Collaborator

javiertuya commented May 13, 2025

@rkampani I have checked with all LTS versions here: https://github.com/javiertuya/zerocode/tree/java-matrix. This adds to the workflow: a matrix for testing all versions, -ntp to make logs less verbose and added junit reports that are sent to the archive.

You can check the results in the actions log, the last build is here: https://github.com/javiertuya/zerocode/actions/runs/15005394050

  • Java 17 and 21 pass
  • Java 11 fails a single test (testProduceAnd_wrongFileName). The failure is due to a difference of the strings compared in an assert. If it can be transformed to an assert comparing substrings, it could pass
  • Java 8 fails at startup due to a forked VM crash. Maybe some fine tuning of surefire parameters and/or memory could avoid this crash.

Note:

@javiertuya
Copy link
Collaborator

javiertuya commented May 14, 2025

@rkampani I repeated the tests in the above comment. Now, I removed the --add-opens configuration.

The result summary is:

  • Java 8 and 11 run fine, but fail in a single test (testProduceAnd_wrongFileName)
  • Java 17 and 21 fail four tests (due to module access and reflection issues)

This is my suggestion to finish this PR:

  1. Set Java 8 as the version to run the workflow. Using temuring is fine. I would recommend run maven with -ntp to have cleaner logs
  2. Update the test testProduceAnd_wrongFileName to accept both the Java 8/11 and Java 17/21 required strings (one is a substring the other). Maybe you can use the zerocode $CONTAINS.STRING token
  3. To be able to pass the tests under Java 17 and 21, create a profile with implicit activation that adds a surefire configuration with the --add-opens arguments when running a JVM>=17
  4. I would also suggest squashing some commits to have a cleaner history.

@authorjapps @nirmalchandra @rkampani What are your thoughts on this?

@rkampani rkampani force-pushed the feature/704-fixJunitsJdk24 branch from 407deb3 to 812dcce Compare May 15, 2025 00:57
@rkampani
Copy link
Collaborator Author

rkampani commented May 15, 2025

@javiertuya : Thank you for the feedback! I've updated the PR based on your review comments. I appreciate all the suggestions and the effort you put into running the build and validating it across different Java versions.

Please take another look when you get a chance and let me know your thoughts. Thanks again!


With the above changes, CI stats Looks Green 🟢 .

    strategy:
      matrix:
        version: [8, 11, 17, 21]

Stats:

Pasted Graphic 10

Copy link
Collaborator

@javiertuya javiertuya left a comment

Choose a reason for hiding this comment

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

Thanks @rkampani. I think this is ready to merge after resolving a few comments.

I've updated the PR description and filled the checklist

@authorjapps Can I merge it or do we need more feedback?

@mailtoach79
Copy link

Hi @javiertuya, @rkampani , Many thanks for looking into this and fixing this. Please give me sometime(couple of days) to test this in my machine using Java21/23. I will let you know the feedback asap.

(I am aware, I will have to clone the PR branch)
Is there anything else I have to do or change in my laptop, in order to run in Java21/23?

@javiertuya
Copy link
Collaborator

@mailtoach79

(I am aware, I will have to clone the PR branch) Is there anything else I have to do or change in my laptop, in order to run in Java21/23?

  • If you run the zerocode tests, you should not have to do anything, the profiles set the appropriate configuration for higher JDK versions
  • If you are using zerocde to test your own API, you may need to add some configuration to surefire (see the profiles in the parent pom in this PR's branch).

@authorjapps
Copy link
Owner

authorjapps commented May 16, 2025

Hello @mailtoach79 , Thanks for picking it up from here.

Please let us know how it went:

  • Running the Zerocode
  • Running with the Zerocode (using a SNAPSHOT on another project)

i.e.

  • Running the zerocode tests (the Kafka tests you were stuck on I suppose, as per the issue)
  • Running a test with Zerocode as a dependency (if you were able to do that as well)

Also, if you need any help or get stuck during the testing, feel free to give a shout here.

Cheers!

@mailtoach79
Copy link

mailtoach79 commented May 17, 2025

@rkampani Test validated using Java 23, and its successfull in running Kafka test in intellij.
Following is the output of from intellij console.

"C:\Program Files\Java\jdk-23\bin\java.exe" -ea --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.lang.reflect=ALL-UNNAMED -"
....
Process finished with exit code 0

But from the normal terminal, where I have JDK 21 configured,

$ java --version
java 21.0.6 2025-01-21 LTS
Java(TM) SE Runtime Environment (build 21.0.6+8-LTS-188)

Then I ran the test using mvn test -Dtest=...

its showing something related to 1.8, which is bit confusing and test is successful.

[INFO] Compiling 41 source files with javac [debug target 1.8] to target\test-classes
[WARNING] bootstrap class path not set in conjunction with -source 8
[WARNING] source value 8 is obsolete and will be removed in a future release
[WARNING] target value 8 is obsolete and will be removed in a future release

Anyway I'm unblocked now, tests working fine using Java 23 successfully 🟢 in local. so I am not sure about the above outputs , does it really matter to me or not.

Thanks for your support. plesae let me know if anything you want me to chang and run again.

@rkampani
Copy link
Collaborator Author

@mailtoach79 : Thank you for the validation. imho, Yes, these warnings can be safely ignored for now, as:

  • They don’t impact the functionality of the build or the tests.
  • The code compiles and runs correctly, as confirmed by the successful test.
  • Java 21 continues to support compiling and running Java 8-compatible code, so there's no immediate issue.

The warnings are simply a heads-up that Java 8 compatibility mode (-source 8 and -target 8) is deprecated and may eventually be removed in future Java versions. If the warnings are bothersome, we could configure the Maven Compiler Plugin to suppress them while still maintaining Java 8 compatibility. That said, I’d prefer to let the team @authorjapps / @javiertuya weigh in on whether we should do that. Personally, I think it’s a helpful reminder that we’ll need to upgrade at some point in the future.

@authorjapps
Copy link
Owner

Thanks to:

  • @mailtoach79 – for the compatibility tests, awesome mate! Glad to know you're unblocked .
  • @rkampani – for the fixes; amazing work !
  • @javiertuya – for all your timely support in resolving this, excellent findings !

Please go ahead and merge this when you get a chance 👍


Next Step >>

@javiertuya javiertuya merged commit 39cfd6e into authorjapps:master May 19, 2025
1 check passed
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.

Error in running the zerocode tests in JDK23/21

4 participants