Roll a custom TestEnv and drop lazy_static#176
Roll a custom TestEnv and drop lazy_static#176luisschwab wants to merge 3 commits intobitcoindevkit:masterfrom
TestEnv and drop lazy_static#176Conversation
|
Need to pin some stuff... |
luisschwab
force-pushed
the
feat/custom-testenv
branch
3 times, most recently
from
28bba06 to
4f5ee42
Compare
notmandatory
left a comment
notmandatory
left a comment
There was a problem hiding this comment.
I ran this through my AI friend 🤖 and it found two issue and one nit that I think are worth mentioning. Otherwise very good improvement to the testing!
|
Concept ACK |
Implement a custom `TestEnv`, which is instantiated per-test and fixes the un-dropped `bitcoind` issue. This also allows running tests with more than 1 thread, since they no longer share `bitcoind` and `electrsd` instances. Also adds helpers to fetch externally sourced addresses, fixing the issue where `bitcoind`'s wallet would send change outputs to it's own wallet address, which would mess with `test_get_scripthash_stats`. The `test` recipe and CI job are updated to now use 16 threads.
And add a test to verify that both async and blocking clients were built with headers.
luisschwab
force-pushed
the
feat/custom-testenv
branch
from
4f5ee42 to
8653954
Compare
|
Addressed Steve's review and added a |
|
Looks like your code is correctly setting the headers on the Builder for both the async and blocking clients, so no need for the unused |
The extra field is for the test only, will be handy to catch any regressioms when we switch to |
oleonardolima
left a comment
oleonardolima
left a comment
There was a problem hiding this comment.
Overall looks good! However, we shouldn't have the new field on AsyncClient, left an explanation how to properly test it in a comment below.
|
|
||
| # Pin dependencies for MSRV (1.75.0) | ||
| cargo update -p minreq --precise "2.13.2" | ||
| cargo update -p native-tls --precise "0.2.11" |
There was a problem hiding this comment.
why did you have to pin this down to 0.2.11, if the 0.2.13 already has 1.53.0 as MSRV ?
There was a problem hiding this comment.
Some transitive dependency was breaking our MSRV.
| fn wait_until_electrum_sees_block(&self) { | ||
| let _header = self.electrsd.client.block_headers_subscribe().unwrap(); | ||
| loop { | ||
| if self.electrsd.client.block_headers_pop().unwrap().is_some() { | ||
| break; | ||
| } | ||
| self.electrsd.trigger().unwrap(); | ||
| self.electrsd.client.ping().unwrap(); | ||
| std::thread::sleep(Duration::from_millis(500)); | ||
| } | ||
| } |
There was a problem hiding this comment.
You should keep the exponential backoff approach, instead of having a fixed delay.
| /// Get a `Legacy` regtest address. | ||
| fn get_legacy_address(&self) -> Address { | ||
| Address::from_str("mvUsRD2pNeQQ8nZq8CDEx6fjVQsyzqyhVC") | ||
| .unwrap() | ||
| .assume_checked() | ||
| } |
There was a problem hiding this comment.
What's the rationale for having fixed addresses, instead o fetching one from the client. If it's required only for test_get_scripthash_stats they should probably constants under that test only.
| #[allow(unused)] | ||
| /// HTTP headers to append on every request made to Esplora server. | ||
| pub(crate) headers: HashMap<String, String>, // TODO: remove this when `bitreq` is implemented |
There was a problem hiding this comment.
NACK on adding this field.
AsyncClient already handles the headers properly, as they're passed to reqwest::Client when building it.
I agree that having test_build_client_with_headers is a good idea, thought the right way to do it is asserting that final requests built from both clients contains the expected headers, and not that the client struct has the header field.
As an example, you can do it by updating test_build_client_with_headers to build a GET request with blocking_client.get_request() and async_client.client().get()... and assert that both requests have the expected headers.
4 participants
Closes #149
Implement a custom
TestEnv, which is instantiated per-test and fixes the un-droppedbitcoindissue. This also allows running tests with more than 1 thread, since they no longer sharebitcoindandelectrsdinstances.Also adds helpers to fetch externally sourced addresses, fixing the issue where
bitcoind's wallet would send change outputs to it's own wallet address, which would mess withtest_get_scripthash_stats.The
testrecipe and CI job are updated to now use 16 threads.