[CI] Fixed esplora blockchain tests in CI #430

rajarshimaitra · 2021-08-30T14:26:47Z

Description

Fixes #431 and esplora blockchain test skips in CI

Notes to the reviewers

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

Bugfixes:

I'm linking the issue being fixed by this PR

notmandatory

Looks good, also checkout related tcharding#2. We should be able to test esplora with ureq and reqwest. If you can incorporate into this PR I'll close the one I did.

Cargo.toml

rajarshimaitra · 2021-08-31T09:01:18Z

Updated with suggested refactors and async changes.

Rearranged the commits.

Moved out tls changes.

notmandatory · 2021-08-31T15:41:38Z

ACK 2caa590

codecov-commenter · 2021-08-31T15:59:15Z

Codecov Report

Merging #430 (2caa590) into master (721748e) will decrease coverage by 0.45%.
The diff coverage is n/a.

❗ Current head 2caa590 differs from pull request most recent head 9967045. Consider uploading reports for the commit 9967045 to get more accurate results

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
- Coverage   73.07%   72.62%   -0.46%     
==========================================
  Files          34       34              
  Lines        7399     7419      +20     
==========================================
- Hits         5407     5388      -19     
- Misses       1992     2031      +39

Impacted Files	Coverage Δ
src/blockchain/mod.rs	`0.00% <ø> (ø)`
src/keys/bip39.rs	`97.18% <0.00%> (-2.82%)`	⬇️
src/wallet/export.rs	`86.98% <0.00%> (-1.37%)`	⬇️
src/descriptor/policy.rs	`70.17% <0.00%> (-1.00%)`	⬇️
src/descriptor/dsl.rs	`91.49% <0.00%> (-0.81%)`	⬇️
src/wallet/tx_builder.rs	`91.42% <0.00%> (-0.79%)`	⬇️
src/wallet/mod.rs	`96.48% <0.00%> (-0.54%)`	⬇️
src/keys/mod.rs	`90.67% <0.00%> (-0.52%)`	⬇️
src/wallet/coin_selection.rs	`96.60% <0.00%> (-0.23%)`	⬇️
src/lib.rs	`85.20% <0.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 721748e...9967045. Read the comment docs.

tcharding

Thanks for looking into this. I'm glad to see the things I missed in CI being remedied, however I'm not super fond of the re-introduction of hard dependency on tokio - seems like a step backwards to me.

tcharding · 2021-08-31T22:13:27Z

.github/workflows/cont_integration.yml

          - name: esplora
+            features: test-esplora,use-esplora-reqwest
+          - name: esplora
+            features: test-esplora,use-esplora-ureq    


This line has trailing whitespace.

tcharding · 2021-08-31T22:21:27Z

.github/workflows/cont_integration.yml

-        run: cargo test --features test-${{ matrix.blockchain.name }} ${{ matrix.blockchain.name }}::bdk_blockchain_tests
-
+        run: cargo test --no-default-features --features ${{ matrix.blockchain.features }} ${{ matrix.blockchain.name }}::bdk_blockchain_tests
+  


This line has trailing whitespace.

tcharding · 2021-08-31T22:24:57Z

Cargo.toml

@@ -38,6 +38,10 @@ bitcoinconsensus = { version = "0.19.0-3", optional = true }
 # Needed by bdk_blockchain_tests macro
 core-rpc = { version = "0.14", optional = true }

+# Platform-specific dependencies
+[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
+tokio = { version = "1", features = ["rt"] }


Please correct me if I'm wrong, but this PR re-introduces a hard dependency on tokio just so we can test esplora-reqwest feature, that seems like a bad choice to me. A user of bdk that wishes to use reqwest will always be using async, why do we want to introduce a hard dependency on tokio just to be able to test in a manner that the library will not be used?

The esplora-reqwest feature can be tested coupled with either async-interface feature or using WASM target.

tcharding · 2021-08-31T22:38:20Z

src/blockchain/esplora/reqwest.rs

@@ -106,19 +106,19 @@ impl Blockchain for EsploraBlockchain {
    }

    fn get_tx(&self, txid: &Txid) -> Result<Option<Transaction>, Error> {
-        Ok(self.url_client._get_tx(txid).await?)
+        Ok(await_or_block!(self.url_client._get_tx(txid))?)


I do not like this usage of await_or_block because there is no reason to use reqwest in a blocking environment, we have ureq for that.

tcharding · 2021-08-31T23:34:47Z

All my comments are resolved by the implementation in #433, please see if you have any concerns with that approach @rajarshimaitra. Thanks.

notmandatory · 2021-09-10T23:32:38Z

@tcharding @LLFourn as @rajarshimaitra noted on #433 even if this PR isn't the final solution it will unblock some other work (bitcoindevkit/bdk-cli#41 and #429). Also it's more of a roll back to how the esplora::bdk_blockchain_tests used to work before ureq was introduced. Do you guys have any objection to merging this now while you work on a cleaner solution?

tcharding · 2021-09-12T23:55:19Z

@tcharding @LLFourn as @rajarshimaitra noted on #433 even if this PR isn't the final solution it will unblock some other work (bitcoindevkit/bdk-cli#41 and #429). Also it's more of a roll back to how the esplora::bdk_blockchain_tests used to work before ureq was introduced. Do you guys have any objection to merging this now while you work on a cleaner solution?

Don't stop the progress on my account :)

- Changed to local bdk-macro - Added back tokio - Update esplora-reqwest and test-esplora feature guards

- add back await_or_block! to bdk-macros - use await_or_block! in reqwest tests

- Fix esplora module level feature flag - Move esplora blockchain tests to module, to cover for both variants

rajarshimaitra changed the title ~~Esplora fix~~ [bug] Esplora blockchain test fix in CI Aug 30, 2021

rajarshimaitra requested review from afilini and notmandatory August 30, 2021 14:27

rajarshimaitra mentioned this pull request Aug 30, 2021

[Bug] Esplora reqwest blockchain tests are failing #431

Closed

rajarshimaitra force-pushed the esplora-fix branch from 92c3ef4 to 36f0c4d Compare August 30, 2021 15:02

rajarshimaitra changed the title ~~[bug] Esplora blockchain test fix in CI~~ [Fix] Fixed esplora blockchain tests in CI Aug 30, 2021

rajarshimaitra changed the title ~~[Fix] Fixed esplora blockchain tests in CI~~ [CI] Fixed esplora blockchain tests in CI Aug 30, 2021

notmandatory requested changes Aug 30, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

notmandatory mentioned this pull request Aug 30, 2021

Fix esplora integration test runner #415

Closed

9 tasks

rajarshimaitra force-pushed the esplora-fix branch from 36f0c4d to b6e3cc7 Compare August 31, 2021 08:59

rajarshimaitra mentioned this pull request Aug 31, 2021

Use ureq with default features #432

Merged

3 tasks

rajarshimaitra requested a review from notmandatory August 31, 2021 09:10

notmandatory approved these changes Aug 31, 2021

View reviewed changes

notmandatory mentioned this pull request Aug 31, 2021

Esplora integration test fix tcharding/bdk#2

Closed

3 tasks

tcharding reviewed Aug 31, 2021

View reviewed changes

tcharding mentioned this pull request Aug 31, 2021

Fix esplora testing #433

Closed

6 tasks

rajarshimaitra mentioned this pull request Sep 10, 2021

Update BDK version to 0.12.0 bitcoindevkit/bdk-cli#41

Merged

9 tasks

rajarshimaitra added 4 commits September 14, 2021 11:29

Update Cargo.toml

2459740

- Changed to local bdk-macro - Added back tokio - Update esplora-reqwest and test-esplora feature guards

Fix reqwest blockchain test

a41a003

- add back await_or_block! to bdk-macros - use await_or_block! in reqwest tests

Test refactor

8d1d92e

- Fix esplora module level feature flag - Move esplora blockchain tests to module, to cover for both variants

Update CI blockchain tests

10b53a5

rajarshimaitra force-pushed the esplora-fix branch from 9967045 to 10b53a5 Compare September 14, 2021 05:59

notmandatory merged commit 10b53a5 into bitcoindevkit:master Sep 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[CI] Fixed esplora blockchain tests in CI #430

[CI] Fixed esplora blockchain tests in CI #430

Uh oh!

rajarshimaitra commented Aug 30, 2021 •

edited

Loading

Uh oh!

notmandatory left a comment

Uh oh!

Uh oh!

rajarshimaitra commented Aug 31, 2021 •

edited

Loading

Uh oh!

notmandatory commented Aug 31, 2021

Uh oh!

codecov-commenter commented Aug 31, 2021

Uh oh!

tcharding left a comment •

edited

Loading

Uh oh!

tcharding Aug 31, 2021

Uh oh!

tcharding Aug 31, 2021

Uh oh!

tcharding Aug 31, 2021

Uh oh!

tcharding Aug 31, 2021

Uh oh!

tcharding commented Aug 31, 2021

Uh oh!

notmandatory commented Sep 10, 2021

Uh oh!

tcharding commented Sep 12, 2021

Uh oh!

Uh oh!

		run: cargo test --features test-${{ matrix.blockchain.name }} ${{ matrix.blockchain.name }}::bdk_blockchain_tests

		run: cargo test --no-default-features --features ${{ matrix.blockchain.features }} ${{ matrix.blockchain.name }}::bdk_blockchain_tests

[CI] Fixed esplora blockchain tests in CI #430

[CI] Fixed esplora blockchain tests in CI #430

Uh oh!

Conversation

rajarshimaitra commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Checklists

All Submissions:

Bugfixes:

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rajarshimaitra commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory commented Aug 31, 2021

Uh oh!

codecov-commenter commented Aug 31, 2021

Codecov Report

Uh oh!

tcharding left a comment • 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!

tcharding Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

tcharding Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

tcharding Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

tcharding Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

tcharding commented Aug 31, 2021

Uh oh!

notmandatory commented Sep 10, 2021

Uh oh!

tcharding commented Sep 12, 2021

Uh oh!

Uh oh!

rajarshimaitra commented Aug 30, 2021 •

edited

Loading

rajarshimaitra commented Aug 31, 2021 •

edited

Loading

tcharding left a comment •

edited

Loading