Enhance Test Coverage for Receipt Insertion, Batch Processing, and Sender Accounts #572

carlosvdr · 2025-01-06T16:01:12Z

This new tests cover are here to enhance the testing experience and help ensure tap V2 wont affect the processes in a "bigger picture"
As well as added more test types around receiving receipts. For this a new method was added regarding batch inserts for receipts into the DB, trying to be have a more accurate representation of how TAP works on a day to day basis.

Regarding the tests on the "bigger picture" , tests for several layers were added.
Such as a full process from sender account all the way from starting it, creating a sender allocation up to the point of marking allocation as last for redeeming.

Another test added from the sender account manager layer which tries to do the same process as the previous layer but starting from the manager.

And finally a test for the full Tap Agent and simulate as closely as possible the full process of it

coveralls · 2025-01-06T16:06:51Z

Pull Request Test Coverage Report for Build 12835420725

Details

239 of 250 (95.6%) changed or added relevant lines in 5 files are covered.
5 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.4%) to 78.022%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/tap-agent/src/agent/sender_accounts_manager.rs	47	49	95.92%
crates/tap-agent/src/test.rs	54	56	96.43%
crates/tap-agent/src/agent/sender_account.rs	38	41	92.68%
crates/tap-agent/src/tap_agent_test.rs	93	97	95.88%

Files with Coverage Reduction	New Missed Lines	%
crates/tap-agent/src/agent/sender_account.rs	1	84.41%
crates/watcher/src/lib.rs	4	92.71%

Totals
Change from base Build 12810605983:	0.4%
Covered Lines:	7015
Relevant Lines:	8991

💛 - Coveralls

suchapalaver

@carlosvdr can you squash/fix the commit history, please 🙏

suchapalaver · 2025-01-14T21:14:30Z

@carlosvdr , approving just to remove the block 👍

suchapalaver · 2025-01-14T21:15:55Z

crates/tap-agent/src/agent/sender_account.rs

+    async fn mock_escrow_subgraph_empty_response() -> (MockServer, MockGuard) {
+        let mock_ecrow_subgraph_server: MockServer = MockServer::start().await;
+        let _mock_ecrow_subgraph = mock_ecrow_subgraph_server
+            .register_as_scoped(
+                Mock::given(method("POST"))
+                    .and(body_string_contains("TapTransactions"))
+                    .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "data": {
+                            "transactions": [],
+                        }
+                    }))),
+            )
+            .await;
+        (mock_ecrow_subgraph_server, _mock_ecrow_subgraph)
+    }
+


Did you consider implementing this in a "lazy" way since it could be called by multiple tests in parallel and is deterministic? (https://stackoverflow.com/a/59614532/24786256)

suchapalaver · 2025-01-14T21:18:07Z

crates/tap-agent/src/agent/sender_accounts_manager.rs

+    async fn mock_escrow_subgraph_empty_response() -> (MockServer, MockGuard) {
+        let mock_ecrow_subgraph_server: MockServer = MockServer::start().await;
+        let _mock_ecrow_subgraph = mock_ecrow_subgraph_server
+            .register_as_scoped(
+                Mock::given(method("POST"))
+                    //.and(body_string_contains("TapTransactions"))
+                    .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "data": {
+                            "transactions": [],
+                        }
+                    }))),
+            )
+            .await;
+        (mock_ecrow_subgraph_server, _mock_ecrow_subgraph)
+    }


I had the same thought about "lazy static" here as well: https://stackoverflow.com/a/59614532/24786256

suchapalaver · 2025-01-15T19:29:16Z

crates/tap-agent/src/agent/sender_account.rs

+    lazy_static! {
+        static ref MOCK_ESCROW_SUBGRAPH: Mutex<Option<Arc<(MockServer, MockGuard)>>> =
+            Mutex::new(None);
+        static ref MOCK_EMPTY_ESCROW_SUBGRAPH: Mutex<Option<Arc<(MockServer, MockGuard)>>> =
+            Mutex::new(None);
+    }
+
+    async fn mock_escrow_subgraph() -> Arc<(MockServer, MockGuard)> {
+        let mut guard = MOCK_ESCROW_SUBGRAPH.lock().unwrap();
+        if let Some(server) = guard.as_ref() {
+            server.clone()
+        } else {
+            let mock_ecrow_subgraph_server = MockServer::start().await;
+            let mock_ecrow_subgraph = mock_ecrow_subgraph_server
                .register_as_scoped(
                    Mock::given(method("POST"))
                        .and(body_string_contains("TapTransactions"))
                        .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "data": {
-                                "transactions": [{
-                                    "id": "0x00224ee6ad4ae77b817b4e509dc29d644da9004ad0c44005a7f34481d421256409000000"
-                                }],
-                            }
+                            "transactions": [{
+                                "id": "0x00224ee6ad4ae77b817b4e509dc29d644da9004ad0c44005a7f34481d421256409000000"
+                            }],
+                        }
                        }))),
                )
                .await;
-        (mock_ecrow_subgraph_server, _mock_ecrow_subgraph)
+            let server = Arc::new((mock_ecrow_subgraph_server, mock_ecrow_subgraph));
+            *guard = Some(server.clone());
+            server
+        }


@carlosvdr this is on the right track but I suggest looking at how others use OnceCell instead of the Mutex. I don't have time to try this myself right now, but I would expect things to look more like:

use lazy_static::lazy_static; use tokio::sync::{OnceCell, Mutex}; use std::sync::Arc; use wiremock::{Mock, MockServer, MockGuard, ResponseTemplate}; use wiremock::matchers::{method, body_string_contains}; use serde_json::json; lazy_static! { static ref MOCK_ESCROW_SUBGRAPH: OnceCell<Arc<(MockServer, MockGuard)>> = OnceCell::new(); } async fn mock_escrow_subgraph() -> Arc<(MockServer, MockGuard)> { MOCK_ESCROW_SUBGRAPH .get_or_try_init(async { let mock_server = MockServer::start().await; let mock_guard = mock_server .register_as_scoped( Mock::given(method("POST")) .and(body_string_contains("TapTransactions")) .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "data": { "transactions": [{ "id": "0x00224ee6ad4ae77b817b4e509dc29d644da9004ad0c44005a7f34481d421256409000000" }] } }))), ) .await; Ok::<_, wiremock::Error>(Arc::new((mock_server, mock_guard))) }) .await .unwrap() .clone() }

Thanks let me take a look into this

suchapalaver

I really want to see what @gusinacio thinks, @carlosvdr!

gusinacio · 2025-01-20T19:58:44Z

crates/tap-agent/src/agent/sender_accounts_manager.rs

+    lazy_static! {
+        static ref MOCK_EMPTY_ESCROW_SUBGRAPH: OnceCell<Arc<(MockServer, MockGuard)>> =
+            OnceCell::new();
+    }
+    async fn mock_escrow_subgraph_empty_response() -> Arc<(MockServer, MockGuard)> {
+        MOCK_EMPTY_ESCROW_SUBGRAPH
+            .get_or_try_init(|| async {
+                let mock_ecrow_subgraph_server: MockServer = MockServer::start().await;
+                let _mock_ecrow_subgraph = mock_ecrow_subgraph_server
+                    .register_as_scoped(
+                        Mock::given(method("POST"))
+                            .and(body_string_contains("TapTransactions"))
+                            .respond_with(ResponseTemplate::new(200).set_body_json(
+                                json!({ "data": {
+                                        "transactions": [],
+                                    }
+                                }),
+                            )),
+                    )
+                    .await;
+                Ok::<_, anyhow::Error>(Arc::new((mock_ecrow_subgraph_server, _mock_ecrow_subgraph)))
+            })
+            .await
+            .unwrap()
+            .clone()
+    }


Hey, I see what you did here, but I think it's not needed to use lazy_static, since it's used by a single test.

Also, since you are using register_as_scoped, it doesn't make sense to have a scoped route that is never dropped.

Maybe rstest is better for this context.

gusinacio · 2025-01-21T14:34:47Z

crates/tap-agent/src/tap_agent_test.rs

+    let actor = TestableActor::new(SenderAccountsManager);
+    let notify = actor.notify.clone();
+    (
+        prefix,
+        notify,
+        Actor::spawn(None, actor, args).await.unwrap(),
+        handle_aggregator,
+    )


Why do you need a TestableActor here? You don't seem to wait for any notification.

gusinacio · 2025-01-21T14:35:31Z

crates/tap-agent/src/tap_agent_test.rs

+// Copyright 2023-, Edge & Node, GraphOps, and Semiotic Labs.
+// SPDX-License-Identifier: Apache-2.0
+
+use std::{
+    collections::HashMap,
+    str::FromStr,
+    sync::{atomic::AtomicU32, Arc},
+    time::Duration,
+};
+
+use indexer_monitor::{DeploymentDetails, EscrowAccounts, SubgraphClient};
+use jsonrpsee::server::ServerHandle;
+use ractor::{call, concurrency::JoinHandle, Actor, ActorRef};


This files seems to me that it could be added to tests/integration_test.rs. It seems self-contained.

It all depends if you can use crate:: as indexer_tap_agent::. Maybe the only part that is harder is to migrate create_received_receipt and store_batch_receipts. Maybe we could add something in test-assets

Oh, it seems that you might also have trouble with SenderAllocationMessage::GetUnaggregatedReceipts

Ok, one suggestion that I have is the following:

add a feature flag called "test" in Cargo.toml

[features] test = []

in every #[cfg(cfg)] that we have (that are not tests), replace with #[cfg(any(test, feature = "test"))]

add in dev_dependencies in Cargo.toml

[dev_dependencies] crate_name = { path = ".", features = ["test"] }

I've tried this in the past but I got errors with rust-analyzer. Maybe this is solved, check it out if this works or please let me know the errors.

This is a little bit hacky, but rust doesn't have a straightforward way to do this.

I guess when you said "these files", you meant all those files that contain integration tests?
Also
I did tried separating this into another crate like tests/integration_tests, something similar in a separate refactor but it was too much of a hassle, this created some circular dependency issues

Try to create tests/integration_tests in tap-agent.

gusinacio · 2025-01-21T14:38:13Z

crates/tap-agent/src/tap_agent_test.rs

+    let mut sender_aggregator_endpoints: HashMap<Address, Url> = HashMap::new();
+    sender_aggregator_endpoints.insert(
+        TAP_SENDER.1,
+        Url::from_str(&format!("http://{}", aggregator_endpoint)).expect("This should not fail"),
+    );


Here comes a little cool trick.

let sender_aggregator_endpoints: HashMap<_, _> = vec![( TAP_SENDER.1, Url::from_str(&format!("http://{}", aggregator_endpoint)).unwrap() )].into_iter().collect();

gusinacio · 2025-01-21T17:07:53Z

crates/tap-agent/src/agent/sender_accounts_manager.rs

+    #[sqlx::test(migrations = "../../migrations")]
+    async fn test_account_manger_to_sender_allocation_closing(pgpool: PgPool) {
+        // Start a TAP aggregator server.
+        let (handle, aggregator_endpoint) = run_server(
+            0,
+            SIGNER.0.clone(),
+            vec![SIGNER.1].into_iter().collect(),
+            TAP_EIP712_DOMAIN_SEPARATOR.clone(),
+            100 * 1024,
+            100 * 1024,
+            1,
+        )
+        .await
+        .unwrap();
+
+        let aggregator_endpoint = Some(
+            Url::from_str(&("http://".to_owned() + &aggregator_endpoint.to_string()))
+                .expect("This shouldn't fail"),
+        );
+
+        let mock_network_subgraph_server: MockServer = MockServer::start().await;
+
+        mock_network_subgraph_server
+            .register(
+                Mock::given(method("POST"))
+                    .and(body_string_contains("ClosedAllocations"))
+                    .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "data": {
+                            "meta": {
+                                "block": {
+                                    "number": 1,
+                                    "hash": "hash",
+                                    "timestamp": 1


This test for me seems like an integration tests and not really unit tests. You are creating a SenderAcountManager, that is creating SenderAccounts, that is creating SenderAllocations.

This should be placed somewhere else IMO.

gusinacio · 2025-01-21T17:08:39Z

crates/tap-agent/src/agent/sender_accounts_manager.rs

@@ -761,7 +815,8 @@ mod tests {

    #[sqlx::test(migrations = "../../migrations")]
    async fn test_update_sender_allocation(pgpool: PgPool) {
-        let (prefix, notify, (actor, join_handle)) = create_sender_accounts_manager(pgpool).await;
+        let (prefix, notify, (actor, join_handle)) =
+            create_sender_accounts_manager(pgpool, None, None, None).await;


Nones all over the place seems a little bit of code smell.

Same as other, do you then recommend creating a different create_sender_account_manager
I thought so too, but was too much repeated code, went with the None instead to make it more "flexible"

Maybe use bon to update create_sender_accounts_manager then all None become default and you don't need to provide them.

gusinacio · 2025-01-21T17:59:41Z

crates/tap-agent/src/agent/sender_account.rs

+    lazy_static! {
+        static ref MOCK_EMPTY_ESCROW_SUBGRAPH: OnceCell<Arc<(MockServer, MockGuard)>> =
+            OnceCell::new();
+        static ref MOCK_ESCROW_SUBGRAPH: OnceCell<Arc<(MockServer, MockGuard)>> = OnceCell::new();
+    }
+
+    async fn mock_escrow_subgraph() -> Arc<(MockServer, MockGuard)> {
+        MOCK_ESCROW_SUBGRAPH
+        .get_or_try_init(|| async {
+            let mock_server = MockServer::start().await;
+            let mock_guard = mock_server


Are we getting much performance or speed by doing that?

The same applies as before: MockGuard needs to be dropped to be effective, it doesn't make sense to put one of those in static.

In general, I don't really like to use static things in tests, it's really good to isolate each test with its own data.

Maybe this is a case to use rstest again.

gusinacio · 2025-01-21T18:01:52Z

crates/tap-agent/src/agent/sender_account.rs

+        let rav_marked_as_last = sqlx::query!(
+            r#"
+                SELECT * FROM scalar_tap_ravs WHERE last = true;
+            "#,
+        )
+        .fetch_all(&pgpool)
+        .await
+        .expect("Should not fail to fetch from scalar_tap_ravs");
+        assert!(!rav_marked_as_last.is_empty());
+        // Stop the TAP aggregator server.


This seems to be testing things from SenderAllocation. It seems to me that this is an integration test.

gusinacio · 2025-01-21T18:10:27Z

crates/tap-agent/src/agent/sender_allocation.rs

@@ -1262,6 +1271,184 @@ pub mod tests {
        handle.stopped().await;
    }

+    #[sqlx::test(migrations = "../../migrations")]


You could do the following:

async fn execute<Fut>(pgpool: PgPool, populate: impl FnOnce(PgPool) -> Fut) where: Fut: Future<Output = ()> { .. populate(pgpool.clone()).await; .. } #[sqlx::test(migrations = "../../migrations")] async fn test_several_receipts_rav_request(pgpool: PgPool) { execute(pgpool, |pgpool| async { // Add receipts to the database. const AMOUNT_OF_RECEIPTS: u64 = 1000; for i in 0..AMOUNT_OF_RECEIPTS { let receipt = create_received_receipt(&ALLOCATION_ID_0, &SIGNER.0, i, i + 1, i.into()); store_receipt(&pgpool, receipt.signed_receipt()) .await .unwrap(); } } } #[sqlx::test(migrations = "../../migrations")] async fn test_several_receipts_batch_insert_rav_request(pgpool: PgPool) { execute(pgpool, |pgpool| async { // Add batch receipts to the database. const AMOUNT_OF_RECEIPTS: u64 = 1000; let mut receipts: Vec<ReceiptWithState<Checking>> = Vec::with_capacity(1000); for i in 0..AMOUNT_OF_RECEIPTS { let receipt = create_received_receipt(&ALLOCATION_ID_0, &SIGNER.0, i, i + 1, i.into()); receipts.push(receipt); } let res = store_batch_receipts(&pgpool, receipts).await; assert!(res.is_ok()); } }

You would be able to reduce this test in half, and also could create multiple ways to populate and execute the same test.

You also would need to add a few Send, Sync and 'static as trait bounds but the compiler should help you with that.

gusinacio · 2025-01-21T18:12:47Z

crates/tap-agent/src/agent/sender_allocation.rs

        pgpool: PgPool,
        sender_aggregator_endpoint: String,
        escrow_subgraph_endpoint: &str,
+        rav_request_receipt_limit: u64,
        sender_account: Option<ActorRef<SenderAccountMessage>>,
    ) -> SenderAllocationArgs {


This may be a little out of the scope, but one way to make this better is to add bon and annotate this function with #[bon::builder]. You are able to set default and only provide member that you actually need to change.

https://bon-rs.com/

Feel free to use it or not, it's your choice.

carlosvdr self-assigned this Jan 6, 2025

carlosvdr requested a review from gusinacio January 6, 2025 16:51

carlosvdr marked this pull request as ready for review January 13, 2025 19:39

carlosvdr changed the title ~~Extra tap tests~~ Enhance Test Coverage for Receipt Insertion, Batch Processing, and Sender Accounts” Jan 14, 2025

carlosvdr changed the title ~~Enhance Test Coverage for Receipt Insertion, Batch Processing, and Sender Accounts”~~ Enhance Test Coverage for Receipt Insertion, Batch Processing, and Sender Accounts Jan 14, 2025

suchapalaver requested changes Jan 14, 2025

View reviewed changes

carlosvdr force-pushed the 868b324jv-tap-tests branch from e3ef764 to 025024c Compare January 14, 2025 21:01

carlosvdr requested a review from suchapalaver January 14, 2025 21:01

suchapalaver previously approved these changes Jan 14, 2025

View reviewed changes

suchapalaver reviewed Jan 14, 2025

View reviewed changes

carlosvdr dismissed suchapalaver’s stale review via c59262a January 15, 2025 17:41

suchapalaver reviewed Jan 15, 2025

View reviewed changes

carlosvdr added 4 commits January 17, 2025 13:08

test: Enhance test coverage for tap-agent

896ebc1

test: used lazy static for the mock server

9c9b85d

test: added oncecell

014bf64

test: added oncecell to empty responses

7930988

carlosvdr force-pushed the 868b324jv-tap-tests branch from 8140179 to 7930988 Compare January 17, 2025 19:09

carlosvdr requested a review from suchapalaver January 17, 2025 19:09

style: cargo fmt

0697cbe

suchapalaver reviewed Jan 20, 2025

View reviewed changes

gusinacio requested changes Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance Test Coverage for Receipt Insertion, Batch Processing, and Sender Accounts #572

Enhance Test Coverage for Receipt Insertion, Batch Processing, and Sender Accounts #572

carlosvdr commented Jan 6, 2025 •

edited

Loading

coveralls commented Jan 6, 2025 •

edited

Loading

suchapalaver left a comment

suchapalaver commented Jan 14, 2025

suchapalaver Jan 14, 2025 •

edited

Loading

suchapalaver Jan 14, 2025

suchapalaver Jan 15, 2025

carlosvdr Jan 17, 2025

suchapalaver left a comment

gusinacio Jan 20, 2025

gusinacio Jan 21, 2025

gusinacio Jan 21, 2025

gusinacio Jan 21, 2025

gusinacio Jan 21, 2025

gusinacio Jan 21, 2025

carlosvdr Jan 22, 2025

gusinacio Jan 22, 2025

gusinacio Jan 21, 2025

gusinacio Jan 21, 2025

gusinacio Jan 21, 2025

carlosvdr Jan 22, 2025

gusinacio Jan 22, 2025

gusinacio Jan 21, 2025

gusinacio Jan 21, 2025

gusinacio Jan 21, 2025

gusinacio Jan 21, 2025

gusinacio Jan 21, 2025

gusinacio Jan 21, 2025

Enhance Test Coverage for Receipt Insertion, Batch Processing, and Sender Accounts #572

Are you sure you want to change the base?

Enhance Test Coverage for Receipt Insertion, Batch Processing, and Sender Accounts #572

Conversation

carlosvdr commented Jan 6, 2025 • edited Loading

coveralls commented Jan 6, 2025 • edited Loading

Pull Request Test Coverage Report for Build 12835420725

Details

💛 - Coveralls

suchapalaver left a comment

Choose a reason for hiding this comment

suchapalaver commented Jan 14, 2025

suchapalaver Jan 14, 2025 • edited Loading

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

suchapalaver 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

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

carlosvdr commented Jan 6, 2025 •

edited

Loading

coveralls commented Jan 6, 2025 •

edited

Loading

suchapalaver Jan 14, 2025 •

edited

Loading