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.

Sign up for GitHub

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

Jump to bottom

tests(rpc): Add grpc test for GetTaddressBalanceStream and GetAddressUtxosStream #4407

Merged
merged 2 commits into from
May 19, 2022
Merged

tests(rpc): Add grpc test for GetTaddressBalanceStream and GetAddressUtxosStream #4407

merged 2 commits into from
May 19, 2022

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We skipped a few grpc tests that had streams involved. This PR add tests for GetTaddressBalanceStream and GetAddressUtxosStream.

Close #4362

Solution

Add simple grpc tests for GetTaddressBalanceStream and GetAddressUtxosStream.

Review

Anyone can review.

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 16, 2022 20:30
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team May 16, 2022 20:30
teor2345
teor2345 reviewed May 18, 2022
View reviewed changes
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.

This looks good, but it's not in CI yet (that's ticket #4168).

How did you test it?

@oxarbitrage
Copy link
Contributor Author

This was tested locally, similar to how all the other grpc tests were done. Ill try to make #4168

@teor2345
Copy link
Contributor

We're getting reports of some parsing failures in the gRPC tests, but we're not exactly sure why.

I think having them in CI would be good, but I don't want to block this PR on that.

teor2345
teor2345 approved these changes May 18, 2022
View reviewed changes
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.

Thanks, let's get this merged, and fix any bugs that come up in CI later.

@teor2345
Copy link
Contributor

This was tested locally, similar to how all the other grpc tests were done. Ill try to make #4168

@oxarbitrage you might want to talk to @gustavovalverde about CI tests, because we are currently working on cached state fixes. So #4168 might be blocked.

mergify bot added a commit that referenced this pull request May 18, 2022
@mergify
Merge of #4407
57656b5
@mergify mergify bot mentioned this pull request May 18, 2022
Closed
48 tasks
@teor2345 teor2345 added P-Medium ⚡ C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces lightwalletd any work associated with lightwalletd labels May 18, 2022
mergify bot added a commit that referenced this pull request May 19, 2022
@mergify
Merge of #4407
71d0ebf
@mergify mergify bot mentioned this pull request May 19, 2022
Closed
51 tasks
@mergify mergify bot merged commit d50cf8b into main May 19, 2022
@mergify mergify bot deleted the issue4362 branch May 19, 2022 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

@upbqdn upbqdn Awaiting requested review from upbqdn upbqdn is a code owner automatically assigned from ZcashFoundation/general-rust-reviewers

Assignees

@oxarbitrage oxarbitrage

Labels
A-rpc Area: Remote Procedure Call interfaces C-testing Category: These are tests lightwalletd any work associated with lightwalletd
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Address stream gRPC tests
2 participants
@oxarbitrage @teor2345