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.

Sign up for GitHub

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

Jump to bottom

chore: improvements for test repeatability #766

Merged
merged 1 commit into from
Mar 11, 2025
Merged

chore: improvements for test repeatability #766

merged 1 commit into from
Mar 11, 2025
+241 −84

Conversation

AlexKehayov
Copy link
Contributor

@AlexKehayov AlexKehayov commented Mar 4, 2025 •
edited by AlfredoG87
Loading

Reviewer Notes

  • Added a custom BlockingExecutorService to ensure async logic in tests executes and completes before assertions begin.
  • Used CountDownLatch technique to synchronize execution in cases where BlockingExecutorService cannot be properly injected.
  • Modified PbjBlockStreamServiceProxy to enable custom executor injection for test purposes.

Related Issue(s)

Fixes #642

@georgi-l95 georgi-l95 added this to the 0.7.0 milestone Mar 4, 2025
@AlexKehayov AlexKehayov force-pushed the 642-chore-improvements-for-test-repeatability branch 2 times, most recently from 0c3b0fe to 2a16747 Compare March 4, 2025 09:11
AlexKehayov
AlexKehayov commented Mar 4, 2025
View reviewed changes
Copy link
Contributor Author

@AlexKehayov AlexKehayov left a comment

Choose a reason for hiding this comment

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

self-review

@AlexKehayov AlexKehayov marked this pull request as ready for review March 4, 2025 10:11
@AlexKehayov AlexKehayov requested review from a team as code owners March 4, 2025 10:11
@jsync-swirlds jsync-swirlds added the pull request label to get past the "label required" check when no label is needed or appropriate. label Mar 4, 2025
jsync-swirlds
jsync-swirlds reviewed Mar 5, 2025
View reviewed changes
Copy link
Contributor

@jsync-swirlds jsync-swirlds left a comment

Choose a reason for hiding this comment

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

The work here is very much appreciated.
I do have a question on the executor; it might be a bit more complex than necessary, and I'm not sure the count down is entirely correct.

@AlfredoG87
Copy link
Contributor

Please rebase with latest main so we have an actual PR Code Coverage report and also for the other CI Checks to pass.

@AlexKehayov AlexKehayov force-pushed the 642-chore-improvements-for-test-repeatability branch from 2a16747 to 8e2b88a Compare March 5, 2025 08:53
AlexKehayov added a commit to AlexKehayov/hedera-block-node that referenced this pull request Mar 5, 2025
@AlexKehayov
chore: improvements for test repeatability (hiero-ledger#766)
ab3259d 
Signed-off-by: Alex Kehayov <[email protected]>
@AlexKehayov AlexKehayov force-pushed the 642-chore-improvements-for-test-repeatability branch from 471e01b to ab3259d Compare March 5, 2025 10:26
@AlexKehayov AlexKehayov requested review from a team as code owners March 5, 2025 10:32
@jsync-swirlds
Copy link
Contributor

The merge modified pre-existing commits (possibly due to an out-of-date main).
It might be helpful to back up and then rebase instead.

AlexKehayov added a commit to AlexKehayov/hedera-block-node that referenced this pull request Mar 6, 2025
@AlexKehayov
chore: improvements for test repeatability (hiero-ledger#766)
ad9cbb1 
Signed-off-by: Alex Kehayov <[email protected]>
@AlexKehayov AlexKehayov force-pushed the 642-chore-improvements-for-test-repeatability branch from 1f967a0 to ad9cbb1 Compare March 6, 2025 14:34
@AlexKehayov
Copy link
Contributor Author

I made some modifications to PbjBlockStreamServiceIntegrationTest and PbjBlockStreamServiceProxy to align with the changes from #715 and #760. The branch has been rebased on main, and the previous issue should now be resolved.

jsync-swirlds
jsync-swirlds previously approved these changes Mar 6, 2025
View reviewed changes
AlfredoG87
AlfredoG87 previously approved these changes Mar 7, 2025
View reviewed changes
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, however still needs a rebase from main and also conflicts resolutions 😓

AlexKehayov added a commit to AlexKehayov/hedera-block-node that referenced this pull request Mar 7, 2025
@AlexKehayov
chore: improvements for test repeatability (hiero-ledger#766)
df5ad46 
Signed-off-by: Alex Kehayov <[email protected]>
@AlexKehayov AlexKehayov force-pushed the 642-chore-improvements-for-test-repeatability branch from ad9cbb1 to df5ad46 Compare March 7, 2025 08:59
jsync-swirlds
jsync-swirlds previously approved these changes Mar 7, 2025
View reviewed changes
AlfredoG87
AlfredoG87 previously approved these changes Mar 7, 2025
View reviewed changes
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
Copy link
Contributor

Need rebase to latest main version and conflict resolution. once is done, I can re-approve.

AlexKehayov added a commit to AlexKehayov/hedera-block-node that referenced this pull request Mar 10, 2025
@AlexKehayov
chore: improvements for test repeatability (hiero-ledger#766)
342abb3 
Signed-off-by: Alex Kehayov <[email protected]>
@AlexKehayov AlexKehayov dismissed stale reviews from AlfredoG87 and jsync-swirlds via 342abb3 March 10, 2025 13:43
@AlexKehayov AlexKehayov force-pushed the 642-chore-improvements-for-test-repeatability branch from df5ad46 to 342abb3 Compare March 10, 2025 13:43
ata-nas
ata-nas approved these changes Mar 10, 2025
View reviewed changes
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.

LG

@jsync-swirlds
Copy link
Contributor

@AlfredoG87 The PR title check fails for some reason (not clear why). I believe you can still merge this?

@Nana-EC
Copy link
Contributor

Nana-EC commented Mar 11, 2025

Looking into failing PR check blocker

@nathanklick nathanklick added the Block Node Issues/PR related to the Block Node. label Mar 11, 2025
@Nana-EC
Copy link
Contributor

Nana-EC commented Mar 11, 2025

@AlexKehayov kindly rebase, I just pushed a PR #820 that should help with the GHA that was stuck here

@AlexKehayov AlexKehayov force-pushed the 642-chore-improvements-for-test-repeatability branch from 342abb3 to 14f1d62 Compare March 11, 2025 13:01
@Nana-EC Nana-EC merged commit 5138091 into hiero-ledger:main Mar 11, 2025
13 of 15 checks passed
@codecov Codecov
Copy link

codecov bot commented Mar 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

@@            Coverage Diff            @@
##               main     #766   +/-   ##
=========================================
  Coverage     89.12%   89.12%           
- Complexity      707      710    +3     
=========================================
  Files           130      130           
  Lines          3043     3045    +2     
  Branches        219      219           
=========================================
+ Hits           2712     2714    +2     
  Misses          265      265           
  Partials         66       66
Files with missing lines Coverage Δ Complexity Δ
...a/block/server/pbj/PbjBlockStreamServiceProxy.java 80.30% <100.00%> (+0.30%) 27.00 <5.00> (+3.00)
🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AlexKehayov AlexKehayov deleted the 642-chore-improvements-for-test-repeatability branch March 11, 2025 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgi-l95 georgi-l95 georgi-l95 approved these changes

@ata-nas ata-nas ata-nas approved these changes

@Nana-EC Nana-EC Awaiting requested review from Nana-EC

@mattp-swirldslabs mattp-swirldslabs Awaiting requested review from mattp-swirldslabs

@jsync-swirlds jsync-swirlds Awaiting requested review from jsync-swirlds

@AlfredoG87 AlfredoG87 Awaiting requested review from AlfredoG87

Assignees

@AlexKehayov AlexKehayov

Labels
Block Node Issues/PR related to the Block Node. pull request label to get past the "label required" check when no label is needed or appropriate.
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.

chore: improvements for test repeatability
7 participants
@AlexKehayov @AlfredoG87 @jsync-swirlds @Nana-EC @georgi-l95 @ata-nas @nathanklick