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

Drop OnceCell in lightning-transaction-sync tests #2132

Merged

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Mar 27, 2023

Unfortunately, OnceCell never calls drop upon termination, which makes the spawned bitcoind/electrsd instances linger around after our tests have finished. This had previously led to spurious failures such as:

---- test_esplora_syncs stdout ----
thread 'test_esplora_syncs' panicked at 'called `Result::unwrap()` on an `Err` value: JsonRpc(Rpc(RpcError { code: -4, message: "Wallet file verification failed. SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of Bitcoin Core?\n", data: None }))', tests/integration_tests.rs:35:65
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

To fix this, we simply move the instances out of OnceCell and let every test that needs them spawn their own instances. This might introduce a small overhead over the shared-state version, which however is likely not that bad as long as the number of tests is not growing immensely. It additional lets us drop the OnceCell dev dependency.

@TheBlueMatt
Copy link
Collaborator

This looks good too, but I thought previously you'd decided the runtime of the tests was too high this way? We could still pretty trivially make this globals by just having each test set some flag on completion (or panic) + clean everything off when a defined number of tests complete?

@tnull
Copy link
Contributor Author

tnull commented Mar 28, 2023

This looks good too, but I thought previously you'd decided the runtime of the tests was too high this way? We could still pretty trivially make this globals by just having each test set some flag on completion (or panic) + clean everything off when a defined number of tests complete?

Well, currently the the test cases are partitioned based on feature flags ( async/blocking) anyways as we can't import both features at the same time. We therefore only have two test cases per scenario of which only one requires pre-mining blocks, which is the time-consuming part. Just did some local benchmarks, and it barely makes a difference currently:

BASE sh test.sh  1.34s user 0.68s system 9% cpu 21.652 total
HEAD sh test.sh  2.34s user 1.34s system 15% cpu 23.717 total

There will be some more test cases introduced with the Electrum version, but it still probably would be an insignificant difference as it will also rely on feature flags, meaning that it will likely be tested via separate cargo test calls anyways.
So I'd just not bother introducing additional complexity currently and revisit if we were to add more test cases that would actually benefit from sharing state.

@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: +0.24 🎉

Comparison is base (dbf3d57) 91.42% compared to head (8df5719) 91.66%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2132      +/-   ##
==========================================
+ Coverage   91.42%   91.66%   +0.24%     
==========================================
  Files         101      101              
  Lines       49552    51789    +2237     
  Branches    49552    51789    +2237     
==========================================
+ Hits        45304    47474    +2170     
- Misses       4248     4315      +67     

see 8 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tnull
Copy link
Contributor Author

tnull commented Mar 28, 2023

CI failures are unrelated, see #2133.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to squash the fixup.

tnull added 2 commits March 28, 2023 17:13
`OnceCell` doesn't call `drop`, which makes the spawned
`bitcoind`/`electrsd` instances linger around after our tests have
finished. To fix this, we move them out of `OnceCell` and let every test
that needs them spawn their own instances. This additional let us drop
the `OnceCell` dev dependency.
@tnull tnull force-pushed the 2023-03-tx-sync-even-more-robustness branch from 8df5719 to 7b85eba Compare March 28, 2023 15:14
@tnull
Copy link
Contributor Author

tnull commented Mar 28, 2023

Squashed without further changes.

@TheBlueMatt TheBlueMatt merged commit 369eea4 into lightningdevkit:main Mar 28, 2023
Sign up for free to 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.

4 participants