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

Integration tests (and a little more) #75

Merged
merged 10 commits into from
May 20, 2022
Merged

Conversation

johncantrell97
Copy link
Contributor

This PR introduces a small refactor, a major bugfix, and a way to write integration tests for Sensei with a comprehensive example.

  • Small refactor is a cleanup where I removed the RequestContext object and just replaced it with the AdminService object. The RequestContext was redundant when really the http/grpc services just need access to the AdminService. I guess in theory in the future they might need access to other objects that don't belong as part of the AdminService but for now that's not the case and this is cleaner.

  • The bugfix is a bit complicated but it boils down to there being cases where Sensei can enter a deadlock because of the way tokio works. The fix ( for now at least) is to create a second tokio runtime that is used for performing synchronous database operations (used mostly by ldk persistence layer).

  • Most importantly I utilized the bitcoind crate to enable integration tests in sensei without needing to have any environment setup. Works fine with just cargo test and allows us to write tests that look like this:

async fn smoke_test(bitcoind: BitcoinD, admin_service: AdminService) {
        let alice = create_root_node(&admin_service, "alice", "alice", true).await;
        let bob = create_node(&admin_service, "bob", "bob", true).await;
        let charlie = create_node(&admin_service, "charlie", "charlie", true).await;

        fund_node(&bitcoind, alice.clone()).await;
        fund_node(&bitcoind, bob.clone()).await;

        open_channel(&bitcoind, alice.clone(), bob.clone(), 1_000_000).await;
        open_channel(&bitcoind, bob.clone(), charlie.clone(), 1_000_000).await;

        // create 50 invoices to pay charlie 10sats
        let invoices = batch_create_invoices(charlie.clone(), 10, 50).await;

        // have alice pay all 50 concurrently by routing through bob
        future::try_join_all(
            invoices
                .into_iter()
                .map(|invoice| pay_invoice(alice.clone(), invoice))
                .map(tokio::spawn),
        )
        .await
        .unwrap();

There's still some work to be done to enable polling / wait_until style code in the tests. Currently it's just waiting some amount of time for funding / channel opens / etc.

I plan on add that functionality to this PR hopefully later today.

@johncantrell97
Copy link
Contributor Author

Note: This tests sensei by directly invoking the AdminService and NodeService.

I think we still want e2e tests that can exercise the grpc and http endpoints. devrandom has a python script that uses the grpc api and I'm thinking of writing a quick node script that uses the http api.

I also want to get cypress tests running against the web-admin.

I think once all 4 of these are in place the project will be in a much better place from a testing standpoint going forward.

use crate::services::PaginationRequest;
use bitcoin::{Address, Amount, Network};
use bitcoincore_rpc::RpcApi;
use bitcoind::BitcoinD;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a system test that has external dependencies, it should probably not try to masquerade as a unit test. Those should run quickly and not have external dependencies.

It could be inserted as an integration test, but even that gets run by cargo test, which again, should not have external dependencies.

I would have system tests be organized a separate binary in the Cargo.toml file and/or as a separate crate.

To do this, you might need to make the relevant parts of the Rust API pub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. I'm not saying it makes it 'correct' but this is also how bdk writes their tests.

It's not exactly an external dependency. The whole point of the bitcoind crate is to avoid you having to setup an externally running bitcoind for your tests.

Given that we have zero unit tests it's probably fine to leave this for now but would make sense to do once we have them?

Copy link
Contributor

@devrandom devrandom May 18, 2022

Choose a reason for hiding this comment

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

It's my impression that it still requires installing the bitcoind binary and starts a separate process. Also, it sounds like cargo check --all-targets --profile=test would fail for projects where bdk is a dependency unless bitcoind is in the path, which is not what one would expect. (Looks like the bitcoind crate auto-downloads bitcoind, so this particular concern is not relevant.)

In any case, I don't have a strong opinion about the path there - just a general comment.

@johncantrell97 johncantrell97 force-pushed the integration-test branch 6 times, most recently from 88101ad to 3588bf2 Compare May 20, 2022 00:23
@johncantrell97
Copy link
Contributor Author

Ok this now also includes a bit of a refactor that extracts a "sensei core" lib as described here: #46

It also moves the integration test to the proper place in that library instead of a unit test.

@johncantrell97 johncantrell97 merged commit c70f4d9 into main May 20, 2022
@johncantrell97 johncantrell97 deleted the integration-test branch May 20, 2022 20:18
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.

2 participants