Generalize the `zcash_client_sqlite` test framework and extract it to `zcash_client_backend` #1530

nuttycom · 2024-09-05T20:55:33Z

As a side-effect, this fixes #1260.

…ount` trait.

…dor` for testing.

codecov · 2024-09-05T21:55:15Z

Codecov Report

Attention: Patch coverage is 67.52874% with 339 lines in your changes missing coverage. Please review.

Project coverage is 61.26%. Comparing base (db06580) to head (cd71c30).
Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
zcash_client_backend/src/data_api/testing.rs	66.12%	271 Missing ⚠️
...ash_client_backend/src/data_api/testing/orchard.rs	0.00%	30 Missing ⚠️
zcash_client_sqlite/src/wallet.rs	65.71%	12 Missing ⚠️
zcash_client_sqlite/src/lib.rs	61.53%	10 Missing ⚠️
zcash_client_backend/src/data_api/testing/pool.rs	91.30%	8 Missing ⚠️
zcash_client_backend/src/data_api.rs	25.00%	6 Missing ⚠️
...ash_client_backend/src/data_api/testing/sapling.rs	95.91%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1530      +/-   ##
==========================================
+ Coverage   61.07%   61.26%   +0.18%     
==========================================
  Files         142      146       +4     
  Lines       16710    16890     +180     
==========================================
+ Hits        10205    10347     +142     
- Misses       6505     6543      +38

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… moved to `zcash_client_backend`

…from TestState

…_client_sqlite`

…t_sqlite`

…st from `zcash_client_sqlite`

zcash_client_backend/src/data_api/testing.rs

str4d

Reviewed d4e26d5.

str4d · 2024-09-09T11:37:28Z

zcash_client_backend/src/data_api.rs

@@ -346,9 +346,11 @@ pub enum AccountSource {
 }

 /// A set of capabilities that a client account must provide.
-pub trait Account<AccountId: Copy> {
+pub trait Account {
+    type AccountId: Copy;


Why was this made a type parameter in the first place? I can't immediately think of a reason why an implementation of the Data Access API would have two different Account implementations with different IDs.

str4d · 2024-09-09T11:58:09Z

zcash_client_backend/src/data_api.rs

@@ -662,12 +662,22 @@ impl<NoteRef> SpendableNotes<NoteRef> {
        self.sapling.as_ref()
    }

+    /// Consumes this value and returns the Sapling notes contained within it.
+    pub fn take_sapling(self) -> Vec<ReceivedNote<NoteRef, sapling::Note>> {


Document this addition in the changelog (it was added in the ambassador commit and IDK why yet).

It is used in conjunction with the fix to #1260 inside the implementation of ShieldedPoolTester for Sapling (and similarly for Orchard). So it could instead be made crate-private now that SaplingPoolTester is also in zcash_client_backend instead of documenting it as an addition.

str4d · 2024-09-09T11:58:13Z

zcash_client_backend/src/data_api.rs

    /// Returns the set of spendable Orchard notes.
    #[cfg(feature = "orchard")]
    pub fn orchard(&self) -> &[ReceivedNote<NoteRef, orchard::note::Note>] {
        self.orchard.as_ref()
    }

+    /// Consumes this value and returns the Orchard notes contained within it.
+    pub fn take_orchard(self) -> Vec<ReceivedNote<NoteRef, orchard::note::Note>> {


Document this addition in the changelog (it was added in the ambassador commit and IDK why yet).

str4d · 2024-09-09T12:06:14Z

zcash_client_sqlite/src/lib.rs

-                anchor_height,
-                exclude,
-            )?,
+            if sources.contains(&ShieldedProtocol::Sapling) {


This commit (db6b970) silently fixes #1260.

zcash_client_sqlite/src/testing/db.rs

str4d · 2024-09-09T17:20:17Z

zcash_client_backend/src/data_api/testing/sapling.rs

+
+use super::{pool::ShieldedPoolTester, TestState};
+
+pub struct SaplingPoolTester;


The struct needs documentation now that it is in the public API.

Addressed in #1539.

str4d · 2024-09-09T17:20:44Z

zcash_client_backend/src/data_api/testing/pool.rs

+
+/// Trait that exposes the pool-specific types and operations necessary to run the
+/// single-shielded-pool tests on a given pool.
+pub trait ShieldedPoolTester {


The trait's associated types and methods need documentation now that it is in the public API.

Addressed in #1539.

str4d · 2024-09-09T17:23:39Z

zcash_client_backend/src/data_api/testing/orchard.rs

+    wallet::{Note, ReceivedNote},
+};
+
+pub struct OrchardPoolTester;


The struct needs documentation now that it is in the public API.

Addressed in #1539.

zcash_client_backend/src/data_api/testing.rs

str4d · 2024-09-09T17:31:24Z

zcash_client_backend/src/data_api/testing/pool.rs

@@ -88,3 +98,143 @@ pub trait ShieldedPoolTester {

    fn received_note_count(summary: &ScanSummary) -> usize;
 }
+
+pub fn send_single_step_proposed_transfer<T: ShieldedPoolTester>(


Needs documentation now that it is in the public API.

Addressed in #1539.

str4d

ACK cd71c30. The remaining review comments are doc-only, and will be addressed in a subsequent PR.

Addresses review comments from #1530.

nuttycom added 3 commits September 4, 2024 15:38

zcash_client_backend: Fix broken --all-features build.

026f5f6

zcash_client_backend: Make AccountId an associated type of the `Acc…

69953cc

…ount` trait.

zcash_client_backend: Make data_api traits delegatable via `ambassa…

0ae5ac1

…dor` for testing.

nuttycom force-pushed the generalized_test_framework branch from 6b6b310 to 33ba129 Compare September 5, 2024 21:36

nuttycom marked this pull request as ready for review September 6, 2024 03:52

nuttycom requested review from str4d and daira September 6, 2024 04:06

nuttycom force-pushed the generalized_test_framework branch from 647c266 to 4a3a52b Compare September 6, 2024 14:44

nuttycom added 10 commits September 6, 2024 13:29

zcash_client_sqlite: Generalize the test framework to enable it to be…

db6b970

… moved to `zcash_client_backend`

zcash_client_backend: Move data_api::testing module into its own file.

b43f3bf

zcash_client_sqlite: Move TransactionSummary to zcash_client_backend

acd26d5

zcash_client_sqlite: Generalize more TestState operations.

58b464d

zcash_client_sqlite: Generalize TestState::get_wallet_summary

15e124e

zcash_client_sqlite: Remove the remainder of the sqlite dependencies …

ce59a67

…from TestState

zcash_client_sqlite: Generalize TestBuilder::with_block_cache

7e9f78b

zcash_client_sqlite: Move TestState to zcash_client_backend

e55df6c

zcash_client_backend: Record audit of ambassador crate.

2be0dfb

zcash_client_backend: Fix broken intra-doc links and other doc warnings.

f2654f5

nuttycom force-pushed the generalized_test_framework branch from baade4f to f2654f5 Compare September 6, 2024 19:33

nuttycom added 4 commits September 6, 2024 14:00

zcash_client_sqlite: Remove unused OutputRecoveryError type.

49dffbf

zcash_client_backend: Move the ShieldedPoolTester trait from `zcash…

4f5b3ef

…_client_sqlite`

zcash_client_backend: Move SaplingPoolTester here from `zcash_clien…

7e36561

…t_sqlite`

zcash_client_backend: Move OrchardPoolTester here from `zcash_clien…

33b8f89

…t_sqlite`

nuttycom force-pushed the generalized_test_framework branch from 44d9fc5 to 33b8f89 Compare September 6, 2024 21:37

nuttycom mentioned this pull request Sep 6, 2024

Introduce a Backend type for tests #1526

Closed

zcash_client_backend: Migrate send_single_step_proposed_transfer te…

d4e26d5

…st from `zcash_client_sqlite`

str4d mentioned this pull request Sep 9, 2024

zcash_client_backend: Fix broken --all-features build. #1529

Merged

oxarbitrage mentioned this pull request Sep 9, 2024

Migrate test framework from zcash_client_sqlite to zcash_client_backend #1412

Closed

willemolding reviewed Sep 9, 2024

View reviewed changes

zcash_client_backend/src/data_api/testing.rs Outdated Show resolved Hide resolved

str4d requested changes Sep 9, 2024

View reviewed changes

Address non-documentation review comments

cd71c30

str4d approved these changes Sep 10, 2024

View reviewed changes

str4d merged commit c97e9a1 into zcash:main Sep 10, 2024
27 checks passed

str4d mentioned this pull request Sep 12, 2024

zcash_client_backend: Post-merge cleanups to test framework extraction #1539

Merged

str4d added a commit that referenced this pull request Sep 16, 2024

zcash_client_backend: Add missing documentation to data_api::testing

099025d

Addresses review comments from #1530.

str4d added a commit that referenced this pull request Sep 16, 2024

zcash_client_backend: Add missing documentation to data_api::testing

ff7f478

Addresses review comments from #1530.

str4d added a commit that referenced this pull request Sep 16, 2024

zcash_client_backend: Add missing documentation to data_api::testing

63c1de3

Addresses review comments from #1530.

nuttycom deleted the generalized_test_framework branch September 20, 2024 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize the `zcash_client_sqlite` test framework and extract it to `zcash_client_backend` #1530

Generalize the `zcash_client_sqlite` test framework and extract it to `zcash_client_backend` #1530

nuttycom commented Sep 5, 2024 •

edited by str4d

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

str4d left a comment

str4d Sep 9, 2024

str4d Sep 9, 2024

str4d Sep 9, 2024

str4d Sep 9, 2024

str4d Sep 9, 2024

str4d Sep 9, 2024

str4d Sep 16, 2024

str4d Sep 9, 2024

str4d Sep 16, 2024

str4d Sep 9, 2024

str4d Sep 16, 2024

str4d Sep 9, 2024

str4d Sep 16, 2024

str4d left a comment


		use super::{pool::ShieldedPoolTester, TestState};

		pub struct SaplingPoolTester;

Generalize the zcash_client_sqlite test framework and extract it to zcash_client_backend #1530

Generalize the zcash_client_sqlite test framework and extract it to zcash_client_backend #1530

Conversation

nuttycom commented Sep 5, 2024 • edited by str4d Loading

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Generalize the `zcash_client_sqlite` test framework and extract it to `zcash_client_backend` #1530

Generalize the `zcash_client_sqlite` test framework and extract it to `zcash_client_backend` #1530

nuttycom commented Sep 5, 2024 •

edited by str4d

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading