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

flake: TestLedgerContinuesOnVotersCallbackFailure #5454

Merged
merged 8 commits into from
Jun 14, 2023

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Jun 6, 2023

Summary

Remove a flakey test deemed no longer needed along with a type that its removal orphaned.

Test Plan

CI

@tzaffi tzaffi added the Bug-Fix label Jun 6, 2023
@tzaffi tzaffi self-assigned this Jun 6, 2023
ledger/ledger_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #5454 (64920b4) into master (5ff0c22) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5454      +/-   ##
==========================================
- Coverage   55.60%   55.59%   -0.02%     
==========================================
  Files         447      447              
  Lines       63395    63395              
==========================================
- Hits        35253    35245       -8     
- Misses      25760    25766       +6     
- Partials     2382     2384       +2     

see 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ledger/ledger_test.go Outdated Show resolved Hide resolved
@cce cce requested a review from algorandskiy June 7, 2023 13:59
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

This is incorrect. The test asserts commit happened. Please skip it instead.

@tzaffi tzaffi marked this pull request as ready for review June 7, 2023 17:00
@tzaffi tzaffi requested review from algorandskiy and cce June 7, 2023 17:00
@algorandskiy
Copy link
Contributor

After digging into with Chris, the test is useless now after making OnPrepareVoterCommit not returning any errors, it is better to remove it

algorandskiy
algorandskiy previously approved these changes Jun 7, 2023
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Thank you Zeph for the change!

@tzaffi tzaffi requested review from cce and removed request for cce June 8, 2023 23:28
@@ -3091,32 +3091,6 @@ type errorCommitListener struct{}
func (l *errorCommitListener) OnPrepareVoterCommit(oldBase basics.Round, newBase basics.Round, _ ledgercore.LedgerForSPBuilder) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to remove the errorCommitListener type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cce
Copy link
Contributor

cce commented Jun 9, 2023

For more background, in #5085 the OnPrepareVoterCommit method was changed to not return an error, so as part of that change the errorCommitListener was updated (it used to return an error), but no longer does anything differently from the mockCommitListener in the test above.

@cce cce merged commit 7b69c1b into algorand:master Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants