Fix esplora integration test runner - #415

Closed
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test
Closed

Fix esplora integration test runner#415
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test

Conversation

@tcharding

@tchardingtcharding commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

Description

Recently we added sub-modules to the esplora module for different HTTP
backends. We (Tobin) failed to update the integration test runner.

Modify the test running macro so that the backend is explicitly
selected:

cargo test --no-default-features --features=use-esplora-ureq,test-esplora

Notes to the reviewers

I can't actually get the integration tests to run to completion. They all (test-rpc, test-electrum, test-esplora) hang

$ echo $BITCOIND_EXE ✘ 130 /opt/bitcoin/bin/bitcoind
$ echo $ELECTRS_EXE
/home/tobin/.cargo/bin/electrs
$ cargo test --no-default-features --features=use-esplora-ureq,test-esplora
test blockchain::esplora::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

However without this patch the integration tests do not run at all for esplora so I believe this is still correct. And it seems like a good way to ask if I am doing something wrong with how I'm trying to run the integration tests? Any suggestions please?

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@tcharding
tchardingforce-pushed the esplora-integration-test branch 2 times, most recently from 1053f07 to d28765dCompareAugust 2, 2021 08:15
@RCasatta

Copy link
Copy Markdown
Contributor

I think the hanging is more likely due to the esplora version pointed by your env var. For esplora it's needed a specific electrs fork (branch new-index in https://github.com/blockstream/electrs).

bitcoind and electrsd can run with env var set or by setting a specific feature that during the build process download the needed executables. At the moment the feature is used in our tests, see:

test-esplora = ["electrsd/legacy", "electrsd/esplora_a33e97e1", "test-blockchains"]

a33e97e1 stands for the specific commit of electrs, so you can either use the feature, or build electrs at that commit. Also electrsd/legacy feature is needed because there has been changes in electrs parameters...

@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks for the tips @RCasatta, I tried as you suggested (with no env vars set) but still tests hang. Can see from the log output that its pulling down bitcoind/electrs to run the tests. Any other ideas please?

$ cargo test --features test-electrum
...
[2021-08-02T22:27:17Z DEBUG bdk::testutils::blockchain_tests] launching /home/tobin/.cargo/bitcoin/bitcoin-0.21.1/bin/bitcoind and /home/tobin/.cargo/electrs/electrs_linux_v0.8.10/electrs
...
[2021-08-02T22:27:18Z DEBUG electrsd] args: ["-vvv", "--db-dir", "/tmp/.tmphAsIio", "--network", "regtest", "--cookie-file", "/tmp/.tmpDleo7B/regtest/.cookie", "--daemon-rpc-addr", "127.0.0.1:42461", "--jsonrpc-import", "--electrum-rpc-addr", "0.0.0.0:46495", "--monitoring-addr", "0.0.0.0:44203"]
test blockchain::electrum::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

@RCasatta

RCasatta commented Aug 3, 2021

Copy link
Copy Markdown
Contributor

I can't reproduce on my system and neither on the CI (run in this PR looks failing for unrelated HTTP issue, this one https://github.com/RCasatta/bdk/actions/runs/1093394555 looks fine)

Maybe some more details on your system could help to reproduce.

Also, maybe running this:

RUST_LOG=bdk=debug cargo test --features test-electrum -- test_sync_address_reuse

also with this patch:

diff --git a/src/testutils/blockchain_tests.rs b/src/testutils/blockchain_tests.rs
index 2df72d3..e374cb0 100644
--- a/src/testutils/blockchain_tests.rs
+++ b/src/testutils/blockchain_tests.rs
@@ -24,12 +24,17 @@ pub struct TestClient {
impl TestClient {
pub fn new(bitcoind_exe: String, electrs_exe: String) -> Self {
debug!("launching {} and {}", &bitcoind_exe, &electrs_exe);
- let bitcoind = BitcoinD::new(bitcoind_exe).unwrap();
+ let conf = bitcoind::Conf {
+ view_stdout: true,
+ ..Default::default()
+ };
+ let bitcoind = BitcoinD::with_conf(bitcoind_exe).unwrap();
let http_enabled = cfg!(feature = "test-esplora");
let conf = Conf {
http_enabled,
+ view_stderr: true,
..Default::default()
};
let electrsd = ElectrsD::with_conf(electrs_exe, &bitcoind, &conf).unwrap();

May give insight on what's happening under the hood

UPDATE: the patch seems useful to have in case you have a Debug log level, so I opened a PR #417

@tcharding
tchardingforce-pushed the esplora-integration-test branch from d28765d to 1f6dd33CompareAugust 4, 2021 00:34
notmandatory
notmandatory previously approved these changes Aug 26, 2021

@notmandatorynotmandatory left a comment

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.

Works for me locally (osx) and in CI so ACK 1f6dd33

I just noticed we also need to update the CI test-blockchains job to add the use-esplora-ureq or use-esplora-request features. I'll add a commit for this before merging.

@notmandatory
notmandatory self-requested a review August 26, 2021 10:06
@notmandatory
notmandatory dismissed their stale reviewAugust 26, 2021 10:06

Need to update CI and fix hanging

Recently we added sub-modules to the `esplora` module for different HTTP
backends. We (Tobin) failed to update the integration test runner.
Modify the test running macro so that the backend is explicitly
selected:
`cargo test --no-default-features --features=use-esplora-ureq,test-esplora`
@notmandatory
notmandatoryforce-pushed the esplora-integration-test branch from 1f6dd33 to ad5a9f8CompareAugust 26, 2021 20:43
@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks @notmandatory, picking up the pieces for me :)

@notmandatory

Copy link
Copy Markdown
Member

I think @rajarshimaitra's #430 will go in first then we can add this change. 🙂

@notmandatorynotmandatory mentioned this pull request Sep 1, 2021
6 tasks
@tcharding

Copy link
Copy Markdown
ContributorAuthor

#433 solves this issue in a better way.

@tcharding
tcharding deleted the esplora-integration-test branch October 13, 2023 00:41
Sign up for freeto 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.

3 participants

@tcharding@RCasatta@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix esplora integration test runner - #415

Closed
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test
Closed

Fix esplora integration test runner#415
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test

Conversation

@tcharding

@tchardingtcharding commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

Description

Recently we added sub-modules to the esplora module for different HTTP
backends. We (Tobin) failed to update the integration test runner.

Modify the test running macro so that the backend is explicitly
selected:

cargo test --no-default-features --features=use-esplora-ureq,test-esplora

Notes to the reviewers

I can't actually get the integration tests to run to completion. They all (test-rpc, test-electrum, test-esplora) hang

$ echo $BITCOIND_EXE ✘ 130 /opt/bitcoin/bin/bitcoind
$ echo $ELECTRS_EXE
/home/tobin/.cargo/bin/electrs
$ cargo test --no-default-features --features=use-esplora-ureq,test-esplora
test blockchain::esplora::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

However without this patch the integration tests do not run at all for esplora so I believe this is still correct. And it seems like a good way to ask if I am doing something wrong with how I'm trying to run the integration tests? Any suggestions please?

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@tcharding
tchardingforce-pushed the esplora-integration-test branch 2 times, most recently from 1053f07 to d28765dCompareAugust 2, 2021 08:15
@RCasatta

Copy link
Copy Markdown
Contributor

I think the hanging is more likely due to the esplora version pointed by your env var. For esplora it's needed a specific electrs fork (branch new-index in https://github.com/blockstream/electrs).

bitcoind and electrsd can run with env var set or by setting a specific feature that during the build process download the needed executables. At the moment the feature is used in our tests, see:

test-esplora = ["electrsd/legacy", "electrsd/esplora_a33e97e1", "test-blockchains"]

a33e97e1 stands for the specific commit of electrs, so you can either use the feature, or build electrs at that commit. Also electrsd/legacy feature is needed because there has been changes in electrs parameters...

@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks for the tips @RCasatta, I tried as you suggested (with no env vars set) but still tests hang. Can see from the log output that its pulling down bitcoind/electrs to run the tests. Any other ideas please?

$ cargo test --features test-electrum
...
[2021-08-02T22:27:17Z DEBUG bdk::testutils::blockchain_tests] launching /home/tobin/.cargo/bitcoin/bitcoin-0.21.1/bin/bitcoind and /home/tobin/.cargo/electrs/electrs_linux_v0.8.10/electrs
...
[2021-08-02T22:27:18Z DEBUG electrsd] args: ["-vvv", "--db-dir", "/tmp/.tmphAsIio", "--network", "regtest", "--cookie-file", "/tmp/.tmpDleo7B/regtest/.cookie", "--daemon-rpc-addr", "127.0.0.1:42461", "--jsonrpc-import", "--electrum-rpc-addr", "0.0.0.0:46495", "--monitoring-addr", "0.0.0.0:44203"]
test blockchain::electrum::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

@RCasatta

RCasatta commented Aug 3, 2021

Copy link
Copy Markdown
Contributor

I can't reproduce on my system and neither on the CI (run in this PR looks failing for unrelated HTTP issue, this one https://github.com/RCasatta/bdk/actions/runs/1093394555 looks fine)

Maybe some more details on your system could help to reproduce.

Also, maybe running this:

RUST_LOG=bdk=debug cargo test --features test-electrum -- test_sync_address_reuse

also with this patch:

diff --git a/src/testutils/blockchain_tests.rs b/src/testutils/blockchain_tests.rs
index 2df72d3..e374cb0 100644
--- a/src/testutils/blockchain_tests.rs
+++ b/src/testutils/blockchain_tests.rs
@@ -24,12 +24,17 @@ pub struct TestClient {
impl TestClient {
pub fn new(bitcoind_exe: String, electrs_exe: String) -> Self {
debug!("launching {} and {}", &bitcoind_exe, &electrs_exe);
- let bitcoind = BitcoinD::new(bitcoind_exe).unwrap();
+ let conf = bitcoind::Conf {
+ view_stdout: true,
+ ..Default::default()
+ };
+ let bitcoind = BitcoinD::with_conf(bitcoind_exe).unwrap();
let http_enabled = cfg!(feature = "test-esplora");
let conf = Conf {
http_enabled,
+ view_stderr: true,
..Default::default()
};
let electrsd = ElectrsD::with_conf(electrs_exe, &bitcoind, &conf).unwrap();

May give insight on what's happening under the hood

UPDATE: the patch seems useful to have in case you have a Debug log level, so I opened a PR #417

@tcharding
tchardingforce-pushed the esplora-integration-test branch from d28765d to 1f6dd33CompareAugust 4, 2021 00:34
notmandatory
notmandatory previously approved these changes Aug 26, 2021

@notmandatorynotmandatory left a comment

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.

Works for me locally (osx) and in CI so ACK 1f6dd33

I just noticed we also need to update the CI test-blockchains job to add the use-esplora-ureq or use-esplora-request features. I'll add a commit for this before merging.

@notmandatory
notmandatory self-requested a review August 26, 2021 10:06
@notmandatory
notmandatory dismissed their stale reviewAugust 26, 2021 10:06

Need to update CI and fix hanging

Recently we added sub-modules to the `esplora` module for different HTTP
backends. We (Tobin) failed to update the integration test runner.
Modify the test running macro so that the backend is explicitly
selected:
`cargo test --no-default-features --features=use-esplora-ureq,test-esplora`
@notmandatory
notmandatoryforce-pushed the esplora-integration-test branch from 1f6dd33 to ad5a9f8CompareAugust 26, 2021 20:43
@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks @notmandatory, picking up the pieces for me :)

@notmandatory

Copy link
Copy Markdown
Member

I think @rajarshimaitra's #430 will go in first then we can add this change. 🙂

@notmandatorynotmandatory mentioned this pull request Sep 1, 2021
6 tasks
@tcharding

Copy link
Copy Markdown
ContributorAuthor

#433 solves this issue in a better way.

@tcharding
tcharding deleted the esplora-integration-test branch October 13, 2023 00:41
Sign up for freeto 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.

3 participants

@tcharding@RCasatta@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix esplora integration test runner - #415

Closed
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test
Closed

Fix esplora integration test runner#415
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test

Conversation

@tcharding

@tchardingtcharding commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

Description

Recently we added sub-modules to the esplora module for different HTTP
backends. We (Tobin) failed to update the integration test runner.

Modify the test running macro so that the backend is explicitly
selected:

cargo test --no-default-features --features=use-esplora-ureq,test-esplora

Notes to the reviewers

I can't actually get the integration tests to run to completion. They all (test-rpc, test-electrum, test-esplora) hang

$ echo $BITCOIND_EXE ✘ 130 /opt/bitcoin/bin/bitcoind
$ echo $ELECTRS_EXE
/home/tobin/.cargo/bin/electrs
$ cargo test --no-default-features --features=use-esplora-ureq,test-esplora
test blockchain::esplora::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

However without this patch the integration tests do not run at all for esplora so I believe this is still correct. And it seems like a good way to ask if I am doing something wrong with how I'm trying to run the integration tests? Any suggestions please?

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@tcharding
tchardingforce-pushed the esplora-integration-test branch 2 times, most recently from 1053f07 to d28765dCompareAugust 2, 2021 08:15
@RCasatta

Copy link
Copy Markdown
Contributor

I think the hanging is more likely due to the esplora version pointed by your env var. For esplora it's needed a specific electrs fork (branch new-index in https://github.com/blockstream/electrs).

bitcoind and electrsd can run with env var set or by setting a specific feature that during the build process download the needed executables. At the moment the feature is used in our tests, see:

test-esplora = ["electrsd/legacy", "electrsd/esplora_a33e97e1", "test-blockchains"]

a33e97e1 stands for the specific commit of electrs, so you can either use the feature, or build electrs at that commit. Also electrsd/legacy feature is needed because there has been changes in electrs parameters...

@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks for the tips @RCasatta, I tried as you suggested (with no env vars set) but still tests hang. Can see from the log output that its pulling down bitcoind/electrs to run the tests. Any other ideas please?

$ cargo test --features test-electrum
...
[2021-08-02T22:27:17Z DEBUG bdk::testutils::blockchain_tests] launching /home/tobin/.cargo/bitcoin/bitcoin-0.21.1/bin/bitcoind and /home/tobin/.cargo/electrs/electrs_linux_v0.8.10/electrs
...
[2021-08-02T22:27:18Z DEBUG electrsd] args: ["-vvv", "--db-dir", "/tmp/.tmphAsIio", "--network", "regtest", "--cookie-file", "/tmp/.tmpDleo7B/regtest/.cookie", "--daemon-rpc-addr", "127.0.0.1:42461", "--jsonrpc-import", "--electrum-rpc-addr", "0.0.0.0:46495", "--monitoring-addr", "0.0.0.0:44203"]
test blockchain::electrum::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

@RCasatta

RCasatta commented Aug 3, 2021

Copy link
Copy Markdown
Contributor

I can't reproduce on my system and neither on the CI (run in this PR looks failing for unrelated HTTP issue, this one https://github.com/RCasatta/bdk/actions/runs/1093394555 looks fine)

Maybe some more details on your system could help to reproduce.

Also, maybe running this:

RUST_LOG=bdk=debug cargo test --features test-electrum -- test_sync_address_reuse

also with this patch:

diff --git a/src/testutils/blockchain_tests.rs b/src/testutils/blockchain_tests.rs
index 2df72d3..e374cb0 100644
--- a/src/testutils/blockchain_tests.rs
+++ b/src/testutils/blockchain_tests.rs
@@ -24,12 +24,17 @@ pub struct TestClient {
impl TestClient {
pub fn new(bitcoind_exe: String, electrs_exe: String) -> Self {
debug!("launching {} and {}", &bitcoind_exe, &electrs_exe);
- let bitcoind = BitcoinD::new(bitcoind_exe).unwrap();
+ let conf = bitcoind::Conf {
+ view_stdout: true,
+ ..Default::default()
+ };
+ let bitcoind = BitcoinD::with_conf(bitcoind_exe).unwrap();
let http_enabled = cfg!(feature = "test-esplora");
let conf = Conf {
http_enabled,
+ view_stderr: true,
..Default::default()
};
let electrsd = ElectrsD::with_conf(electrs_exe, &bitcoind, &conf).unwrap();

May give insight on what's happening under the hood

UPDATE: the patch seems useful to have in case you have a Debug log level, so I opened a PR #417

@tcharding
tchardingforce-pushed the esplora-integration-test branch from d28765d to 1f6dd33CompareAugust 4, 2021 00:34
notmandatory
notmandatory previously approved these changes Aug 26, 2021

@notmandatorynotmandatory left a comment

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.

Works for me locally (osx) and in CI so ACK 1f6dd33

I just noticed we also need to update the CI test-blockchains job to add the use-esplora-ureq or use-esplora-request features. I'll add a commit for this before merging.

@notmandatory
notmandatory self-requested a review August 26, 2021 10:06
@notmandatory
notmandatory dismissed their stale reviewAugust 26, 2021 10:06

Need to update CI and fix hanging

Recently we added sub-modules to the `esplora` module for different HTTP
backends. We (Tobin) failed to update the integration test runner.
Modify the test running macro so that the backend is explicitly
selected:
`cargo test --no-default-features --features=use-esplora-ureq,test-esplora`
@notmandatory
notmandatoryforce-pushed the esplora-integration-test branch from 1f6dd33 to ad5a9f8CompareAugust 26, 2021 20:43
@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks @notmandatory, picking up the pieces for me :)

@notmandatory

Copy link
Copy Markdown
Member

I think @rajarshimaitra's #430 will go in first then we can add this change. 🙂

@notmandatorynotmandatory mentioned this pull request Sep 1, 2021
6 tasks
@tcharding

Copy link
Copy Markdown
ContributorAuthor

#433 solves this issue in a better way.

@tcharding
tcharding deleted the esplora-integration-test branch October 13, 2023 00:41
Sign up for freeto 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.

3 participants

@tcharding@RCasatta@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix esplora integration test runner - #415

Closed
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test
Closed

Fix esplora integration test runner#415
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test

Conversation

@tcharding

@tchardingtcharding commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

Description

Recently we added sub-modules to the esplora module for different HTTP
backends. We (Tobin) failed to update the integration test runner.

Modify the test running macro so that the backend is explicitly
selected:

cargo test --no-default-features --features=use-esplora-ureq,test-esplora

Notes to the reviewers

I can't actually get the integration tests to run to completion. They all (test-rpc, test-electrum, test-esplora) hang

$ echo $BITCOIND_EXE ✘ 130 /opt/bitcoin/bin/bitcoind
$ echo $ELECTRS_EXE
/home/tobin/.cargo/bin/electrs
$ cargo test --no-default-features --features=use-esplora-ureq,test-esplora
test blockchain::esplora::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

However without this patch the integration tests do not run at all for esplora so I believe this is still correct. And it seems like a good way to ask if I am doing something wrong with how I'm trying to run the integration tests? Any suggestions please?

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@tcharding
tchardingforce-pushed the esplora-integration-test branch 2 times, most recently from 1053f07 to d28765dCompareAugust 2, 2021 08:15
@RCasatta

Copy link
Copy Markdown
Contributor

I think the hanging is more likely due to the esplora version pointed by your env var. For esplora it's needed a specific electrs fork (branch new-index in https://github.com/blockstream/electrs).

bitcoind and electrsd can run with env var set or by setting a specific feature that during the build process download the needed executables. At the moment the feature is used in our tests, see:

test-esplora = ["electrsd/legacy", "electrsd/esplora_a33e97e1", "test-blockchains"]

a33e97e1 stands for the specific commit of electrs, so you can either use the feature, or build electrs at that commit. Also electrsd/legacy feature is needed because there has been changes in electrs parameters...

@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks for the tips @RCasatta, I tried as you suggested (with no env vars set) but still tests hang. Can see from the log output that its pulling down bitcoind/electrs to run the tests. Any other ideas please?

$ cargo test --features test-electrum
...
[2021-08-02T22:27:17Z DEBUG bdk::testutils::blockchain_tests] launching /home/tobin/.cargo/bitcoin/bitcoin-0.21.1/bin/bitcoind and /home/tobin/.cargo/electrs/electrs_linux_v0.8.10/electrs
...
[2021-08-02T22:27:18Z DEBUG electrsd] args: ["-vvv", "--db-dir", "/tmp/.tmphAsIio", "--network", "regtest", "--cookie-file", "/tmp/.tmpDleo7B/regtest/.cookie", "--daemon-rpc-addr", "127.0.0.1:42461", "--jsonrpc-import", "--electrum-rpc-addr", "0.0.0.0:46495", "--monitoring-addr", "0.0.0.0:44203"]
test blockchain::electrum::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

@RCasatta

RCasatta commented Aug 3, 2021

Copy link
Copy Markdown
Contributor

I can't reproduce on my system and neither on the CI (run in this PR looks failing for unrelated HTTP issue, this one https://github.com/RCasatta/bdk/actions/runs/1093394555 looks fine)

Maybe some more details on your system could help to reproduce.

Also, maybe running this:

RUST_LOG=bdk=debug cargo test --features test-electrum -- test_sync_address_reuse

also with this patch:

diff --git a/src/testutils/blockchain_tests.rs b/src/testutils/blockchain_tests.rs
index 2df72d3..e374cb0 100644
--- a/src/testutils/blockchain_tests.rs
+++ b/src/testutils/blockchain_tests.rs
@@ -24,12 +24,17 @@ pub struct TestClient {
impl TestClient {
pub fn new(bitcoind_exe: String, electrs_exe: String) -> Self {
debug!("launching {} and {}", &bitcoind_exe, &electrs_exe);
- let bitcoind = BitcoinD::new(bitcoind_exe).unwrap();
+ let conf = bitcoind::Conf {
+ view_stdout: true,
+ ..Default::default()
+ };
+ let bitcoind = BitcoinD::with_conf(bitcoind_exe).unwrap();
let http_enabled = cfg!(feature = "test-esplora");
let conf = Conf {
http_enabled,
+ view_stderr: true,
..Default::default()
};
let electrsd = ElectrsD::with_conf(electrs_exe, &bitcoind, &conf).unwrap();

May give insight on what's happening under the hood

UPDATE: the patch seems useful to have in case you have a Debug log level, so I opened a PR #417

@tcharding
tchardingforce-pushed the esplora-integration-test branch from d28765d to 1f6dd33CompareAugust 4, 2021 00:34
notmandatory
notmandatory previously approved these changes Aug 26, 2021

@notmandatorynotmandatory left a comment

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.

Works for me locally (osx) and in CI so ACK 1f6dd33

I just noticed we also need to update the CI test-blockchains job to add the use-esplora-ureq or use-esplora-request features. I'll add a commit for this before merging.

@notmandatory
notmandatory self-requested a review August 26, 2021 10:06
@notmandatory
notmandatory dismissed their stale reviewAugust 26, 2021 10:06

Need to update CI and fix hanging

Recently we added sub-modules to the `esplora` module for different HTTP
backends. We (Tobin) failed to update the integration test runner.
Modify the test running macro so that the backend is explicitly
selected:
`cargo test --no-default-features --features=use-esplora-ureq,test-esplora`
@notmandatory
notmandatoryforce-pushed the esplora-integration-test branch from 1f6dd33 to ad5a9f8CompareAugust 26, 2021 20:43
@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks @notmandatory, picking up the pieces for me :)

@notmandatory

Copy link
Copy Markdown
Member

I think @rajarshimaitra's #430 will go in first then we can add this change. 🙂

@notmandatorynotmandatory mentioned this pull request Sep 1, 2021
6 tasks
@tcharding

Copy link
Copy Markdown
ContributorAuthor

#433 solves this issue in a better way.

@tcharding
tcharding deleted the esplora-integration-test branch October 13, 2023 00:41
Sign up for freeto 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.

3 participants

@tcharding@RCasatta@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix esplora integration test runner - #415

Closed
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test
Closed

Fix esplora integration test runner#415
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test

Conversation

@tcharding

@tchardingtcharding commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

Description

Recently we added sub-modules to the esplora module for different HTTP
backends. We (Tobin) failed to update the integration test runner.

Modify the test running macro so that the backend is explicitly
selected:

cargo test --no-default-features --features=use-esplora-ureq,test-esplora

Notes to the reviewers

I can't actually get the integration tests to run to completion. They all (test-rpc, test-electrum, test-esplora) hang

$ echo $BITCOIND_EXE ✘ 130 /opt/bitcoin/bin/bitcoind
$ echo $ELECTRS_EXE
/home/tobin/.cargo/bin/electrs
$ cargo test --no-default-features --features=use-esplora-ureq,test-esplora
test blockchain::esplora::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

However without this patch the integration tests do not run at all for esplora so I believe this is still correct. And it seems like a good way to ask if I am doing something wrong with how I'm trying to run the integration tests? Any suggestions please?

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@tcharding
tchardingforce-pushed the esplora-integration-test branch 2 times, most recently from 1053f07 to d28765dCompareAugust 2, 2021 08:15
@RCasatta

Copy link
Copy Markdown
Contributor

I think the hanging is more likely due to the esplora version pointed by your env var. For esplora it's needed a specific electrs fork (branch new-index in https://github.com/blockstream/electrs).

bitcoind and electrsd can run with env var set or by setting a specific feature that during the build process download the needed executables. At the moment the feature is used in our tests, see:

test-esplora = ["electrsd/legacy", "electrsd/esplora_a33e97e1", "test-blockchains"]

a33e97e1 stands for the specific commit of electrs, so you can either use the feature, or build electrs at that commit. Also electrsd/legacy feature is needed because there has been changes in electrs parameters...

@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks for the tips @RCasatta, I tried as you suggested (with no env vars set) but still tests hang. Can see from the log output that its pulling down bitcoind/electrs to run the tests. Any other ideas please?

$ cargo test --features test-electrum
...
[2021-08-02T22:27:17Z DEBUG bdk::testutils::blockchain_tests] launching /home/tobin/.cargo/bitcoin/bitcoin-0.21.1/bin/bitcoind and /home/tobin/.cargo/electrs/electrs_linux_v0.8.10/electrs
...
[2021-08-02T22:27:18Z DEBUG electrsd] args: ["-vvv", "--db-dir", "/tmp/.tmphAsIio", "--network", "regtest", "--cookie-file", "/tmp/.tmpDleo7B/regtest/.cookie", "--daemon-rpc-addr", "127.0.0.1:42461", "--jsonrpc-import", "--electrum-rpc-addr", "0.0.0.0:46495", "--monitoring-addr", "0.0.0.0:44203"]
test blockchain::electrum::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

@RCasatta

RCasatta commented Aug 3, 2021

Copy link
Copy Markdown
Contributor

I can't reproduce on my system and neither on the CI (run in this PR looks failing for unrelated HTTP issue, this one https://github.com/RCasatta/bdk/actions/runs/1093394555 looks fine)

Maybe some more details on your system could help to reproduce.

Also, maybe running this:

RUST_LOG=bdk=debug cargo test --features test-electrum -- test_sync_address_reuse

also with this patch:

diff --git a/src/testutils/blockchain_tests.rs b/src/testutils/blockchain_tests.rs
index 2df72d3..e374cb0 100644
--- a/src/testutils/blockchain_tests.rs
+++ b/src/testutils/blockchain_tests.rs
@@ -24,12 +24,17 @@ pub struct TestClient {
impl TestClient {
pub fn new(bitcoind_exe: String, electrs_exe: String) -> Self {
debug!("launching {} and {}", &bitcoind_exe, &electrs_exe);
- let bitcoind = BitcoinD::new(bitcoind_exe).unwrap();
+ let conf = bitcoind::Conf {
+ view_stdout: true,
+ ..Default::default()
+ };
+ let bitcoind = BitcoinD::with_conf(bitcoind_exe).unwrap();
let http_enabled = cfg!(feature = "test-esplora");
let conf = Conf {
http_enabled,
+ view_stderr: true,
..Default::default()
};
let electrsd = ElectrsD::with_conf(electrs_exe, &bitcoind, &conf).unwrap();

May give insight on what's happening under the hood

UPDATE: the patch seems useful to have in case you have a Debug log level, so I opened a PR #417

@tcharding
tchardingforce-pushed the esplora-integration-test branch from d28765d to 1f6dd33CompareAugust 4, 2021 00:34
notmandatory
notmandatory previously approved these changes Aug 26, 2021

@notmandatorynotmandatory left a comment

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.

Works for me locally (osx) and in CI so ACK 1f6dd33

I just noticed we also need to update the CI test-blockchains job to add the use-esplora-ureq or use-esplora-request features. I'll add a commit for this before merging.

@notmandatory
notmandatory self-requested a review August 26, 2021 10:06
@notmandatory
notmandatory dismissed their stale reviewAugust 26, 2021 10:06

Need to update CI and fix hanging

Recently we added sub-modules to the `esplora` module for different HTTP
backends. We (Tobin) failed to update the integration test runner.
Modify the test running macro so that the backend is explicitly
selected:
`cargo test --no-default-features --features=use-esplora-ureq,test-esplora`
@notmandatory
notmandatoryforce-pushed the esplora-integration-test branch from 1f6dd33 to ad5a9f8CompareAugust 26, 2021 20:43
@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks @notmandatory, picking up the pieces for me :)

@notmandatory

Copy link
Copy Markdown
Member

I think @rajarshimaitra's #430 will go in first then we can add this change. 🙂

@notmandatorynotmandatory mentioned this pull request Sep 1, 2021
6 tasks
@tcharding

Copy link
Copy Markdown
ContributorAuthor

#433 solves this issue in a better way.

@tcharding
tcharding deleted the esplora-integration-test branch October 13, 2023 00:41
Sign up for freeto 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.

3 participants

@tcharding@RCasatta@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix esplora integration test runner - #415

Closed
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test
Closed

Fix esplora integration test runner#415
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test

Conversation

@tcharding

@tchardingtcharding commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

Description

Recently we added sub-modules to the esplora module for different HTTP
backends. We (Tobin) failed to update the integration test runner.

Modify the test running macro so that the backend is explicitly
selected:

cargo test --no-default-features --features=use-esplora-ureq,test-esplora

Notes to the reviewers

I can't actually get the integration tests to run to completion. They all (test-rpc, test-electrum, test-esplora) hang

$ echo $BITCOIND_EXE ✘ 130 /opt/bitcoin/bin/bitcoind
$ echo $ELECTRS_EXE
/home/tobin/.cargo/bin/electrs
$ cargo test --no-default-features --features=use-esplora-ureq,test-esplora
test blockchain::esplora::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

However without this patch the integration tests do not run at all for esplora so I believe this is still correct. And it seems like a good way to ask if I am doing something wrong with how I'm trying to run the integration tests? Any suggestions please?

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@tcharding
tchardingforce-pushed the esplora-integration-test branch 2 times, most recently from 1053f07 to d28765dCompareAugust 2, 2021 08:15
@RCasatta

Copy link
Copy Markdown
Contributor

I think the hanging is more likely due to the esplora version pointed by your env var. For esplora it's needed a specific electrs fork (branch new-index in https://github.com/blockstream/electrs).

bitcoind and electrsd can run with env var set or by setting a specific feature that during the build process download the needed executables. At the moment the feature is used in our tests, see:

test-esplora = ["electrsd/legacy", "electrsd/esplora_a33e97e1", "test-blockchains"]

a33e97e1 stands for the specific commit of electrs, so you can either use the feature, or build electrs at that commit. Also electrsd/legacy feature is needed because there has been changes in electrs parameters...

@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks for the tips @RCasatta, I tried as you suggested (with no env vars set) but still tests hang. Can see from the log output that its pulling down bitcoind/electrs to run the tests. Any other ideas please?

$ cargo test --features test-electrum
...
[2021-08-02T22:27:17Z DEBUG bdk::testutils::blockchain_tests] launching /home/tobin/.cargo/bitcoin/bitcoin-0.21.1/bin/bitcoind and /home/tobin/.cargo/electrs/electrs_linux_v0.8.10/electrs
...
[2021-08-02T22:27:18Z DEBUG electrsd] args: ["-vvv", "--db-dir", "/tmp/.tmphAsIio", "--network", "regtest", "--cookie-file", "/tmp/.tmpDleo7B/regtest/.cookie", "--daemon-rpc-addr", "127.0.0.1:42461", "--jsonrpc-import", "--electrum-rpc-addr", "0.0.0.0:46495", "--monitoring-addr", "0.0.0.0:44203"]
test blockchain::electrum::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

@RCasatta

RCasatta commented Aug 3, 2021

Copy link
Copy Markdown
Contributor

I can't reproduce on my system and neither on the CI (run in this PR looks failing for unrelated HTTP issue, this one https://github.com/RCasatta/bdk/actions/runs/1093394555 looks fine)

Maybe some more details on your system could help to reproduce.

Also, maybe running this:

RUST_LOG=bdk=debug cargo test --features test-electrum -- test_sync_address_reuse

also with this patch:

diff --git a/src/testutils/blockchain_tests.rs b/src/testutils/blockchain_tests.rs
index 2df72d3..e374cb0 100644
--- a/src/testutils/blockchain_tests.rs
+++ b/src/testutils/blockchain_tests.rs
@@ -24,12 +24,17 @@ pub struct TestClient {
impl TestClient {
pub fn new(bitcoind_exe: String, electrs_exe: String) -> Self {
debug!("launching {} and {}", &bitcoind_exe, &electrs_exe);
- let bitcoind = BitcoinD::new(bitcoind_exe).unwrap();
+ let conf = bitcoind::Conf {
+ view_stdout: true,
+ ..Default::default()
+ };
+ let bitcoind = BitcoinD::with_conf(bitcoind_exe).unwrap();
let http_enabled = cfg!(feature = "test-esplora");
let conf = Conf {
http_enabled,
+ view_stderr: true,
..Default::default()
};
let electrsd = ElectrsD::with_conf(electrs_exe, &bitcoind, &conf).unwrap();

May give insight on what's happening under the hood

UPDATE: the patch seems useful to have in case you have a Debug log level, so I opened a PR #417

@tcharding
tchardingforce-pushed the esplora-integration-test branch from d28765d to 1f6dd33CompareAugust 4, 2021 00:34
notmandatory
notmandatory previously approved these changes Aug 26, 2021

@notmandatorynotmandatory left a comment

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.

Works for me locally (osx) and in CI so ACK 1f6dd33

I just noticed we also need to update the CI test-blockchains job to add the use-esplora-ureq or use-esplora-request features. I'll add a commit for this before merging.

@notmandatory
notmandatory self-requested a review August 26, 2021 10:06
@notmandatory
notmandatory dismissed their stale reviewAugust 26, 2021 10:06

Need to update CI and fix hanging

Recently we added sub-modules to the `esplora` module for different HTTP
backends. We (Tobin) failed to update the integration test runner.
Modify the test running macro so that the backend is explicitly
selected:
`cargo test --no-default-features --features=use-esplora-ureq,test-esplora`
@notmandatory
notmandatoryforce-pushed the esplora-integration-test branch from 1f6dd33 to ad5a9f8CompareAugust 26, 2021 20:43
@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks @notmandatory, picking up the pieces for me :)

@notmandatory

Copy link
Copy Markdown
Member

I think @rajarshimaitra's #430 will go in first then we can add this change. 🙂

@notmandatorynotmandatory mentioned this pull request Sep 1, 2021
6 tasks
@tcharding

Copy link
Copy Markdown
ContributorAuthor

#433 solves this issue in a better way.

@tcharding
tcharding deleted the esplora-integration-test branch October 13, 2023 00:41
Sign up for freeto 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.

3 participants

@tcharding@RCasatta@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix esplora integration test runner - #415

Closed
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test
Closed

Fix esplora integration test runner#415
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test

Conversation

@tcharding

@tchardingtcharding commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

Description

Recently we added sub-modules to the esplora module for different HTTP
backends. We (Tobin) failed to update the integration test runner.

Modify the test running macro so that the backend is explicitly
selected:

cargo test --no-default-features --features=use-esplora-ureq,test-esplora

Notes to the reviewers

I can't actually get the integration tests to run to completion. They all (test-rpc, test-electrum, test-esplora) hang

$ echo $BITCOIND_EXE ✘ 130 /opt/bitcoin/bin/bitcoind
$ echo $ELECTRS_EXE
/home/tobin/.cargo/bin/electrs
$ cargo test --no-default-features --features=use-esplora-ureq,test-esplora
test blockchain::esplora::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

However without this patch the integration tests do not run at all for esplora so I believe this is still correct. And it seems like a good way to ask if I am doing something wrong with how I'm trying to run the integration tests? Any suggestions please?

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@tcharding
tchardingforce-pushed the esplora-integration-test branch 2 times, most recently from 1053f07 to d28765dCompareAugust 2, 2021 08:15
@RCasatta

Copy link
Copy Markdown
Contributor

I think the hanging is more likely due to the esplora version pointed by your env var. For esplora it's needed a specific electrs fork (branch new-index in https://github.com/blockstream/electrs).

bitcoind and electrsd can run with env var set or by setting a specific feature that during the build process download the needed executables. At the moment the feature is used in our tests, see:

test-esplora = ["electrsd/legacy", "electrsd/esplora_a33e97e1", "test-blockchains"]

a33e97e1 stands for the specific commit of electrs, so you can either use the feature, or build electrs at that commit. Also electrsd/legacy feature is needed because there has been changes in electrs parameters...

@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks for the tips @RCasatta, I tried as you suggested (with no env vars set) but still tests hang. Can see from the log output that its pulling down bitcoind/electrs to run the tests. Any other ideas please?

$ cargo test --features test-electrum
...
[2021-08-02T22:27:17Z DEBUG bdk::testutils::blockchain_tests] launching /home/tobin/.cargo/bitcoin/bitcoin-0.21.1/bin/bitcoind and /home/tobin/.cargo/electrs/electrs_linux_v0.8.10/electrs
...
[2021-08-02T22:27:18Z DEBUG electrsd] args: ["-vvv", "--db-dir", "/tmp/.tmphAsIio", "--network", "regtest", "--cookie-file", "/tmp/.tmpDleo7B/regtest/.cookie", "--daemon-rpc-addr", "127.0.0.1:42461", "--jsonrpc-import", "--electrum-rpc-addr", "0.0.0.0:46495", "--monitoring-addr", "0.0.0.0:44203"]
test blockchain::electrum::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

@RCasatta

RCasatta commented Aug 3, 2021

Copy link
Copy Markdown
Contributor

I can't reproduce on my system and neither on the CI (run in this PR looks failing for unrelated HTTP issue, this one https://github.com/RCasatta/bdk/actions/runs/1093394555 looks fine)

Maybe some more details on your system could help to reproduce.

Also, maybe running this:

RUST_LOG=bdk=debug cargo test --features test-electrum -- test_sync_address_reuse

also with this patch:

diff --git a/src/testutils/blockchain_tests.rs b/src/testutils/blockchain_tests.rs
index 2df72d3..e374cb0 100644
--- a/src/testutils/blockchain_tests.rs
+++ b/src/testutils/blockchain_tests.rs
@@ -24,12 +24,17 @@ pub struct TestClient {
impl TestClient {
pub fn new(bitcoind_exe: String, electrs_exe: String) -> Self {
debug!("launching {} and {}", &bitcoind_exe, &electrs_exe);
- let bitcoind = BitcoinD::new(bitcoind_exe).unwrap();
+ let conf = bitcoind::Conf {
+ view_stdout: true,
+ ..Default::default()
+ };
+ let bitcoind = BitcoinD::with_conf(bitcoind_exe).unwrap();
let http_enabled = cfg!(feature = "test-esplora");
let conf = Conf {
http_enabled,
+ view_stderr: true,
..Default::default()
};
let electrsd = ElectrsD::with_conf(electrs_exe, &bitcoind, &conf).unwrap();

May give insight on what's happening under the hood

UPDATE: the patch seems useful to have in case you have a Debug log level, so I opened a PR #417

@tcharding
tchardingforce-pushed the esplora-integration-test branch from d28765d to 1f6dd33CompareAugust 4, 2021 00:34
notmandatory
notmandatory previously approved these changes Aug 26, 2021

@notmandatorynotmandatory left a comment

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.

Works for me locally (osx) and in CI so ACK 1f6dd33

I just noticed we also need to update the CI test-blockchains job to add the use-esplora-ureq or use-esplora-request features. I'll add a commit for this before merging.

@notmandatory
notmandatory self-requested a review August 26, 2021 10:06
@notmandatory
notmandatory dismissed their stale reviewAugust 26, 2021 10:06

Need to update CI and fix hanging

Recently we added sub-modules to the `esplora` module for different HTTP
backends. We (Tobin) failed to update the integration test runner.
Modify the test running macro so that the backend is explicitly
selected:
`cargo test --no-default-features --features=use-esplora-ureq,test-esplora`
@notmandatory
notmandatoryforce-pushed the esplora-integration-test branch from 1f6dd33 to ad5a9f8CompareAugust 26, 2021 20:43
@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks @notmandatory, picking up the pieces for me :)

@notmandatory

Copy link
Copy Markdown
Member

I think @rajarshimaitra's #430 will go in first then we can add this change. 🙂

@notmandatorynotmandatory mentioned this pull request Sep 1, 2021
6 tasks
@tcharding

Copy link
Copy Markdown
ContributorAuthor

#433 solves this issue in a better way.

@tcharding
tcharding deleted the esplora-integration-test branch October 13, 2023 00:41
Sign up for freeto 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.

3 participants

@tcharding@RCasatta@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix esplora integration test runner - #415

Closed
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test
Closed

Fix esplora integration test runner#415
tcharding wants to merge 1 commit into
bitcoindevkit:masterfrom
tcharding:esplora-integration-test

Conversation

@tcharding

@tchardingtcharding commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

Description

Recently we added sub-modules to the esplora module for different HTTP
backends. We (Tobin) failed to update the integration test runner.

Modify the test running macro so that the backend is explicitly
selected:

cargo test --no-default-features --features=use-esplora-ureq,test-esplora

Notes to the reviewers

I can't actually get the integration tests to run to completion. They all (test-rpc, test-electrum, test-esplora) hang

$ echo $BITCOIND_EXE ✘ 130 /opt/bitcoin/bin/bitcoind
$ echo $ELECTRS_EXE
/home/tobin/.cargo/bin/electrs
$ cargo test --no-default-features --features=use-esplora-ureq,test-esplora
test blockchain::esplora::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::esplora::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

However without this patch the integration tests do not run at all for esplora so I believe this is still correct. And it seems like a good way to ask if I am doing something wrong with how I'm trying to run the integration tests? Any suggestions please?

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@tcharding
tchardingforce-pushed the esplora-integration-test branch 2 times, most recently from 1053f07 to d28765dCompareAugust 2, 2021 08:15
@RCasatta

Copy link
Copy Markdown
Contributor

I think the hanging is more likely due to the esplora version pointed by your env var. For esplora it's needed a specific electrs fork (branch new-index in https://github.com/blockstream/electrs).

bitcoind and electrsd can run with env var set or by setting a specific feature that during the build process download the needed executables. At the moment the feature is used in our tests, see:

test-esplora = ["electrsd/legacy", "electrsd/esplora_a33e97e1", "test-blockchains"]

a33e97e1 stands for the specific commit of electrs, so you can either use the feature, or build electrs at that commit. Also electrsd/legacy feature is needed because there has been changes in electrs parameters...

@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks for the tips @RCasatta, I tried as you suggested (with no env vars set) but still tests hang. Can see from the log output that its pulling down bitcoind/electrs to run the tests. Any other ideas please?

$ cargo test --features test-electrum
...
[2021-08-02T22:27:17Z DEBUG bdk::testutils::blockchain_tests] launching /home/tobin/.cargo/bitcoin/bitcoin-0.21.1/bin/bitcoind and /home/tobin/.cargo/electrs/electrs_linux_v0.8.10/electrs
...
[2021-08-02T22:27:18Z DEBUG electrsd] args: ["-vvv", "--db-dir", "/tmp/.tmphAsIio", "--network", "regtest", "--cookie-file", "/tmp/.tmpDleo7B/regtest/.cookie", "--daemon-rpc-addr", "127.0.0.1:42461", "--jsonrpc-import", "--electrum-rpc-addr", "0.0.0.0:46495", "--monitoring-addr", "0.0.0.0:44203"]
test blockchain::electrum::bdk_blockchain_tests::test_sync_address_reuse has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_after_send has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_before_and_after_receive has been running for over 60 seconds
test blockchain::electrum::bdk_blockchain_tests::test_sync_bump_fee_add_input_no_change has been running for over 60 seconds

@RCasatta

RCasatta commented Aug 3, 2021

Copy link
Copy Markdown
Contributor

I can't reproduce on my system and neither on the CI (run in this PR looks failing for unrelated HTTP issue, this one https://github.com/RCasatta/bdk/actions/runs/1093394555 looks fine)

Maybe some more details on your system could help to reproduce.

Also, maybe running this:

RUST_LOG=bdk=debug cargo test --features test-electrum -- test_sync_address_reuse

also with this patch:

diff --git a/src/testutils/blockchain_tests.rs b/src/testutils/blockchain_tests.rs
index 2df72d3..e374cb0 100644
--- a/src/testutils/blockchain_tests.rs
+++ b/src/testutils/blockchain_tests.rs
@@ -24,12 +24,17 @@ pub struct TestClient {
impl TestClient {
pub fn new(bitcoind_exe: String, electrs_exe: String) -> Self {
debug!("launching {} and {}", &bitcoind_exe, &electrs_exe);
- let bitcoind = BitcoinD::new(bitcoind_exe).unwrap();
+ let conf = bitcoind::Conf {
+ view_stdout: true,
+ ..Default::default()
+ };
+ let bitcoind = BitcoinD::with_conf(bitcoind_exe).unwrap();
let http_enabled = cfg!(feature = "test-esplora");
let conf = Conf {
http_enabled,
+ view_stderr: true,
..Default::default()
};
let electrsd = ElectrsD::with_conf(electrs_exe, &bitcoind, &conf).unwrap();

May give insight on what's happening under the hood

UPDATE: the patch seems useful to have in case you have a Debug log level, so I opened a PR #417

@tcharding
tchardingforce-pushed the esplora-integration-test branch from d28765d to 1f6dd33CompareAugust 4, 2021 00:34
notmandatory
notmandatory previously approved these changes Aug 26, 2021

@notmandatorynotmandatory left a comment

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.

Works for me locally (osx) and in CI so ACK 1f6dd33

I just noticed we also need to update the CI test-blockchains job to add the use-esplora-ureq or use-esplora-request features. I'll add a commit for this before merging.

@notmandatory
notmandatory self-requested a review August 26, 2021 10:06
@notmandatory
notmandatory dismissed their stale reviewAugust 26, 2021 10:06

Need to update CI and fix hanging

Recently we added sub-modules to the `esplora` module for different HTTP
backends. We (Tobin) failed to update the integration test runner.
Modify the test running macro so that the backend is explicitly
selected:
`cargo test --no-default-features --features=use-esplora-ureq,test-esplora`
@notmandatory
notmandatoryforce-pushed the esplora-integration-test branch from 1f6dd33 to ad5a9f8CompareAugust 26, 2021 20:43
@tcharding

Copy link
Copy Markdown
ContributorAuthor

Thanks @notmandatory, picking up the pieces for me :)

@notmandatory

Copy link
Copy Markdown
Member

I think @rajarshimaitra's #430 will go in first then we can add this change. 🙂

@notmandatorynotmandatory mentioned this pull request Sep 1, 2021
6 tasks
@tcharding

Copy link
Copy Markdown
ContributorAuthor

#433 solves this issue in a better way.

@tcharding
tcharding deleted the esplora-integration-test branch October 13, 2023 00:41
Sign up for freeto 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.

3 participants

@tcharding@RCasatta@notmandatory