Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature(testenv)/run regtest tests in series #1728

Conversation

riverKanies
Copy link
Contributor

@riverKanies riverKanies commented Nov 18, 2024

Description

This PR is a proposed solution to #1723
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

@riverKanies riverKanies force-pushed the feature(testenv)/run-regtest-tests-in-series branch 2 times, most recently from 9c33c3d to 9a48794 Compare November 18, 2024 15:52
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
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants