Improve testing around TestCandidateSelfVoteAfterWonElection #127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit improves the testing around cases where a (pre-)candidate wins an election without having voted for themselves. This was already tested by TestCandidateDeliversPreCandidateSelfVoteAfterBecomingCandidate, but this commit expands the testing to handle three related cases and to mirror the existing TestCandidateSelfVoteAfterLostElection test. Signed-off-by: Nathan VanBenschoten <[email protected]>
|
@pav-kv when you get a chance, want to give this testing PR a pass? |
pav-kv
approved these changes
Feb 1, 2024
| expState = StatePreCandidate | ||
| } | ||
| if sm.state != expState { | ||
| t.Errorf("state = %v, want %v", sm.state, expState) |
There was a problem hiding this comment.
Convert these to use require package while here?
Comment on lines
+1941
to
+1942
| // Its self-vote does not make its way to its ProgressTracker. | ||
| granted, _, _ := sm.trk.TallyVotes() |
There was a problem hiding this comment.
Why? Is this because the leader doesn't count votes in general (it doesn't need to already)? Or there is just special handling of leader self-vote?
Might be good to clarify the comment.
ahrtr
reviewed
Feb 2, 2024
Comment on lines
+1821
to
+1823
| if sm.state != expState { | ||
| t.Errorf("state = %v, want %v", sm.state, expState) | ||
| } |
There was a problem hiding this comment.
This minor comment also applies to all other places.
Suggested change
| if sm.state != expState { | |
| t.Errorf("state = %v, want %v", sm.state, expState) | |
| } | |
| require.Equal(t, expState, sm.state) |
| expState = StatePreCandidate | ||
| } | ||
| if sm.state != expState { | ||
| t.Errorf("state = %v, want %v", sm.state, expState) |
Comment on lines
+1904
to
+1908
| // Nothing new should be sent since n1 first learned that it was a | ||
| // candidate. | ||
| if steps3 := sm.takeMessagesAfterAppend(); len(steps3) != 0 { | ||
| t.Errorf("unexpected new messages after pre-vote self-vote: %+v", steps3) | ||
| } |
There was a problem hiding this comment.
The r.msgsAfterAppend has already been reset to nil when getting the steps2 (line 1888)
Suggested change
| // Nothing new should be sent since n1 first learned that it was a | |
| // candidate. | |
| if steps3 := sm.takeMessagesAfterAppend(); len(steps3) != 0 { | |
| t.Errorf("unexpected new messages after pre-vote self-vote: %+v", steps3) | |
| } |
| sm.stepOrSend(steps) | ||
| if sm.state != StateLeader { | ||
| t.Errorf("state = %v, want %v", sm.state, StateLeader) | ||
| } | ||
|
|
||
| // Its self-vote does not make its way to its ProgressTracker. |
There was a problem hiding this comment.
Suggested change
| // Its self-vote does not make its way to its ProgressTracker. | |
| // The delayed self-vote and pre-vote do not make its way to its ProgressTracker. |
|
Overall looks good to me. Just a couple of minor comments. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit improves the testing around cases where a (pre-)candidate wins an election without having voted for themselves. This was already tested by TestCandidateDeliversPreCandidateSelfVoteAfterBecomingCandidate, but this commit expands the testing to handle three related cases and to mirror the existing TestCandidateSelfVoteAfterLostElection test.
cc. @pav-kv