Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests: Fix t.Parallel() errors in cmd package #4991

Merged
merged 7 commits into from
Mar 7, 2023
Merged

Conversation

michaeldiamant
Copy link
Contributor

Enables https://github.com/kunwardeep/paralleltest on cmd by fixing linter warnings. Incrementally moves the ball towards greater unit test parallelization, which reduces local + CI test durations.

I vetted for flakiness by running:

go test -race -count 10 ./cmd/...

Notes:

  • The PR does not intend to rework all sequential tests into parallel tests. My objective is to enable already available parallelism throughout go-algorand. And then make a pass through remaining sequential tests to see if/where more parallelism exists.

@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Merging #4991 (0368212) into master (a688c23) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4991      +/-   ##
==========================================
- Coverage   53.44%   53.42%   -0.02%     
==========================================
  Files         431      431              
  Lines       54364    54364              
==========================================
- Hits        29056    29046      -10     
- Misses      23053    23064      +11     
+ Partials     2255     2254       -1     
Impacted Files Coverage Δ
ledger/blockqueue.go 82.25% <0.00%> (-2.69%) ⬇️
catchup/service.go 67.76% <0.00%> (-1.18%) ⬇️
network/wsNetwork.go 64.98% <0.00%> (-0.19%) ⬇️
ledger/tracker.go 75.10% <0.00%> (ø)
ledger/acctonline.go 79.16% <0.00%> (+0.52%) ⬆️

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

@michaeldiamant michaeldiamant marked this pull request as draft January 11, 2023 18:47
@michaeldiamant michaeldiamant marked this pull request as ready for review January 11, 2023 19:16
jdtzmn
jdtzmn previously approved these changes Jan 23, 2023
Copy link
Contributor

@jdtzmn jdtzmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good! I confirmed locally that the tests pass with the given test command. I found that some of the test files were not caught currently by golangci (see #5046 for more information) and added a commit to address those errors.

Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased master into this branch, and checked linter and for flakiness by running the race flag. Also added a commit (0368212) to disable linting on the newest set of cmd tests, which seem to share os variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants