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

[jdtzmn]

Summary

This PR parallelizes existing tests within the data 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 second PR of several that will reduce test times by parallelizing existing tests within the repository. My initial, basic benchmarking shows an average decrease in data package test times of at least 20%.

Test Plan

All existing data package tests passed locally and the golangci-lint paralleltest linter showed no paralleltest errors within the data 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
...

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

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

[codecov]

Codecov Report

Merging #4981 (1243316) into master (19fa6c2) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4981      +/-   ##
==========================================
- Coverage   53.65%   53.65%   -0.01%     
==========================================
  Files         432      432              
  Lines       54058    54058              
==========================================
- Hits        29007    29006       -1     
- Misses      22805    22809       +4     
+ Partials     2246     2243       -3     

Impacted Files	Coverage Δ
ledger/tracker.go	75.10% <0.00%> (-4.65%)	⬇️
catchup/service.go	69.80% <0.00%> (+0.48%)	⬆️
util/rateLimit.go	83.33% <0.00%> (+0.98%)	⬆️
catchup/peerSelector.go	100.00% <0.00%> (+1.04%)	⬆️
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 your effort here! I successfully re-ran the build to improve my confidence that the changes do not introduce flakiness.

Based on my local runs, the PR reduces go test -count 1 ./data by ~20% (38s vs 31s).

I think more work is needed to vet CI improvements. As an aside, https://app.circleci.com/insights/github/algorand/go-algorand/workflows/circleci_build_and_test/overview?branch=master shows a significant increase in build time since 2023-01-06 (35min vs 20min).

[michaeldiamant]

Merging - There's a large number of diffs making the PR susceptible to merge conflicts. We can address any further feedback in a follow on PR.

[michaeldiamant]

For future reviewer reference: A more robust local test is go test -race -count 1 ./data/....

The test I supplied in my earlier comment only tests ./data and not subpackages. Instead, ./data/... ought to be used.

[algorandskiy]

For future reviewer reference: A more robust local test is go test -race -count 1 ./data/....

Use count > 1 as well since a parallel test should be runnable in parallel with self.

[michaeldiamant]

My understanding is that go tests runs each count in isolation.

Nevertheless, my standard test is go test -race -count 10 ./data/.... At a minimum, I agree running with -count > 1 is important because it helps to isolate non-determinism.