Skip to content

set non-overlapping port to faucet in JsonRpcConfig default_for_tests#7741

Closed
puhtaytow wants to merge 1 commit intoanza-xyz:masterfrom
puhtaytow:rpc-default-for-tests-non-overlapping-ports
Closed

set non-overlapping port to faucet in JsonRpcConfig default_for_tests#7741
puhtaytow wants to merge 1 commit intoanza-xyz:masterfrom
puhtaytow:rpc-default-for-tests-non-overlapping-ports

Conversation

@puhtaytow
Copy link
Copy Markdown

Problem

Tests that engage ports binding are flaky. This is continuation of #7736

The JsonRpcConfig default_for_tests doesn't take ports from unique_port_range_for_tests in faucet case.

Summary of Changes

  • JsonRpcConfig::default_for_tests now populate socket with port taken from unique_port_range_for_tests range.

@mergify mergify Bot requested a review from a team August 27, 2025 10:29
@mergify
Copy link
Copy Markdown

mergify Bot commented Aug 27, 2025

If this PR represents a change to the public RPC API:

  1. Make sure it includes a complementary update to rpc-client/ (example)
  2. Open a follow-up PR to update the JavaScript client @solana/kit (example)

Thank you for keeping the RPC clients in sync with the server API @puhtaytow.

@puhtaytow puhtaytow marked this pull request as draft August 27, 2025 10:44
@alexpyattaev alexpyattaev added the CI Pull Request is ready to enter CI label Aug 27, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Aug 27, 2025
@alexpyattaev alexpyattaev requested a review from t-nelson August 27, 2025 11:37
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.0%. Comparing base (48c9d0c) to head (e07e914).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #7741     +/-   ##
=========================================
- Coverage    83.0%    83.0%   -0.1%     
=========================================
  Files         812      812             
  Lines      357065   357069      +4     
=========================================
- Hits       296700   296673     -27     
- Misses      60365    60396     +31     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alexpyattaev alexpyattaev marked this pull request as ready for review August 27, 2025 13:00
@alexpyattaev
Copy link
Copy Markdown

This looks appropriate, but I do not know much about faucets and possible side effects

Comment thread rpc/src/rpc.rs
Comment on lines +208 to +211
faucet_addr: Some(SocketAddr::new(
IpAddr::V4(Ipv4Addr::LOCALHOST),
unique_port_range_for_tests(1).start,
)),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i think you want to address port the issue here instead (may be other places too):

agave/faucet/src/faucet.rs

Lines 346 to 354 in 996570b

// For integration tests. Listens on random open port and reports port to Sender.
pub fn run_local_faucet(faucet_keypair: Keypair, per_time_cap: Option<u64>) -> SocketAddr {
let (sender, receiver) = unbounded();
run_local_faucet_with_port(faucet_keypair, sender, None, per_time_cap, None, 0);
receiver
.recv()
.expect("run_local_faucet")
.expect("faucet_addr")
}

from my understanding, this faucet_addr in this default_for_tests() appears to always get overwritten by some passed in faucet_addr, so setting the socketaddr here won't do anything.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have this PR here #7736 which does address it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It might be strange coincidence, but i observed changes in default_for_tests() having impact on how many tests bind at random high port. I'll investigate more, turning it draft for now. Thank you.

@puhtaytow puhtaytow marked this pull request as draft August 27, 2025 16:33
@puhtaytow puhtaytow closed this Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants