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 snapshot tests for rpc methods responses #4352

Merged
merged 12 commits into from
May 16, 2022
Merged

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented May 7, 2022

Motivation

We want to lock the responses of the rpc methods so if anything change, tests will fail.

Close #4131

Solution

Add snapshots for each rpc method.

Review

@teor2345 introduced the snapshots and worked in most of them so it can be a good candidate, however anyone who had worked in this file before will find this tests easy to understand (with the addition of the snapshot).

Reviewer Checklist

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

Follow Up Work

@oxarbitrage oxarbitrage requested a review from a team as a code owner May 7, 2022 22:27
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team May 7, 2022 22:27
@teor2345 teor2345 self-requested a review May 8, 2022 22:32
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 really good!

Can we snapshot the serialized JSON format, and open tickets for the missing RPCs?

upbqdn
upbqdn previously approved these changes May 11, 2022
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

All looks good to me. I just noticed a few petty nits while going through the PR.

* Create the snapshot settings once

* Snapshot testnet state, RPCs, and transparent addresses

* Tweak header comment

* Ignore some duplicated dependencies we don't control
Co-authored-by: Marek <mail@marek.onl>
@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 16, 2022
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.

Let's merge this once CI passes

mergify bot added a commit that referenced this pull request May 16, 2022
@mergify mergify bot merged commit 128c9cb into main May 16, 2022
@mergify mergify bot deleted the issue4131 branch May 16, 2022 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces C-testing Category: These are tests lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add RPC response snapshot tests
3 participants