Skip to content

test: make explicit returned stdlib types in tests#604

Merged
tcharding merged 1 commit into
rust-bitcoin:masterfrom
alexgrad42:feature/add_explicit_return_type
May 27, 2026
Merged

test: make explicit returned stdlib types in tests#604
tcharding merged 1 commit into
rust-bitcoin:masterfrom
alexgrad42:feature/add_explicit_return_type

Conversation

@alexgrad42
Copy link
Copy Markdown
Contributor

Fixes #600

No more primitive return types in tests

$  grep -rn 'let _ = .*node\.client' . --exclude-dir=target | awk -F'node.client.' '{print $2}' | awk -F'(' '{print $1}' | sort -u | xargs -I {} grep -rnE 'fn[[:space:]]+{}' .  --exclude-dir=target
./client/src/client_sync/v17/hidden.rs:20:            pub fn estimate_raw_fee(&self, conf_target: u32) -> Result<EstimateRawFee> {
./client/src/client_sync/v17/generating.rs:17:            pub fn generate_to_address(
./client/src/client_sync/v26/mining.rs:17:            pub fn get_prioritised_transactions(&self) -> Result<GetPrioritisedTransactions> {
./client/src/client_sync/v18/control.rs:15:            pub fn get_rpc_info(&self) -> Result<GetRpcInfo> { self.call("getrpcinfo", &[]) }
./client/src/client_sync/v17/wallet.rs:180:            pub fn new_address(&self) -> Result<bitcoin::Address> {
./client/src/client_sync/v17/wallet.rs:187:            pub fn new_address_with_type(&self, ty: AddressType) -> Result<bitcoin::Address> {
./client/src/client_sync/v17/wallet.rs:195:            pub fn new_address_with_label(
./client/src/client_sync/v17/wallet.rs:195:            pub fn new_address_with_label(
./client/src/client_sync/v17/wallet.rs:187:            pub fn new_address_with_type(&self, ty: AddressType) -> Result<bitcoin::Address> {
./client/src/client_sync/v17/raw_transactions.rs:180:            pub fn send_raw_transaction(
./client/src/client_sync/v17/wallet.rs:579:            pub fn send_to_address(
./client/src/client_sync/v17/wallet.rs:589:            pub fn send_to_address_rbf(
$

Copy link
Copy Markdown
Collaborator

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

One nit, the rest is good.

Comment thread integration_test/tests/control.rs Outdated
fn control__uptime() {
let node = BitcoinD::with_wallet(Wallet::None, &[]);
let _ = node.client.uptime().unwrap();
let _ : u32 = node.client.uptime().unwrap();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let _ : u32 = node.client.uptime().unwrap();
let _: u32 = node.client.uptime().unwrap();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just ran CI and the formatter also picked this up

Copy link
Copy Markdown
Contributor Author

@alexgrad42 alexgrad42 May 22, 2026

Choose a reason for hiding this comment

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

Fixed
Checking formatting...
$ cargo fmt -p bitcoind -p corepc-client -p jsonrpc -p bitreq -p corepc-types -p jsonrpc-fuzz --check
Formatting check passed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just fmt does what you need. Did you come up with that command yourself or did you get AI do it? I'm interested in the answer because if LLMs are not reading the justfile to work out commands then we need to configure them better.

Copy link
Copy Markdown
Contributor Author

@alexgrad42 alexgrad42 May 26, 2026

Choose a reason for hiding this comment

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

I've extracted from CI logs

Run cargo rbmt fmt --check
Checking formatting...
$ cargo fmt -p bitcoind -p corepc-client -p jsonrpc -p bitreq -p corepc-types -p jsonrpc-fuzz --check
Formatting check passed
++ git rev-parse --show-toplevel
+ REPO_DIR=/home/runner/work/corepc/corepc
++ cat ./nightly-version
+ cargo +nightly-2025-09-12 fmt --manifest-path /home/runner/work/corepc/corepc/integration_test/Cargo.toml --all -- --check
++ git rev-parse --show-toplevel
+ REPO_DIR=/home/runner/work/corepc/corepc
++ cat ./nightly-version
+ cargo +nightly-2025-09-12 fmt --manifest-path /home/runner/work/corepc/corepc/verify/Cargo.toml --all -- --check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh, nice. You can also use our super awesome maintainer tool if you don't mind installing stuff. https://crates.io/crates/cargo-rbmt

@alexgrad42 alexgrad42 force-pushed the feature/add_explicit_return_type branch from 61730dc to 60c493d Compare May 22, 2026 15:33
Copy link
Copy Markdown
Collaborator

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

ACK 60c493d

Thanks

Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 60c493d

@tcharding
Copy link
Copy Markdown
Member

tcharding commented May 25, 2026

Missed one, not sure how or why.

#[test]
#[cfg(not(feature = "v25_and_below"))]
fn mining__get_prioritised_transactions() {
    let node = BitcoinD::with_wallet(Wallet::Default, &[]);
    node.fund_wallet();

    let _ = node.client.get_prioritised_transactions().expect("getprioritisedtransactions");
}

@tcharding
Copy link
Copy Markdown
Member

Related: the get_rpc_info is missing its GetRpcInfo type

Everything else looks good. Thanks for uncovering our bugs!

@alexgrad42
Copy link
Copy Markdown
Contributor Author

alexgrad42 commented May 26, 2026

Something wrong with joining "let _" grep output through pipe with return type. But anyway I've checked both get_prioritised_transactions/get_rpc_info, they are not primitives

@tcharding
Copy link
Copy Markdown
Member

My bad, thanks mate. (I wrote #612 after you corrected me.)

@tcharding tcharding merged commit 818e5f2 into rust-bitcoin:master May 27, 2026
73 of 74 checks passed
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.

Add explicit type when testing any method that returns a stdlib type

3 participants