test: lock in types.Header RLP encoding
#87
Conversation
ARR4N
requested review from
a team,
darioush,
qdm12 and
ceyonur
and removed request for
a team
| require.Equal(t, BloomByteLength, rng.Read(hdr.Bloom[:])) | ||
| require.Equal(t, len(BlockNonce{}), rng.Read(hdr.Nonce[:])) | ||
| require.Equal(t, numExtraBytes, rng.Read(hdr.Extra)) | ||
| t.Logf("%T:\n%+v", hdr, hdr) |
There was a problem hiding this comment.
I think we can remove this?
| t.Logf("%T:\n%+v", hdr, hdr) |
There was a problem hiding this comment.
This is to help make the input more readable in the case of a failure of RLP assertion later. Without it, there are just two huge hex blobs. See identifying the input as a specific instance of useful test failures.
| require.NoError(t, err, "hex.DecodeString()") | ||
|
|
||
| got, err := rlp.EncodeToBytes(hdr) | ||
| require.NoErrorf(t, err, "rlp.EncodeToBytes(%T)", hdr) | ||
| assert.Equalf(t, want, got, "rlp.EncodeToBytes(%T)", hdr) |
There was a problem hiding this comment.
nit I don't think there is a need for assert/require additional error messages, it will log out the line number in the test and the diffs between the two values if there is any by default, so adding a custom message feels redundant. Thoughts?
| require.NoError(t, err, "hex.DecodeString()") | |
| got, err := rlp.EncodeToBytes(hdr) | |
| require.NoErrorf(t, err, "rlp.EncodeToBytes(%T)", hdr) | |
| assert.Equalf(t, want, got, "rlp.EncodeToBytes(%T)", hdr) | |
| require.NoError(t, err) | |
| got, err := rlp.EncodeToBytes(hdr) | |
| require.NoErrorf(t, err) | |
| assert.Equalf(t, want, got) |
There was a problem hiding this comment.
As with identifying input, see identifying the function. Yes, the line number is there, but this makes error messages self-contained.
It may be overkill but it's a habit that dies hard and I think the even small pro outweighs the negligible con of including it.
| require.Equal(t, BloomByteLength, rng.Read(hdr.Bloom[:])) | ||
| require.Equal(t, len(BlockNonce{}), rng.Read(hdr.Nonce[:])) | ||
| require.Equal(t, numExtraBytes, rng.Read(hdr.Extra)) |
There was a problem hiding this comment.
nit it would be nice to populate bloom, nonce and extra before and separate from the assertions, it took me a solid minute to find where it was populated 😄
There was a problem hiding this comment.
Done. I've inlined them instead by creating the necessary methods on ethtest.PseudoRand.
| } | ||
| require.Equal(t, BloomByteLength, rng.Read(hdr.Bloom[:])) | ||
| require.Equal(t, len(BlockNonce{}), rng.Read(hdr.Nonce[:])) | ||
| require.Equal(t, numExtraBytes, rng.Read(hdr.Extra)) |
There was a problem hiding this comment.
nit That check is rather unneeded, since we test the test really, especially given numExtraBytes is defined in the test and we create the slice inline in this test.
There was a problem hiding this comment.
Now obsolete since inlining of the random generation.
Why this should be merged
We're making changes to very sensitive code that, if broken, can result in block hashes being different.
How this works
Change-detector test.
How this was tested
A randomly filled
types.Headerwith expected RLP locked as a constant in the test.