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

blockchain: Rework vote tests. #3075

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Mar 8, 2023

While working in related code, I noticed the existing tests related to voting could use some improvement since they are rather inconsistent with the rest of the code base and are otherwise rather difficult to follow given they use a bunch of magic numbers, reference inconsistent global definitions, and don't really explain what it is they're intending to test. I also noticed they have some off by ones such that they aren't actually testing the edge conditions as was likely intended.

This fully reworks the tests to address the aforementioned points and also takes the opportunity to make them more compact and improve their coverage in the process. They are also hopefully significantly easier to follow now as well both from the code structure itself and the new comments that describe what they actually intend to test.

Quantifying the improvement to compactness, this includes all of the aforementioned improvements and comes in just under 540 less lines of code. Since it adds 357 additional lines of comments, the overall absolute reduction in the diff size is 180 lines.

The following is an overview of the changes:

  • Convert tests to run all votes in the deployment in parallel
  • Create locally-scoped mock vote definitions for the deployment
  • Use named values for all vote bits extracted from the definitions
  • Calculate all critical values (edge conditions, intervals, etc) from the parameters so they will remain correct even if the params change
  • Combine and expand all tests related to parallel voting and quorum
  • Add additional tests for several edge conditions
  • Add additional test for attempting to reverse a vote with a different majority choice
  • Make every test check the states of all votes in the deployment for extra added assurance of correctness

@davecgh davecgh added the test coverage Discussion and pull requests for improving test coverage. label Mar 8, 2023
@davecgh davecgh added this to the 1.8.0 milestone Mar 8, 2023
@davecgh davecgh force-pushed the blockchain_rework_vote_tests branch 4 times, most recently from 18e7aff to a271c11 Compare March 8, 2023 17:08
While working in related code, I noticed the existing tests related to
voting could use some improvement since they are rather inconsistent
with the rest of the code base and are otherwise rather difficult to
follow given they use a bunch of magic numbers, reference inconsistent
global definitions, and don't really explain what it is they're
intending to test.  I also noticed they have some off by ones such that
they aren't actually testing the edge conditions as was likely intended.

This fully reworks the tests to address the aforementioned points and
also takes the opportunity to make them more compact and improve their
coverage in the process.  They are also hopefully significantly easier
to follow now as well both from the code structure itself and the new
comments that describe what they actually intend to test.

Quantifying the improvement to compactness, this includes all of the
aforementioned improvements and comes in just under 540 less lines of
code.  Since it adds 357 additional lines of comments, the overall
absolute reduction in the diff size is 180 lines.

The following is an overview of the changes:

- Convert tests to run all votes in the deployment in parallel
- Create locally-scoped mock vote definitions for the deployment
- Use named values for all vote bits extracted from the definitions
- Calculate all critical values (edge conditions, intervals, etc) from
  the parameters so they will remain correct even if the params change
- Combine and expand all tests related to parallel voting and quorum
- Add additional tests for several edge conditions
- Add additional test for attempting to reverse a vote with a different
  majority choice
- Make every test check the states of all votes in the deployment for
  extra added assurance of correctness
@davecgh davecgh force-pushed the blockchain_rework_vote_tests branch from a271c11 to 7f7247f Compare March 12, 2023 17:20
@davecgh davecgh merged commit 7f7247f into decred:master Mar 13, 2023
@davecgh davecgh deleted the blockchain_rework_vote_tests branch March 13, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test coverage Discussion and pull requests for improving test coverage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants