Tests: 'Revert Tests Fix t.Parallel() errors in data package'#4995
Tests: 'Revert Tests Fix t.Parallel() errors in data package'#4995onetechnical merged 1 commit intomasterfrom
Conversation
This reverts commit c41cd69.
onetechnical
changed the title
t.Parallel() errors in data package"t.Parallel() errors in data package"
onetechnical
changed the title
t.Parallel() errors in data package"t.Parallel() errors in data package"
onetechnical
changed the title
t.Parallel() errors in data package"
onetechnical
changed the title
|
My comment is particularly for txn_test.go: I feel bad here because I spent a lot of time carefully removing the t.Parallel() calls from this file, and only I see them come back and removed again... |
Codecov Report
@@ Coverage Diff @@
## master #4995 +/- ##
=======================================
Coverage 53.63% 53.64%
=======================================
Files 432 432
Lines 54057 54057
=======================================
+ Hits 28996 28999 +3
+ Misses 22818 22810 -8
- Partials 2243 2248 +5
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| // TestStreamVerifierPostVBlocked tests the behavior when the return channel (result chan) of verified | ||
| // transactions is blocked, and checks droppedFromPool counter to confirm the drops | ||
| func TestStreamVerifierPostVBlocked(t *testing.T) { | ||
| partitiontest.PartitionTest(t) |
| } | ||
|
|
||
| func TestStreamVerifierMakeStreamVerifierErr(t *testing.T) { | ||
| partitiontest.PartitionTest(t) |
|
|
||
| // TestBlockWatcher runs multiple goroutines to check the concurency and correctness of the block watcher | ||
| func TestStreamVerifierBlockWatcher(t *testing.T) { | ||
| partitiontest.PartitionTest(t) |
| } | ||
|
|
||
| func TestMakeTxHandlerErrors(t *testing.T) { | ||
| partitiontest.PartitionTest(t) |
| // stops, starts in a loop, sends more transactions, and makes sure all the transactions | ||
| // are accounted for. It uses the production backlog worker | ||
| func TestTxHandlerRestartWithBacklogAndTxPool(t *testing.T) { //nolint:paralleltest // Not parallel because it mutates global metrics | ||
| partitiontest.PartitionTest(t) |
There was a problem hiding this comment.
Please don't remove this.
|
@algonautshant our intention is to straight revert this PR (fix the build) then to revisit with @jdtzmn I suggest you/him align on a pattern/annotation to apply to tests to indicate which ones we've decided from past work can NOT be parallelized to save folks in the future from the pain. Even better if we can point out the why, though that might be a bridge too far. |
| defer registryCloseTest(t, registry, dbfile) | ||
|
|
||
| access, err := db.MakeAccessor(t.Name()+"_stateprooftest", false, true) | ||
| access, err := db.MakeAccessor("stateprooftest", false, true) |
There was a problem hiding this comment.
Keeping this as is won't hurt. Keeping or reverting is fine here and other places for this.
|
|
||
| // TestJsonMarshal ensures that BoxRef names are b64 encoded, since they may not be characters. | ||
| func TestJsonMarshal(t *testing.T) { | ||
| partitiontest.PartitionTest(t) |
There was a problem hiding this comment.
Need to keep this please partitiontest.PartitionTest(t)
|
|
||
| // TestJsonUnmarshal ensures that BoxRef unmarshaling expects b64 names | ||
| func TestJsonUnmarshal(t *testing.T) { | ||
| partitiontest.PartitionTest(t) |
There was a problem hiding this comment.
Need to keep this please partitiontest.PartitionTest(t)
algonautshant
left a comment
algonautshant
left a comment
There was a problem hiding this comment.
Looks good.
Some comments. Must keep the partitiontest.PartitionTest(t) wherever removed (I commented on all).
Will be nice to keep the linter and explanations for the t.parallel removals (I commented on all).
| // encoded. These things could change without breaking the protocol, should stay | ||
| // the same for the sake of REST API compatibility. | ||
| func TestTxnJson(t *testing.T) { | ||
| partitiontest.PartitionTest(t) |
There was a problem hiding this comment.
Need to keep this please partitiontest.PartitionTest(t)
| } | ||
| } | ||
|
|
||
| func TestTxHandlerRememberReportErrorsWithTxPool(t *testing.T) { //nolint:paralleltest // Not parallel because it mutates global metrics |
There was a problem hiding this comment.
It's a good idea to keep the explanation here.
| a.NoError(err) | ||
| } | ||
|
|
||
| func TestKeyregValidityPeriod(t *testing.T) { //nolint:paralleltest // Not parallel because it modifies config.Consensus |
There was a problem hiding this comment.
It's a good idea to keep the explanation here.
| require.Equal(t, bd2.NextProtocolSwitchOn-bd2.NextProtocolVoteBefore, basics.Round(5)) | ||
| } | ||
|
|
||
| func TestBlockUnsupported(t *testing.T) { //nolint:paralleltest // Not parallel because it modifies config.Consensus |
There was a problem hiding this comment.
It's a good idea to keep the explanation here.
|
|
||
| const spProto = protocol.ConsensusVersion("test-state-proof-enabled") | ||
|
|
||
| func TestTxnValidationStateProof(t *testing.T) { //nolint:paralleltest // Not parallel because it modifies config.Consensus |
There was a problem hiding this comment.
It's a good idea to keep the explanation here.
| } | ||
|
|
||
| // TestTxHandlerIncomingTxHandleDrops accounts for the dropped txns when the verifier/exec pool is saturated | ||
| func TestTxHandlerIncomingTxHandleDrops(t *testing.T) { //nolint:paralleltest // Not parallel because it changes the backlog size |
There was a problem hiding this comment.
It's a good idea to keep the explanation here.
| } | ||
|
|
||
| // TestTxHandlerIncomingTxHandleDrops accounts for the dropped txns when the verifier/exec pool is saturated | ||
| func TestTxHandlerIncomingTxHandleDrops(t *testing.T) { //nolint:paralleltest // Not parallel because it changes the backlog size |
There was a problem hiding this comment.
It's a good idea to keep the explanation here.
| handler.Stop() // cancel the handler ctx | ||
| } | ||
|
|
||
| func TestTxHandlerPostProcessError(t *testing.T) { //nolint:paralleltest // Not parallel because it mutates global metrics |
There was a problem hiding this comment.
It's a good idea to keep the explanation here.
| require.Len(t, result, expected+1) | ||
| } | ||
|
|
||
| func TestTxHandlerPostProcessErrorWithVerify(t *testing.T) { //nolint:paralleltest // Not parallel because it mutates global metrics |
There was a problem hiding this comment.
It's a good idea to keep the explanation here.
| } | ||
|
|
||
| // TestTxHandlerRememberReportErrors checks Is and As statements work as expected | ||
| func TestTxHandlerRememberReportErrors(t *testing.T) { //nolint:paralleltest // Not parallel because incomingTxHandlerProcessing mutates global metrics |
There was a problem hiding this comment.
It's a good idea to keep the explanation here.
…4981)" (algorand#4995) This reverts commit c41cd69.
3 participants
Reverts #4981