Use ids.GenerateTestID helper in tests
#3165
Conversation
dhrubabasu
added
testing
dhrubabasu
requested review from
abi87,
StephenButtolph and
danlaine
as code owners
| @@ -1262,7 +1262,7 @@ func TestStateSyncIsStoppedIfEnoughVotesAreCastedWithNoClearMajority(t *testing. | |||
| context.Background(), | |||
| voterID, | |||
| reqID, | |||
| set.Of(ids.ID{'u', 'n', 'k', 'n', 'o', 'w', 'n', 'I', 'D'}), | |||
| set.Of(ids.GenerateTestID()), | |||
There was a problem hiding this comment.
Was it ever useful in debugging to have these kinds of "labelled" ids? If so, you could add ids.GenerateLabelledTestID(tb testing.TB, lbl string) that then calls tb.Logf("test ID %q: %v", lbl, id) before returning the value.
The only problem I see with this would be introducing the testing package into prod code.
There was a problem hiding this comment.
This is completely optional and dependent on the answer to my opening question.
| @@ -142,20 +145,23 @@ func TestBanffProposalBlockJSON(t *testing.T) { | |||
| simpleBanffProposalBlockBytes, err := json.MarshalIndent(simpleBanffProposalBlock, "", "\t") | |||
| require.NoError(err) | |||
|
|
|||
| require.Equal(`{ | |||
| expectedSimpleBanffProposalBlockString := `{ | |||
There was a problem hiding this comment.
This would be clearer if it used fmt.Sprintf() instead of strings.Replace().
(optional, subjective) You could use explicit arg indices if it makes it more readable, but I'm not sure that it will.
There was a problem hiding this comment.
TIL about explicit arg indicies
| @@ -186,7 +192,7 @@ func TestBanffProposalBlockJSON(t *testing.T) { | |||
| complexBanffProposalBlockBytes, err := json.MarshalIndent(complexBanffProposalBlock, "", "\t") | |||
| require.NoError(err) | |||
|
|
|||
| require.Equal(`{ | |||
| expectedComplexBanffProposalBlockString := `{ | |||
| @@ -142,20 +145,23 @@ func TestBanffProposalBlockJSON(t *testing.T) { | |||
| simpleBanffProposalBlockBytes, err := json.MarshalIndent(simpleBanffProposalBlock, "", "\t") | |||
| require.NoError(err) | |||
|
|
|||
| require.Equal(`{ | |||
| expectedSimpleBanffProposalBlockString := `{ | |||
There was a problem hiding this comment.
TIL about explicit arg indicies
|
Converting to draft for now, there seems to be dependencies in the codebase already that rely on the sorted-ness of the IDs returned. Will investigate further. |
Why this should be merged
We hardcode random byte slices in a lot of the tests. Let's use the helper we have already instead :)
How this works
Replace hard-coded byte slices with
ids.GenerateTestID()How this was tested
CI