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

Fix CI unit tests #5196

Merged
merged 1 commit into from
Jan 28, 2025
Merged

Fix CI unit tests #5196

merged 1 commit into from
Jan 28, 2025

Conversation

oleks-rip
Copy link
Collaborator

@oleks-rip oleks-rip commented Nov 18, 2024

High Level Overview of Change

MACOS unit tests fix

Context of Change

Add retries to Env RPC requests to fix Macos disconnects during CI testing.
RPC server use ip ports assigned by operating system automatically (instead of hard-coded) when unit tests run.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

API Impact

  • No API impact

Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 92.20779% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.0%. Comparing base (b6e3453) to head (e51876d).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/core/detail/Config.cpp 88.2% 2 Missing ⚠️
src/xrpld/rpc/detail/ServerHandler.cpp 85.7% 2 Missing ⚠️
src/xrpld/app/main/Application.cpp 93.8% 1 Missing ⚠️
src/xrpld/app/main/GRPCServer.cpp 93.3% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5196   +/-   ##
=======================================
  Coverage     78.0%   78.0%           
=======================================
  Files          789     789           
  Lines        67007   67060   +53     
  Branches      8107    8109    +2     
=======================================
+ Hits         52278   52321   +43     
- Misses       14729   14739   +10     
Files with missing lines Coverage Δ
include/xrpl/basics/BasicConfig.h 87.2% <ø> (ø)
include/xrpl/server/detail/ServerImpl.h 92.5% <100.0%> (+0.8%) ⬆️
src/libxrpl/basics/BasicConfig.cpp 89.6% <100.0%> (ø)
src/libxrpl/server/Port.cpp 76.8% <100.0%> (+0.2%) ⬆️
src/xrpld/app/main/GRPCServer.h 100.0% <ø> (ø)
src/xrpld/overlay/detail/OverlayImpl.cpp 34.7% <100.0%> (ø)
src/xrpld/rpc/ServerHandler.h 100.0% <100.0%> (ø)
src/xrpld/app/main/Application.cpp 69.3% <93.8%> (+0.4%) ⬆️
src/xrpld/app/main/GRPCServer.cpp 44.4% <93.3%> (+2.0%) ⬆️
src/xrpld/core/detail/Config.cpp 75.2% <88.2%> (+0.5%) ⬆️
... and 1 more

... and 2 files with indirect coverage changes

Impacted file tree graph

@oleks-rip oleks-rip force-pushed the mac_test2 branch 2 times, most recently from e6e8628 to 6799369 Compare November 22, 2024 00:06
@oleks-rip oleks-rip marked this pull request as ready for review November 22, 2024 01:19
@oleks-rip oleks-rip force-pushed the mac_test2 branch 2 times, most recently from 8332099 to b6c13b7 Compare November 26, 2024 15:06
@vlntb vlntb self-requested a review December 4, 2024 16:14
Copy link
Collaborator

@vlntb vlntb left a comment

Choose a reason for hiding this comment

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

This is an elegant solution to rely on the system for dynamic port resolution.
I'm happy to approve this PR after a minor change.

Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

I started this review, but I didn't post it, and I'm not sure if I finished. Consider it a partial review.

@oleks-rip
Copy link
Collaborator Author

oleks-rip commented Dec 7, 2024

I started this review, but I didn't post it, and I'm not sure if I finished. Consider it a partial review.

It is not considered finished until approved, feel free to make breaks among your reviews.

@ximinez ximinez self-requested a review December 19, 2024 19:02
@bthomee bthomee requested a review from vlntb January 2, 2025 23:50
@oleks-rip oleks-rip force-pushed the mac_test2 branch 2 times, most recently from 4918210 to 05b1f13 Compare January 7, 2025 17:50
@oleks-rip oleks-rip force-pushed the mac_test2 branch 3 times, most recently from 37982f2 to 3acdb64 Compare January 21, 2025 19:44
@ximinez ximinez self-requested a review January 23, 2025 18:25
@oleks-rip oleks-rip force-pushed the mac_test2 branch 2 times, most recently from 8076e5d to 225aa8a Compare January 24, 2025 15:08
Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Looks like all my concerns have been addressed. Really looking forward to getting those CI jobs reliable again.

Two things:

  1. When you're updating from develop on a non-draft PR, please merge instead of rebasing. Using the "Show changes since your last review" doesn't work if you remerge, which makes it unnecessarily difficult to find updates.
  2. Please don't resolve comments that I (and others?) leave in reviews, unless Github does it automatically (i.e. through the "Accept Suggestion" button). Again, it makes it a lot easier for me to follow up on my previous feedback when it may have been long enough that I don't remember precisely what I said, or where I said it.

@oleks-rip
Copy link
Collaborator Author

oleks-rip commented Jan 27, 2025

  1. Please don't resolve comments that I (and others?) leave in reviews, unless Github does it automatically (i.e. through the "Accept Suggestion" button). Again, it makes it a lot easier for me to follow up on my previous feedback when it may have been long enough that I don't remember precisely what I said, or where I said it.

After some numbers of open conversations GH going crazy and makes review impossible. It will randomly drop you to some another comment when you just reading something. When you scroll - it will shows you some random previous comments. When you reply it can redirect you replying to another comment. It became slow and unresponsive. (I observe this on different machines, OS and browsers, it is not my cache)

Copy link
Collaborator

@vlntb vlntb left a comment

Choose a reason for hiding this comment

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

All comments addressed. LGTM now.

@oleks-rip oleks-rip added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jan 27, 2025
@ximinez
Copy link
Collaborator

ximinez commented Jan 27, 2025

After some numbers of open conversations GH going crazy and makes review impossible. It will randomly drop you to some another comment when you just reading something. When you scroll - it will shows you some random previous comments. When you reply it can redirect you replying to another comment. It became slow and unresponsive. (I observe this on different machines, OS and browsers, it is not my cache)

Weeeeeird.

- Add retries for rpc client
- Add dynamic port allocation for rpc servers
@ximinez ximinez changed the title Mac UT check Fix CI unit tests Jan 28, 2025
@ximinez ximinez merged commit 50b8f19 into XRPLF:develop Jan 28, 2025
22 checks passed
q73zhao pushed a commit that referenced this pull request Feb 24, 2025
- Add retries for rpc client
- Add dynamic port allocation for rpc servers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants