Tests: Fix t.Parallel() errors in data/transactions/logic package

[jdtzmn]

Summary

This PR parallelizes existing tests within the logic package. Tests that could be parallelized and were missing the t.Parallel() demarcation have been fixed and tests that are unable to be parallelized were labelled accordingly.

This is the first PR of several that will reduce test times by parallelizing existing tests within the repository. These changes will be distributed across several smaller PRs so that benefits can be seen immediately, fewer changes need reviewing, and it is less likely that new tests are introduced while waiting for these PRs to be merged.

Test Plan

All existing logic package tests passed locally and the golangci-lint paralleltest linter showed no paralleltest errors within the logic package since the initial commit.

Confirming This Locally

First, remove the revision configuration:

...
-        new-from-rev: eb019291beed556ec6ac1ceb4a15114ce4df0c57~25
+        # new-from-rev: eb019291beed556ec6ac1ceb4a15114ce4df0c57~25
...

And remove the data package exclusion within the .golangci.yml:

...
-        - path: (agreement|catchup|cmd|config|crypto|daemon|data|gen|ledger|logging|netdeploy|network|node|protocol|rpcs|shared|stateproof|test|tools|util).*_test\.go
+        - path: (agreement|catchup|cmd|config|crypto|daemon|gen|ledger|logging|netdeploy|network|node|protocol|rpcs|shared|stateproof|test|tools|util).*_test\.go
...

I then ran the paralleltest linter with the following command. If nothing is output, then no errors were found.

$ make lint 2>/dev/null | grep "data/transactions/logic" | grep "paralleltest"

[codecov]

Codecov Report

Merging #4931 (2f0ea96) into master (3fbe53c) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4931      +/-   ##
==========================================
- Coverage   53.51%   53.51%   -0.01%     
==========================================
  Files         432      432              
  Lines       53615    53615              
==========================================
- Hits        28691    28690       -1     
- Misses      22695    22698       +3     
+ Partials     2229     2227       -2     

Impacted Files	Coverage Δ
catchup/peerSelector.go	98.95% <0.00%> (-1.05%)	⬇️
catchup/service.go	69.08% <0.00%> (-0.73%)	⬇️
network/wsNetwork.go	64.74% <0.00%> (-0.18%)	⬇️
data/transactions/verify/txn.go	74.57% <0.00%> (+0.84%)	⬆️
ledger/testing/randomAccounts.go	56.88% <0.00%> (+1.22%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[michaeldiamant]

@jdtzmn Thanks for the effort here - By making local changes, I confirmed data/transactions/logic no longer produces paralleletest linter errors. In addition to my visual inspection, I successfully re-ran the CI build 3 times to build confidence that the PR does not introduce non-determinism.

[algochoi]

Looking good - I'm also generally interested if there were any performance improvements with this change.

[michaeldiamant]

I'm also generally interested if there were any performance improvements with this change.

Happy to expand on the rationale live.

• From my testing before/after, I see ~9% reduction in test duration locally (go test -count=100 ./data/transactions/logic).
• I'm more interested in observing CI changes. However, it's more difficult to observe the CI impact because there's many variables at play (e.g. many other tests executed, random partitioning strategy).
  • Unfortunately, without changing the partitioning strategy and/or CI test workflow, I think we need to make more progress parallelizing unit tests to observe CI benefits.
  • If we find the opaqueness to be too problematic, I think we can explore modifying CI workflows to isolate benefits.