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

tests(rpc): Add wallet grpc tests #4253

Merged
merged 5 commits into from
May 4, 2022
Merged

tests(rpc): Add wallet grpc tests #4253

merged 5 commits into from
May 4, 2022

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We want to test all the possible calls a wallet connected to a lightwalletd server can possible do.

Close #3655

Solution

This PR add very basic tests that could be extended for a series of rpc methods a wallet connected to lightwalletd can possible call.

There are a few tests that are depending on merging additional code to main.

Review

I am looking some feedback in regards to the tests. Should we make more complicated stuff now or try to merge this code and open additional smaller issues to complete what we want ?

Any ideas to implement in this PR ?

I think @teor2345 and/or @conradoplg are good candidates to take a look.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@oxarbitrage oxarbitrage requested a review from a team as a code owner May 1, 2022 13:59
@oxarbitrage oxarbitrage requested review from dconnolly and removed request for a team May 1, 2022 13:59
@oxarbitrage
Copy link
Contributor Author

FYI here is how i am running this test locally:

export ZEBRA_CACHED_STATE_DIR="/media/oxarbitrage/4eb53770-4e4f-4d80-a830-0914f3f5b89a/chain/zebra"
export LIGHTWALLETD_DATA_DIR="/home/oxarbitrage/zebra/issue3655/lightwalletd"
export ZEBRA_TEST_LIGHTWALLETD="true"
export RUST_LOG="info"

cargo test -- --nocapture lightwalletd_wallet_grpc_tests

Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

I think this looks good so far, and makes sense to leave the simpler tests and add more complicated tests later.

I'm not sure about how we're handling the tests that require some state. I think some of them have #[ignore] so that they are only run explicitly (even if this runs only with ZEBRA_TEST_LIGHTWALLETD). I need to check the current state of the other tests, though.

I can resume taking a closer look at this PR tomorrow.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks like a great test, I just have a few suggestions for making it more reliable.

zebrad/tests/common/lightwalletd/wallet_grpc_test.rs Outdated Show resolved Hide resolved
zebrad/tests/common/lightwalletd/wallet_grpc_test.rs Outdated Show resolved Hide resolved
zebrad/tests/common/lightwalletd/wallet_grpc_test.rs Outdated Show resolved Hide resolved
zebrad/tests/common/lightwalletd/wallet_grpc_test.rs Outdated Show resolved Hide resolved
@oxarbitrage oxarbitrage mentioned this pull request May 3, 2022
3 tasks
@teor2345
Copy link
Contributor

teor2345 commented May 3, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented May 3, 2022

update

✅ Branch has been successfully updated

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

The code looks good, but we need the latest CI fixes to make the tests run reliably.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks like all the tests passed!

mergify bot added a commit that referenced this pull request May 4, 2022
@mergify mergify bot merged commit 25712f1 into main May 4, 2022
@mergify mergify bot deleted the wallet-grpc-tests branch May 4, 2022 04:00
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.

Integration tests for lightwalletd RPCs that are only used when a wallet connects
3 participants