Skip to content

build: run tests on module path #1104

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

build: run tests on module path #1104

wants to merge 7 commits into from

Conversation

jjohannes
Copy link
Contributor

@jjohannes jjohannes commented May 9, 2025

Description:

Update hiero-gradle-conventions to 0.4.1 and by that run tests on the module path.

See hiero-ledger/hiero-consensus-node#18922 for more details on how that works.

Blocked until hcn 0.63.0 is released: Need to wait for next hcn (swirlds version) release as patched module name io.prometheus.simpleclient was changed to simpleclient which is the name expected by module-info.class files published in helidon modules.

@jjohannes jjohannes self-assigned this May 9, 2025
@jjohannes jjohannes force-pushed the run-on-module-path branch from 8195d82 to 6a6e588 Compare May 9, 2025 07:50
@jjohannes jjohannes changed the title Run tests on module path build: run tests on module path May 9, 2025
@jjohannes jjohannes force-pushed the run-on-module-path branch 3 times, most recently from 5d5ddb3 to 6a6f8c3 Compare May 9, 2025 14:13
@jjohannes jjohannes added this to the 0.13.0 milestone May 9, 2025
@jjohannes jjohannes added the CI/CD Issues related to the CI/CD. label May 9, 2025
@jjohannes jjohannes force-pushed the run-on-module-path branch 3 times, most recently from e547f44 to 2891aa3 Compare May 22, 2025 17:58
jjohannes added 6 commits May 23, 2025 07:39
Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>
Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>
Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>
Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>
Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>
Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>
@jjohannes jjohannes force-pushed the run-on-module-path branch from fcc2026 to ad05e5d Compare May 23, 2025 05:39
@jjohannes
Copy link
Contributor Author

@AlfredoG87 I figured that we temporarily can add patch code for com.swirlds.common to workaround the renaming issue until 0.63 is released. Then this does not block the other updates.

@jjohannes
Copy link
Contributor Author

@AlfredoG87 @jasperpotts With moving tests to the Module Path, the test BlockNodeAppTest.testMain() behaves different. The service registry is now available at test time. Also, loading one of a duplicated resource in the root of the module/class path behaves different. It is an unstable thing to rely on in any case. There is a duplication with app/src/main/resources/app.properties and app/src/test/resources/app.properties. I propose two changes, but please let me know if we should solve this differently:

@jjohannes jjohannes marked this pull request as ready for review May 23, 2025 05:52
@jjohannes jjohannes requested review from a team as code owners May 23, 2025 05:52
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also consider suppressing the test generation by PBJ so that we do not create or run those tests at all (it is an option in the PBJ compiler now).
Those tests verify that PBJ is correct, which isn't particularly necessary to run here.

@AlfredoG87
Copy link
Contributor

@AlfredoG87 @jasperpotts With moving tests to the Module Path, the test BlockNodeAppTest.testMain() behaves different. The service registry is now available at test time. Also, loading one of a duplicated resource in the root of the module/class path behaves different. It is an unstable thing to rely on in any case. There is a duplication with app/src/main/resources/app.properties and app/src/test/resources/app.properties. I propose two changes, but please let me know if we should solve this differently:

I agree with the approach 👍

Copy link
Contributor

@AlfredoG87 AlfredoG87 left a comment

Choose a reason for hiding this comment

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

LGTM

AlfredoG87
AlfredoG87 previously approved these changes May 29, 2025
@jjohannes jjohannes force-pushed the run-on-module-path branch from dd5aecd to 53f31e9 Compare June 2, 2025 07:09
@jjohannes jjohannes requested a review from AlfredoG87 June 2, 2025 07:34
Copy link

codecov bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...in/java/org/hiero/block/node/app/BlockNodeApp.java 75.00% 0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (75.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

@@             Coverage Diff              @@
##               main    #1104      +/-   ##
============================================
+ Coverage     84.33%   84.41%   +0.07%     
- Complexity      915      941      +26     
============================================
  Files           105      107       +2     
  Lines          3786     3901     +115     
  Branches        415      424       +9     
============================================
+ Hits           3193     3293     +100     
- Misses          423      431       +8     
- Partials        170      177       +7     
Files with missing lines Coverage Δ Complexity Δ
...in/java/org/hiero/block/node/app/BlockNodeApp.java 88.18% <75.00%> (+5.12%) 13.00 <0.00> (+2.00)

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@AlfredoG87 AlfredoG87 left a comment

Choose a reason for hiding this comment

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

LG

@@ -14,7 +14,8 @@
// export configuration classes to the config module
exports org.hiero.block.node.app to
com.swirlds.config.impl,
com.swirlds.config.extensions;
com.swirlds.config.extensions,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit concerning, as this module both requires and exports to the com.swirlds.config.extensions module.

As noted above, this module really should not be exporting anything, so we probably need to fix whatever is needing us to export here.

Outside the scope of this PR, but I've filed an issue to investigate and resolve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Issues related to the CI/CD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants