Add feature (parity) tests for API methods #10

tnull · 2022-09-16T13:06:06Z

This PR adds basic feature (parity) tests for the API methods:

coveralls · 2022-09-16T13:08:42Z

Pull Request Test Coverage Report for Build 3068063421

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

106 of 107 (99.07%) changed or added relevant lines in 3 files are covered.
64 unchanged lines in 2 files lost coverage.
Overall coverage increased (+38.4%) to 49.045%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/lib.rs	104	105	99.05%

Files with Coverage Reduction	New Missed Lines	%
src/async.rs	25	44.71%
src/blocking.rs	39	28.78%

Totals
Change from base Build 3061069813:	38.4%
Covered Lines:	308
Relevant Lines:	628

💛 - Coveralls

tnull · 2022-09-16T13:50:59Z

I now covered all feasible API methods. Testing the feature parity of get_fee_estimates is not really feasible since the results vary quite a bit.

tnull · 2022-09-16T14:19:36Z

Squashed fixups.

Cargo.toml

vladimirfomene

Tested ACK.

notmandatory · 2022-09-18T22:00:13Z

Tests look good but I'd like to see if we can run the tests against a local regtest bitcoind and esplora rather than hitting the blockstream server URL. I'm going to give it a try using the electrsd crate, feel free to give it a try also, and/or when I get it working I'll show you how to do it.

notmandatory · 2022-09-19T14:24:13Z

@tnull, @vladimirfomene has the local regtest setup working in vladimirfomene@829b2fc, I'd like to get his changes in as a setup function that all the tests can share. Then can we put each of your tests in it's own test function ? I know each test will be very small but it's a good idea for unit tests to only test a single thing, it will make it easier to read and to see if a method call fails which one it is.

tnull · 2022-09-19T15:01:56Z

@tnull, @vladimirfomene has the local regtest setup working in vladimirfomene@829b2fc, I'd like to get his changes in as a setup function that all the tests can share. Then can we put each of your tests in it's own test function ? I know each test will be very small but it's a good idea for unit tests to only test a single thing, it will make it easier to read and to see if a method call fails which one it is.

Sounds good, will add the setup function and break the test code down into individual methods!

vladimirfomene · 2022-09-19T15:23:28Z

@tnull, you can find the updated code here: https://github.com/vladimirfomene/rust-esplora-client/tree/add-testing

vladimirfomene · 2022-09-20T14:03:20Z

@tnull, will it help if I refactor my code into a setup function and write a couple of test functions?

tnull · 2022-09-20T14:07:39Z

@tnull, will it help if I refactor my code into a setup function and write a couple of test functions?

Don't worry. I already started making the changes, will probably push an update in the next 1-2 days.

tnull · 2022-09-21T12:44:24Z

Now split the tests and updated to run against local ElectrsD/BitcoinD instances.

tnull · 2022-09-21T13:20:46Z

I now covered all feasible API methods. Testing the feature parity of get_fee_estimates is not really feasible since the results vary quite a bit.

Also added basic functionality testing of get_fee_estimates now.

afilini · 2022-09-22T10:59:20Z

(needs rebase because #14 changed the CI)

src/lib.rs

afilini · 2022-09-22T11:11:40Z

src/lib.rs

+            let bitcoind_exe =
+                bitcoind::downloaded_exe_path().expect("bitcoind version feature must be enabled");


Normally we read an env variable for the bitcoind/electrsd binaries and only if that's not set we try to use the embedded one. I personally use this on nixos because the binaries downloaded by the crate don't work right out of the box, and it's just easier to have my own bitcoind/electrsd and use them

I think they are called BITCOIND_EXE and ELECTRSD_EXEin bdk

Done in 37e2142

I now adopted the somewhat inconsistent ELECTRS_EXE naming from BDK (vs. ELECTRSD_EXE) to make it more convenient.

tnull · 2022-09-22T11:11:54Z

(needs rebase because #14 changed the CI)

Rebased on master, squashed commits, and added a minor clippy warning fix.

tnull · 2022-09-22T12:43:01Z

Wow, clippy can be a pain 😁 . But passes CI now, squashed the fixups.

vladimirfomene

Tested ACK

notmandatory · 2022-09-25T21:53:11Z

src/lib.rs

+                .ok()
+                .or_else(|| bitcoind::downloaded_exe_path().ok())
+                .expect(
+                    "you should provide env var BITCOIND_EXE or specifiy a bitcoind version feature",


nit, spelling typo

Suggested change

"you should provide env var BITCOIND_EXE or specifiy a bitcoind version feature",

"you should provide env var BITCOIND_EXE or specify a bitcoind version feature",

Ah, fixed and directly squashed the changes.

Btw., I blatantly copied these typos from https://github.com/bitcoindevkit/bdk/blob/c3faf05be9361eb8e0afa2c066448fd10fe30b58/src/testutils/blockchain_tests.rs#L329-L340

Not sure if it warrants a PR for BDK, but if someone is already editing that file, fixing them there also might make sense.

src/lib.rs

notmandatory

ACK d1b5877

I commented on some small spelling typos, but everything else looks great to me.

notmandatory · 2022-09-25T22:00:55Z

If ok for everyone I'd like to get this published with Vlad on Mon or Tues so we can get a PR into bdk for the next release to remove the esplora client code there and use this crate.

tnull marked this pull request as draft September 16, 2022 13:13

tnull marked this pull request as ready for review September 16, 2022 13:48

tnull force-pushed the 2022-09-add-feature-parity-tests branch 2 times, most recently from 087559c to 0624093 Compare September 16, 2022 14:19

notmandatory reviewed Sep 16, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

vladimirfomene approved these changes Sep 18, 2022

View reviewed changes

tnull force-pushed the 2022-09-add-feature-parity-tests branch 3 times, most recently from d5ac02f to 4fdd176 Compare September 21, 2022 13:16

Test feature parity of API methods

e390c20

tnull force-pushed the 2022-09-add-feature-parity-tests branch from 4fdd176 to e390c20 Compare September 22, 2022 11:08

afilini reviewed Sep 22, 2022

View reviewed changes

tnull force-pushed the 2022-09-add-feature-parity-tests branch from 1a90378 to 400676c Compare September 22, 2022 11:20

Allow reading BITCOIN_EXE/ELECTRS_EXE env

37e2142

tnull force-pushed the 2022-09-add-feature-parity-tests branch 3 times, most recently from fa39cde to d1b5877 Compare September 22, 2022 12:41

vladimirfomene reviewed Sep 23, 2022

View reviewed changes

notmandatory reviewed Sep 25, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

notmandatory approved these changes Sep 25, 2022

View reviewed changes

Fix clippy warnings

ee8aee0

tnull force-pushed the 2022-09-add-feature-parity-tests branch from d1b5877 to ee8aee0 Compare September 26, 2022 08:04

notmandatory merged commit 8067110 into bitcoindevkit:master Sep 26, 2022

notmandatory added this to the Release 0.1.0 milestone Sep 26, 2022

gcomte mentioned this pull request Oct 5, 2022

Query block hash by by block height #13

Merged

		let bitcoind_exe =
		bitcoind::downloaded_exe_path().expect("bitcoind version feature must be enabled");

	"you should provide env var BITCOIND_EXE or specifiy a bitcoind version feature",
	"you should provide env var BITCOIND_EXE or specify a bitcoind version feature",

Add feature (parity) tests for API methods #10

Add feature (parity) tests for API methods #10

Uh oh!

Conversation

tnull commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3068063421

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

tnull commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Sep 16, 2022

Uh oh!

Uh oh!

vladimirfomene left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Sep 18, 2022

Uh oh!

notmandatory commented Sep 19, 2022

Uh oh!

tnull commented Sep 19, 2022

Uh oh!

vladimirfomene commented Sep 19, 2022

Uh oh!

vladimirfomene commented Sep 20, 2022

Uh oh!

tnull commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Sep 21, 2022

Uh oh!

tnull commented Sep 21, 2022

Uh oh!

afilini commented Sep 22, 2022

Uh oh!

Uh oh!

afilini Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

afilini Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

tnull Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

tnull commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Sep 22, 2022

Uh oh!

vladimirfomene left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory Sep 25, 2022

Choose a reason for hiding this comment

Uh oh!

tnull Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Sep 25, 2022

Uh oh!

Uh oh!

tnull commented Sep 16, 2022 •

edited

Loading

coveralls commented Sep 16, 2022 •

edited

Loading

tnull commented Sep 16, 2022 •

edited

Loading

tnull commented Sep 20, 2022 •

edited

Loading

tnull Sep 22, 2022 •

edited

Loading

tnull commented Sep 22, 2022 •

edited

Loading