agreement: use specific error assertions in tests#6542
agreement: use specific error assertions in tests#6542cce wants to merge 4 commits intoalgorand:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #6542 +/- ##
===========================================
- Coverage 61.80% 47.75% -14.05%
===========================================
Files 484 645 +161
Lines 67520 87792 +20272
===========================================
+ Hits 41730 41925 +195
- Misses 22224 43117 +20893
+ Partials 3566 2750 -816
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Replace bare require.Error/assert.Error calls with more specific assertions (ErrorContains, ErrorIs) that verify the actual error message content. This improves test quality by ensuring tests fail for the right reasons.
cce
force-pushed
the
errorcontains-agreement
branch
from
e26b601 to
6358672
Compare
Convert ErrorContains to ErrorIs where the error message matches a known sentinel error variable.
…author likely meant to assert
Use require.ErrorAs with *proposalManager.errProposalManagerPVNotFound where runtime captures showed a typed error rather than a plain string.
jannotti
left a comment
jannotti
left a comment
There was a problem hiding this comment.
This one I'm much less able to figure out from context whether the string is correct. Willing to approve, just want to mention that I'm relying on others more for that aspect.
| var errProposalSeekerNotLess errProposalSeekerNotLess | ||
| assert.ErrorAs(t, err, &errProposalSeekerNotLess) |
There was a problem hiding this comment.
Are these wrapped, so you can't use ErrorIs?
| @@ -89,8 +89,9 @@ func TestBundleCreationWithZeroVotes(t *testing.T) { | |||
| var ub unauthenticatedBundle | |||
| makeBundlePanicWrapper(t, "makeBundle: no votes present in bundle (len(equivocationVotes) =", proposal, votes, nil) | |||
There was a problem hiding this comment.
Look at makeBundlePanicWrapper below - it does not use message, seems uncompleted - nullWriter vs string buf writer and message check
| var ub unauthenticatedBundle | ||
| makeBundlePanicWrapper(t, "makeBundle: no votes present in bundle (len(equivocationVotes) =", proposal, votes, nil) | ||
|
|
||
| ub.Step = step(s) |
There was a problem hiding this comment.
Without this change all errors are the same for all 5 tested steps that does not make much sense (s not used).
The change looks correct.
I also suggest to test the error it raises with ub.Step=0 as before the modification, outside of the for loop.
if b.Step == propose {
return termFmtErrorFn("unauthenticatedBundle.verify: b.Step = %v", propose)
}
| require.Error(t, err) | ||
| require.ErrorContains(t, err, `bundle: did not see enough votes: 0`) | ||
|
|
||
| bundles = append(bundles, bundle) |
There was a problem hiding this comment.
bundles are not used, the line below _ = bundles[0].u() is useless, it should probably assert that all bundles are empty
|
Btw, the test failure (race) is fixed in #6546 |
cce
force-pushed
the
errorcontains-agreement
branch
from
e882a3f to
ae3f3ab
Compare
3 participants
Summary
Replace bare
require.Error/assert.Errorcalls withErrorContainsin agreement tests. This ensures tests verify the actual error message, not just that some error occurred.Also adds a
forbidigolint rule to preventrequire.Error/assert.Errorinagreement/going forward. Split off from #6512 to make review easier.Test Plan
Existing tests should pass with the new, more precise error assertions.