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

fix(rpc): Use populated state in more RPC snapshot tests #6700

Merged
merged 10 commits into from
May 17, 2023

Conversation

teor2345
Copy link
Contributor

Motivation

We're getting lower test coverage in some of our snapshot tests, because they accidentally use a mock state. We get more coverage by actually calling the state.

Solution

  • Rename the mock state instances so we don't accidentally use them
  • For one test that failed without the mock state, add a snapshot of that failure

Review

This is a test cleanup, it isn't urgent at all.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@teor2345 teor2345 added C-cleanup Category: This is a cleanup P-Low ❄️ C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG C-tech-debt Category: Code maintainability issues labels May 17, 2023
@teor2345 teor2345 self-assigned this May 17, 2023
@teor2345 teor2345 requested a review from a team as a code owner May 17, 2023 00:05
@teor2345 teor2345 requested review from upbqdn and removed request for a team May 17, 2023 00:05
@teor2345 teor2345 force-pushed the populated-state-tests branch from e19d34a to 38607ce Compare May 17, 2023 00:09
@github-actions github-actions bot added the C-bug Category: This is a bug label May 17, 2023
@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #6700 (38607ce) into main (185d138) will increase coverage by 0.03%.
The diff coverage is 58.62%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6700      +/-   ##
==========================================
+ Coverage   78.01%   78.05%   +0.03%     
==========================================
  Files         310      310              
  Lines       40740    40757      +17     
==========================================
+ Hits        31784    31812      +28     
+ Misses       8956     8945      -11     

@teor2345
Copy link
Contributor Author

Failed due to #5819

Base automatically changed from fix-db-height-panic to main May 17, 2023 15:13
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.

The target branch changed when I was finishing the review on GitHub, which was interesting to observe because the code suddenly changed, and I just kept reading. :D

mergify bot added a commit that referenced this pull request May 17, 2023
@mergify mergify bot merged commit cb66670 into main May 17, 2023
@mergify mergify bot deleted the populated-state-tests branch May 17, 2023 18:50
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-bug Category: This is a bug C-cleanup Category: This is a cleanup C-tech-debt Category: Code maintainability issues C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants