Conversation
oxarbitrage
left a comment
There was a problem hiding this comment.
Thanks for the PR, it seems to be some good work here, but it’s currently trying to do too much at once, which makes it hard to identify and evaluate the core idea.
I’d recommend splitting this into smaller, focused PRs:
-
Separate the new RPC method: the
dump_priv_keyRPC should live in its own PR so it can be reviewed independently. -
Reduce the test scope for now: instead of adding many tests, please include a single test that illustrates how the framework is intended to be used. Additional coverage can be added incrementally once the framework itself is approved.
-
Add a high-level explanation of the framework: a short description of what the framework does, how it’s meant to be used, and what problem it’s solving would really help reviewers understand the intent and design.
Adds a reusable test framework (`TestWallet`) that creates in-memory wallets backed by SQLite for isolated integration tests of RPC methods. Each test creates a fresh `TestWallet` with a unique database name, so `cargo test` can run all tests in parallel without interference. As an example, stateful wallet tests are included in this commit for `z_listaccounts` and for a few keystore operations.
43e29d8 to
91ee5e1
Compare
|
Apologies I pushed this draft WIP up quickly before rushing to a meeting. I have updated the draft to be more focused on the proposed testing approach, with a few working examples. I will submit a separate PR for the dumpprivkey WIP which leaked into my rushed push. Here's an overview and how to use it currently:
Each test gets its own isolated in-memory SQLite database. An atomic counter ensures unique database names across parallel test runs. The connection pool is configured with
All integration tests use This is a draft intended to spark discussion and is not intended to represent a final implementation ready for merge. TestWallet Usage ExamplesThis draft includes working stateful integration tests for Generally, this is how to use TestWallet: let wallet = TestWallet::new(Network::Consensus(consensus::Network::MainNetwork))
.await.unwrap();
let account = wallet.account_builder()
.with_name("test")
.build()
.await.unwrap();
let handle = wallet.handle().await.unwrap();
// Call your RPC method with handle.as_ref()
let result = list_accounts::call(handle.as_ref(), Some(true)).unwrap();
assert_eq!(result.0.len(), 1);The mnemonic lifecycle test is another good overview of the utility having a stateful wallet and keystore around for tests. |
|
@emersonian you likely missed the many discussions we had last year about integration testing. The approach we had planned on doing is to reuse the large suite of integration tests from The next step is getting them working in a way that is not tied to the Zebra repo, by having a standalone repository for the RPC tests that is automatically triggered to run whenever PRs are opened on either Zebra or Zallet (and reports results back to those PRs). I thought someone had started working on this but I cannot find it anywhere, so I will probably pick it up myself (as I have implemented precisely this kind of workflow before). Regarding this PR, I do not consider it to be integration testing, because it is not testing the Zallet binary in integration with a full node (which is the only way that Zallet can correctly run). You appear to instead be combining sub-components of the internals of Zallet together to implement stateful black-box testing of those sub-components. This is similar to the testing framework we have in |
|
Yes as a new zallet contributor I did miss those discussions. Thank you for the overview. I agree that "integration" is not the right word for the proposed ability to test with a stateful wallet within the same codebase and language. The tests you describe I would consider to be "end to end tests" working a system across multiple projects and languages. My fear is that as we approach implementing features that require wallet state, they are written without true test coverage of their intended functionality within this language and project. Tests are incredibly valuable for developer productivity, confidence, and for documenting intended behavior as code. We are potentially about to welcome way more contributors into this codebase and I believe that keeping tests close can greatly simplify code reviews, and a low friction testing stack can maintain high contributor morale. How do you propose that functionality requiring a wallet is tested within Zallet? Only using the E2E approach in progress, or do you have a vision of how stateful wallet-requiring unit tests can happen. (Using "unit" word this time) |
This is inaccurate. Zallet's RPC is to be largely compatible with Zcashd. My proposed tests will document the expected output of Zallet, and validate that its output is indeed compatible with Zcashd, including in its error messages. This is the right project to hold the tests for Zallet's expected behavior. What you describe as a "black box" is intentional: we are not testing the crates you mentioned, their functionality is an assumed-correct black box from the perspective of Zallet. We are testing that Zallet's output is correct, and that part of each test is not at all a "black box". An increasing need is for those tests to check things like transaction histories and balances, and I believe that requiring running a full E2E test across multiple languages and projects to validate such things in-development will slow down progress and discourage contributors. Developers need a test suite which can run quickly and minimally while work is in progress, and is easy to understand. |
Zallet developers need to be able to write stateful integration tests to match the level of test coverage that zcashd has. Our most essential RPC methods do not yet have integration test coverage (tests using actual wallet state).
Today our integration testing process is looking very manual, potentially burning a lot of dev hours: #364 (comment)
This PR is a first attempt at implementing an in-memory SQLite wallet for integration testing purposes.
I think we should create an issue to properly implement integration testing sooner than later.
I'm happy to iterate on this first attempt based on your feedback, or to close this PR. I'm mostly opening it to raise this as a conversation topic.