Skip to content

Conversation

@riverKanies
Copy link
Contributor

@riverKanies riverKanies commented Nov 18, 2024

Description

This PR is a proposed solution to bitcoindevkit/bdk_wallet#43
It enforces running tests that use bitcoind or electrsd in series (rather than parallel)

Notes to the reviewers

Originally I had suggested just including a test troubleshooting section in docs to show ways to run in series

Bugfixes:

  • I'm linking the issue being fixed by this PR

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

Even though it's under dev-dependencies and being used exclusively for tests, I'm not entirely sure if we do need this one. Are there any major improvements that it adds to the tests?

@riverKanies
Copy link
Contributor Author

@oleonardolima this is so tests work with cargo test (on mac or wherever). I guess it could make tests on linux a little slower if it doesn't have the issue running parallel. I softof feel that it makes sense to run blockchain client tests in series regardless since tests would be sharing state otherwise. maybe linux just does this by default, which would mean there's no performance cost

@riverKanies
Copy link
Contributor Author

riverKanies commented Nov 20, 2024

should we update rustc version? https://github.com/bitcoindevkit/bdk/actions/runs/11896104128/job/33175482146?pr=1728

I can just downgrade serial_test for now

update: setting serial_test = { version = "=3.0.0" } to remove scc dep which requires rustc 1.65

@riverKanies riverKanies force-pushed the feature(testenv)/run-regtest-tests-in-series branch from 9a48794 to a8e1095 Compare November 20, 2024 01:00
@riverKanies
Copy link
Contributor Author

riverKanies commented Nov 21, 2024

hmm, looks like 3.0.0 also has a dep on 1.65
ended up having to force my local to build on 1.63 and set msrv and try every version all the way down to 0.8.0 😅

(build failed again https://github.com/bitcoindevkit/bdk/actions/runs/11924661651/job/33299306353?pr=1728)

@riverKanies riverKanies force-pushed the feature(testenv)/run-regtest-tests-in-series branch from a8e1095 to 7caa75f Compare November 21, 2024 15:09
@riverKanies riverKanies force-pushed the feature(testenv)/run-regtest-tests-in-series branch from 7caa75f to 391452a Compare November 21, 2024 15:49
@notmandatory
Copy link
Member

notmandatory commented Nov 21, 2024

As mentioned on the issue I'm not on board with serializing all tests. This is probably something better to just document as a caveat for MacOS users so we can still by default run tests in parallel for linux users (and CI).

@riverKanies
Copy link
Contributor Author

@notmandatory ok, I was wondering if it would effect performance. I was thinking maybe linux ran in series by default or something and that's why we're only seeing this on mac. I'll close this and we can just document the issue in the other pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants