Skip to content

Conversation

jsync-swirlds
Copy link
Contributor

@jsync-swirlds jsync-swirlds commented Oct 1, 2025

Reviewer Notes

This PR is a vehicle for clearer discussion of some recommended changes that we were struggling to discuss in a previous PR.

  • Renamed sessions to remove redundant "Verification" and "Block" terms.
  • Renamed session factory to include criteria in the class name instead of redundant words
  • Added some message variables and cleaned up excessive wrapping
  • Removed exception-as-goto usage in VerificationServicePlugin; replaced with if/else usage.
  • Moved failure notification to a new method for consistency.
  • Applied spotless which further adjusted wrapping

@jsync-swirlds jsync-swirlds added this to the 0.21.0 milestone Oct 1, 2025
@jsync-swirlds jsync-swirlds self-assigned this Oct 1, 2025
@jsync-swirlds jsync-swirlds added the Verification Plugin Issue related to Verification Plugin label Oct 1, 2025
@jsync-swirlds jsync-swirlds force-pushed the rename-and-adjust-session-selection branch 3 times, most recently from 9c9bb67 to 10e3d14 Compare October 2, 2025 01:33
@jsync-swirlds jsync-swirlds requested a review from Nana-EC October 2, 2025 01:34
@jsync-swirlds jsync-swirlds force-pushed the rename-and-adjust-session-selection branch from 10e3d14 to 2c47a33 Compare October 14, 2025 20:02
@jsync-swirlds jsync-swirlds marked this pull request as ready for review October 14, 2025 20:07
@jsync-swirlds jsync-swirlds requested review from a team as code owners October 14, 2025 20:07
ata-nas
ata-nas previously approved these changes Oct 15, 2025
Copy link
Contributor

@ata-nas ata-nas left a comment

Choose a reason for hiding this comment

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

Looks good, I like the shorter and improved naming. 👍

Copy link
Contributor

@mustafauzunn mustafauzunn left a comment

Choose a reason for hiding this comment

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

Build is failing because of ExtendedMerkleTreeVerificationSessionV0680 usage in ExtendedMerkleTreeVerificationSessionV0680Test

@jsync-swirlds
Copy link
Contributor Author

jsync-swirlds commented Oct 15, 2025

Build is failing because of ExtendedMerkleTreeVerificationSessionV0680 usage in ExtendedMerkleTreeVerificationSessionV0680Test

Merge error, will fix Fixed.

* Renamed sessions to remove redundant "Verification" and "Block" terms.
* Renamed session factory to include criteria in the class name instead of redundant words
* Added some message variables and cleaned up excessive wrapping
* Removed exception-as-goto usage in VerificationServicePlugin; replaced with if/else usage.
* Moved failure notification to a new method for consistency.
* Applied spotless which further adjusted wrapping

Signed-off-by: Joseph S <[email protected]>
@jsync-swirlds jsync-swirlds force-pushed the rename-and-adjust-session-selection branch from 2eec807 to 7bdc226 Compare October 15, 2025 16:35
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 85.41667% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...k/node/verification/VerificationServicePlugin.java 83.33% 7 Missing ⚠️
@@             Coverage Diff              @@
##               main    #1706      +/-   ##
============================================
+ Coverage     79.97%   80.04%   +0.06%     
  Complexity     1144     1144              
============================================
  Files           125      125              
  Lines          5333     5331       -2     
  Branches        566      567       +1     
============================================
+ Hits           4265     4267       +2     
+ Misses          817      811       -6     
- Partials        251      253       +2     
Files with missing lines Coverage Δ Complexity Δ
...erification/session/HapiVersionSessionFactory.java 90.00% <100.00%> (ø) 10.00 <0.00> (?)
...cation/session/impl/ExtendedMerkleTreeSession.java 86.66% <100.00%> (ø) 2.00 <0.00> (?)
...ication/session/impl/PreviewSimpleHashSession.java 93.54% <100.00%> (ø) 11.00 <0.00> (?)
...k/node/verification/VerificationServicePlugin.java 90.32% <83.33%> (+2.95%) 14.00 <2.00> (+2.00)

... and 1 file with indirect coverage changes

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

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

Labels

Verification Plugin Issue related to Verification Plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants