tests: use new roundtrip test helper with more conversion function pairs #6486
Conversation
cce
force-pushed
the
roundtrip-tests
branch
2 times, most recently
from
56d2cec to
d6b0f83
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6486 +/- ##
==========================================
+ Coverage 47.53% 47.67% +0.14%
==========================================
Files 667 657 -10
Lines 88572 87921 -651
==========================================
- Hits 42102 41920 -182
+ Misses 43705 43223 -482
- Partials 2765 2778 +13 ☔ View full report in Codecov by Sentry. |
| // By default, tests the provided example plus 100 randomly generated values using protocol.RandomizeObject. | ||
| // Use WithRapid to provide a custom rapid.Generator for property-based testing. | ||
| // Use Opts to customize the number of random tests or pass RandomizeObjectOptions. | ||
| func Check[A any, B any](t *testing.T, a A, toB func(A) B, toA func(B) A, opts ...CheckOption) bool { |
There was a problem hiding this comment.
Where I've used RoundTrip elsewhere, I generate an exhaustive set of values that set every field to a non zero value, one at a time. Does that seem no good? It's very simple.
for _, nz := range basics_testing.NearZeros(t, basics.AssetParams{}) {
assert.True(t, basics_testing.RoundTrip(t, nz, assetToRD, rdToAsset), nz)
}
There was a problem hiding this comment.
Yes, I thought there was a helper for that somewhere that set each field to a non-zero field, one at a time somewhere in our codebase that we'd been using for tests but then realized there wasn't..
There was a problem hiding this comment.
The advantage of just doing one field at a time is that the test fault tells you exactly where the problem is.
There was a problem hiding this comment.
right I forgot nearzero was in the same package as roundtrip.. I reworked it now, wdyt, now whenever you use roundtrip.Check you get NearZero testing for all fields and plus 100 random-object test cases as a bonus
| c, err := AccountToAccountData(&conv) | ||
| require.NoError(t, err) | ||
| require.Equal(t, b, c) | ||
| require.True(t, roundtrip.Check(t, b, toModel, toBasics, roundtrip.NoRandomCases(), roundtrip.NoNearZeros())) |
There was a problem hiding this comment.
With all automatic testing off, isn't it just:
assert.Equal(t, b, toModel(toBasics(b))
?
| t.Errorf("Round-trip failed for provided example: %+v", a) | ||
| return false |
There was a problem hiding this comment.
Why Errorf and report passing as a boolean? I would think things that take a testing.T handle failure internally. There's no advantage to calling require.True around Check is there?
| if !cfg.skipNearZeros { | ||
| nearZeroValues := NearZeros(t, a) | ||
| for i, nzA := range nearZeroValues { | ||
| if !checkOne(t, nzA, toB, toA) { | ||
| t.Errorf("Round-trip failed for NearZero variant %d: %+v", i, nzA) | ||
| return false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd probably put NearZeros cases first, because when they fail and print, the field that's the problem is very, very obvious.
| // Test with RandomizeObject for additional coverage | ||
| var template A | ||
| for i := 0; i < randomCount; i++ { | ||
| randObj, err := protocol.RandomizeObject(&template, cfg.randomOpts...) |
There was a problem hiding this comment.
With generics, would it be worth making a type safe generator of random values, so you can just
var a A
for randA := range protocol.RandomObjects(a, randomObject) {
if !checkOne(t, randA, toB, toA) {
t.Errorf("Round-trip failed for random variant: %+v", randA)
return false
}
}
There was a problem hiding this comment.
Maybe RandomizeObject (and a hypothetical RandomObjects iterator) belong in this new roundtrip package, not protocol.
2 participants
Summary
In #6446 a new roundtrip test helper was added, for testing pairs of conversion functions work correctly. This moves it to its own package (to avoid circular imports with other data/basics/testing dependencies) and adds optional integration with
protocol.RandomizeObjectandrapid.Generatorto automatically exercise more test cases than the provided example.Test Plan
Existing tests should pass, new tests added for additional pairs of conversion functions I found in the codebase.