Gatewayd integration test suite proposal #1702

okjodom · 2023-02-17T07:42:54Z

Proposes an integration test suite for gatewayd.

To start with, tests are only run with mocks of dependencies like ILnRpcClient and IFederationApi.

In it's full extent, this proposal would involve sharing the following from fedimint-testing crate:

fixtures for setting up fake bitcoin tests
fixtures for setting up real bitcoin tests
fixtures for setting up fake lightning tests
fixtures for setting up real lightning tests
fixtures for setting up fake fedimintd with modules
fixtures for setting up real fedimintd with modules

Gateway would then own it's own integration tests where it uses real or fake fixtures in validating it's functionality in serving one or more federations

towards #742

codecov-commenter · 2023-02-17T08:12:56Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.12 ⚠️

Comparison is base (0b89f34) 62.95% compared to head (08fc244) 62.84%.

📣 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             @@
##           master    #1702      +/-   ##
==========================================
- Coverage   62.95%   62.84%   -0.12%     
==========================================
  Files         137      137              
  Lines       26517    26518       +1     
==========================================
- Hits        16694    16664      -30     
- Misses       9823     9854      +31

Impacted Files	Coverage Δ
fedimint-core/src/config.rs	`67.30% <100.00%> (+0.10%)`	⬆️

... and 10 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.

jkitman · 2023-02-17T18:23:37Z

Why would the real tests and mock tests be separated if we can hide the implementation behind an interface?

I think similar to how we do in integrationtests it can be useful to run both with and without mocks depending on the level of testing required, using an env flag.

okjodom · 2023-02-17T21:29:21Z

Why would the real tests and mock tests be separated if we can hide the implementation behind an interface?

Spinning up a real federation with bitcoind and other integration artifacts might too heavy for the gateway test suite. However, we may want to:

have functional test spec that uses a mock of the federation and mocks of lightning
have an independent test suite for any real lightning extensions we build

integrationtest would have simpler tests for the gateway, but might ochestrate its real gateway with various implementations of lightning

okjodom · 2023-02-19T20:21:47Z

Why would the real tests and mock tests be separated if we can hide the implementation behind an interface?

Spinning up a real federation with bitcoind and other integration artifacts might too heavy for the gateway test suite.

Not true! After #1717 , I see how we can move all gateway integration tests to this framework. We'd just need to share he fixture for spinning up real bitcoind, lightning, and federations

mxz42

just some comments; I know it's WIP, but just wanted to know what's going on.
I probably need to have a closer look at the gateway API myself to get some context.

gateway/ln-gateway/tests/authentication_tests.rs

mxz42 · 2023-03-08T15:37:44Z

gateway/ln-gateway/tests/authentication_tests.rs

+        )
+        .await?
+        .status(),
+        401


is that a copy paste error? the returned status should not be 401, right?
edit: I see, it's a asser_ne. maybe change this to an assert_eq and expect the status code 200 that is returned when the call was successful (I assume).

assert_eq(200) tests for success on authentication, AND success in whatever authenticated api we are currently checking. Since we cannot be sure that our API succeeds after successful auth, we did not assert a 200.

Same discussion, months apart :) #1023 (comment)

gateway/ln-gateway/tests/integration_tests.rs

integrationtests/tests/tests.rs

mxz42 · 2023-03-08T16:40:18Z

gateway/ln-gateway/tests/integration_tests.rs

+            // assert cannot withdraw amount greater than the available balance
+            let outcome = parse_response::<TransactionId>(
+                rpc_ref
+                    .withdraw(
+                        gw_password.clone(),
+                        WithdrawPayload {
+                            federation_id: fid.clone(),
+                            amount: bitcoin::Amount::from_sat(amount_sats.clone() + 1),
+                            address: bitcoin.get_new_address().await,
+                        },
+                    )
+                    .await
+                    .map_err(|e| anyhow::anyhow!(e))
+                    .unwrap(),
+            )
+            .await
+            .unwrap();
+
+            assert_eq!(outcome.0, 200);
+            assert!(outcome.1.is_some());


hm, I would expect that we get some error or similar that the amount we want to withdrawal is too big?
Basically, these assert make sure that the call to withdraw was successful and then we ensure below by calling get_balance that nothing was withdrawn, right?

I'm not sure if we can design the API slightly different, so that the call to withdraw already lets us know that it's not possible to withdraw that amount (I'm missing a bit of context here. didn't dig into it).

edit: ok, I think I fundamentally do not understand something. We do a withdraw that is too large here and we get back a Some(TransactionId) in outcome.1. In line 367 we withdraw a permitted amount (everything that the gateway has) and we assert that outcome.1.is_none(), so no TransactionId?

Revisiting these proposals.
In order to give the best attention to each test case, I will first propose the test suite and then fill in the test cases one-by-one

Okay, so I deferred the test implementation themselves. Following up with those. With consideration for the review above

justinmoon · 2023-03-15T21:49:53Z

What is our strategy for merging this?

Perhaps it would be best to just move the existing tests to the new crate, and then add new tests in follow-up commits?

okjodom · 2023-03-16T12:17:47Z

Perhaps it would be best to just move the existing tests to the new crate, and then add new tests in follow-up commits?

How about the reverse strategy: add new non-overlapping tests so we have more coverage, and then move the existing tests over? IMO this is preferable because the existing tests don't exercise the public API surface of gatewayd. Moving them over will include a considerable rewrite.

justinmoon · 2023-03-16T20:54:33Z

How about the reverse strategy: add new non-overlapping tests so we have more coverage, and then move the existing tests over? IMO this is preferable because the existing tests don't exercise the public API surface of gatewayd. Moving them over will include a considerable rewrite.

Works for me!

jkitman

Looks good as a first step. I'd aim for an optional mock / real mode like with our existing tests.

Often you find bugs in real mode you wouldn't with mocks, but you don't want to run it while developing.

okjodom changed the title ~~Propose: Gatewayd functional test suite~~ Gatewayd functional test suite proposal Feb 17, 2023

okjodom assigned justinmoon Feb 17, 2023

okjodom force-pushed the gatewayd-tests branch from a4cced0 to 3d5984b Compare February 17, 2023 08:02

okjodom marked this pull request as ready for review February 17, 2023 08:04

okjodom assigned mxz42 Feb 17, 2023

okjodom mentioned this pull request Feb 17, 2023

Gateway tests reorg and cleanup #1703

Merged

okjodom requested a review from jkitman February 17, 2023 16:49

okjodom unassigned justinmoon and mxz42 Feb 17, 2023

okjodom requested review from mxz42 and justinmoon February 17, 2023 16:49

okjodom marked this pull request as draft February 18, 2023 04:02

okjodom force-pushed the gatewayd-tests branch from 182d3da to b8fbf6d Compare February 20, 2023 21:48

okjodom mentioned this pull request Feb 20, 2023

Use new gateway code in cli + rust tests #1717

Merged

okjodom changed the title ~~Gatewayd functional test suite proposal~~ Gatewayd integration test suite proposal Feb 20, 2023

okjodom force-pushed the gatewayd-tests branch 2 times, most recently from e98cd4a to 914e09e Compare February 27, 2023 22:41

mxz42 reviewed Mar 8, 2023

View reviewed changes

okjodom mentioned this pull request Mar 15, 2023

Implement filtering and details on gatewayd info API #1725

Closed

okjodom force-pushed the gatewayd-tests branch from 914e09e to 08fc244 Compare March 15, 2023 12:50

justinmoon mentioned this pull request Mar 15, 2023

Lnd Support #1907

Merged

okjodom force-pushed the gatewayd-tests branch from 08fc244 to ebe94cf Compare March 15, 2023 20:53

feat: propose new gatewayd integration test suite

889317f

okjodom added 4 commits March 17, 2023 23:14

feat: implement reusable test runner similar to integration tests

6800c35

feat: add rpc fixture for gateway fn tests

2c07b57

feat: random federation id dummy for testing

f818f54

docs: fix spelling and typos in test fixture description

12fcb6e

okjodom force-pushed the gatewayd-tests branch from ebe94cf to 12fcb6e Compare March 17, 2023 20:14

okjodom marked this pull request as ready for review March 17, 2023 20:16

jkitman approved these changes Mar 18, 2023

View reviewed changes

justinmoon approved these changes Mar 22, 2023

View reviewed changes

justinmoon merged commit 1901a9d into fedimint:master Mar 22, 2023

justinmoon mentioned this pull request May 3, 2023

Complete gatewayd test suite #2393

Closed

okjodom deleted the gatewayd-tests branch June 9, 2023 20:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gatewayd integration test suite proposal #1702

Gatewayd integration test suite proposal #1702

okjodom commented Feb 17, 2023 •

edited

codecov-commenter commented Feb 17, 2023 •

edited

jkitman commented Feb 17, 2023

okjodom commented Feb 17, 2023

okjodom commented Feb 19, 2023 •

edited

mxz42 left a comment

mxz42 Mar 8, 2023

okjodom Mar 16, 2023

okjodom Mar 16, 2023

mxz42 Mar 8, 2023

okjodom Mar 15, 2023

okjodom Mar 17, 2023

justinmoon commented Mar 15, 2023

okjodom commented Mar 16, 2023

justinmoon commented Mar 16, 2023

jkitman left a comment

Gatewayd integration test suite proposal #1702

Gatewayd integration test suite proposal #1702

Conversation

okjodom commented Feb 17, 2023 • edited

codecov-commenter commented Feb 17, 2023 • edited

Codecov Report

jkitman commented Feb 17, 2023

okjodom commented Feb 17, 2023

okjodom commented Feb 19, 2023 • edited

mxz42 left a comment

Choose a reason for hiding this comment

mxz42 Mar 8, 2023

Choose a reason for hiding this comment

okjodom Mar 16, 2023

Choose a reason for hiding this comment

okjodom Mar 16, 2023

Choose a reason for hiding this comment

mxz42 Mar 8, 2023

Choose a reason for hiding this comment

okjodom Mar 15, 2023

Choose a reason for hiding this comment

okjodom Mar 17, 2023

Choose a reason for hiding this comment

justinmoon commented Mar 15, 2023

okjodom commented Mar 16, 2023

justinmoon commented Mar 16, 2023

jkitman left a comment

Choose a reason for hiding this comment

okjodom commented Feb 17, 2023 •

edited

codecov-commenter commented Feb 17, 2023 •

edited

okjodom commented Feb 19, 2023 •

edited