Skip to content

Comments

split tests up into separate functions#183

Open
philippem wants to merge 4 commits intoBlockstream:new-indexfrom
philippem:split-tests-up-v2
Open

split tests up into separate functions#183
philippem wants to merge 4 commits intoBlockstream:new-indexfrom
philippem:split-tests-up-v2

Conversation

@philippem
Copy link
Collaborator

@philippem philippem commented Feb 19, 2026

  • split REST tests up
  • split electrum tests up
  • expand REST tests
  • make port allocation more robust for test runner
  • address lifetime/construction error in request handling

@philippem philippem requested a review from RCasatta February 19, 2026 15:48
@Randy808
Copy link

Randy808 commented Feb 19, 2026

ACK d3e629f

Tested on regtest

rpc_threads.install() was wrapping the lazy iterator *construction*
in requests_iter(), but parallel work only executes when the iterator
is consumed (.collect()). By the time collect() ran in requests(),
the install scope had ended and Rayon dispatched onto the global pool.

Move install() to wrap the collect() call in requests() (and the
equivalent in get_transactions()), so the actual parallel work — and
the per-thread DAEMON_INSTANCE TCP connections — run on the dedicated
rpc_threads pool as intended.

NOTE: this fix is required for parallel integration tests to work
correctly. Without it, concurrent tests share global-pool threads
whose DAEMON_INSTANCE thread-locals were initialised by a different
test's daemon, causing cross-test RPC connection pollution.
tests/rest.rs Outdated

// Verify status on the TransactionValue itself
assert_eq!(res["status"]["confirmed"].as_bool(), Some(true));
assert_eq!(res["status"]["block_height"].as_u64(), Some(102));

Choose a reason for hiding this comment

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

If test setup changes these hardcoded heights could break.. could maybe call get_block_count() like the reorg test does:

let init_height = tester.node_client().get_block_count()?;

RCasatta
RCasatta previously approved these changes Feb 20, 2026
Copy link
Collaborator

@RCasatta RCasatta left a comment

Choose a reason for hiding this comment

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

ACK 977047c code review, tested locally

I would add the lock suggestion while the other is just a nit

tests/common.rs Outdated
loop {
let socket = net::TcpListener::bind("127.0.0.1:0").unwrap();
let addr = socket.local_addr().unwrap();
if USED_PORTS.lock().unwrap().insert(addr.port()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

lock should be taken before the socket binding to really avoid race conditions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed in amended commit

- Use TcpListener instead of UdpSocket for port allocation to avoid
  races between parallel test instances; track allocated ports in a
  process-wide HashSet to prevent reuse within the same test run
- Poll the REST and Electrum servers until they accept connections
  before returning, eliminating timing-dependent startup
Break test_rest() into individual test functions, each covering a
specific REST endpoint, with assertions on concrete field values
rather than just shape checks.
Break test_electrum() into individual test functions (balance,
history, payment, raw), each covering a specific scenario, and
extract shared setup into a WalletTester struct.
@philippem philippem force-pushed the split-tests-up-v2 branch 2 times, most recently from fa8fd04 to 19ab74c Compare February 20, 2026 21:35
@philippem philippem requested a review from shesek February 21, 2026 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants