[GuruduGanesh]

…(#27423)

Description

Refs #27423.

Adds t.Parallel() to a small batch of utility unit tests where the tests are isolated and safe to run in parallel.

This updates tests under:

• util/collections
• util/io
• util/io/path
• util/rand
• util/security

TestGetPathIfExists is intentionally left sequential because its subtests share the same RandomizedTempPaths instance and depend on execution order. The "does not exist" case must run before the "does exist" case populates the path.

TestGetPaths_no_race is also intentionally left sequential because it is a race smoke test that starts goroutines using testing.T.

For timing, I compared the touched package set against upstream/master with go test ... -count=50. This small utility batch does not show a meaningful standalone speedup; timings were noise-level equivalent. This PR is intended as a small, low-risk slice of #27423.

Testing

go vet ./util/...
make lint-local
make TEST_MODULE="./util/collections ./util/io ./util/io/path ./util/rand ./util/security" test-local
make TEST_MODULE="./util/collections ./util/io ./util/io/path ./util/rand ./util/security" test-race-local
make build-local
go test ./util/collections ./util/io ./util/io/path ./util/rand ./util/security -shuffle=on -count=10

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Title of the PR
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[bunnyshell]

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

• 🚀 /bns:deploy to deploy the environment

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.98%. Comparing base (0dc5b3f) to head (0e8e968).
⚠️ Report is 661 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27771      +/-   ##
==========================================
- Coverage   63.98%   63.98%   -0.01%     
==========================================
  Files         421      421              
  Lines       57774    57774              
==========================================
- Hits        36968    36967       -1     
+ Misses      17327    17325       -2     
- Partials     3479     3482       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[blakepettersson]

@GuruduGanesh do we have any benchmarks showing if this is faster than baseline? You also need to set DCO for your commit

git commit --amend -s

[GuruduGanesh]

Thanks for the review.

I fixed the DCO issue by amending the commit with a matching sign-off:

Item	Value
Sign-off	Signed-off-by: Ganesh Gurudu <ganesh.gurudu@gmail.com>
Baseline commit	5ec06031b (upstream/master)
PR commit	6bcf4e229
Packages tested	./util/collections ./util/io ./util/io/path ./util/rand ./util/security

Test Runtime Comparison

Command	Baseline Mean	PR Mean	Difference	Notes
go test -p=1 <packages> -count=500	1.542s	1.410s	~8.6% faster	Serializes package execution, so it better isolates parallelism inside each package.
go test <packages> -count=500	1.319s	1.289s	~2.3% faster	Closer to default go test; smaller/noisier because packages already run concurrently.

Raw Stats From 9 Runs

Command	Version	Mean	Median	Stddev	Min	Max
go test -p=1 <packages> -count=500	Baseline	1.542s	1.523s	0.071s	1.469s	1.686s
go test -p=1 <packages> -count=500	PR	1.410s	1.401s	0.029s	1.384s	1.462s
go test <packages> -count=500	Baseline	1.319s	1.318s	0.069s	1.221s	1.408s
go test <packages> -count=500	PR	1.289s	1.245s	0.106s	1.216s	1.555s

This is a small utility-test slice, so I do not expect a dramatic standalone improvement. The data shows a modest improvement when package-level parallelism is controlled, and roughly noise-level improvement under default package parallelism. The main goal is to contribute a safe incremental batch toward #27423.

[ppapapetrou76]

LGTM

[nitishfy]

can you provide a justification on how these Parallel tests won't lead to race conditions when they're running parallely especially in CI?

[GuruduGanesh]

can you provide a justification on how these Parallel tests won't lead to race conditions when they're running parallely especially in CI?

@nitishfy - Thanks for raising this.

I limited t.Parallel() to tests that are isolated and do not mutate shared process/global state.

The repo's race test path (make test-race-local → ./hack/test.sh -race) is the right gate for this kind of change. I verified the touched package set with that locally, and any data race would surface as WARNING: DATA RACE and fail the run. The per-file reasoning is below.

Area	Why parallel execution is safe
util/collections/maps_test.go	Each table case builds its own input maps and only calls Merge(...). There is no shared mutable package state.
util/io/bytereadseeker_test.go	Each test creates its own ByteReadSeeker instance and local byte slice. No reader instance is shared across tests.
util/io/path/resolved_test.go	Tests use read-only path resolution and read-only testdata symlinks/files. They do not create, delete, chmod, or mutate filesystem state. The shared allowedRemoteProtocols value is read-only.
util/io/paths_test.go	The parallelized tests each create their own RandomizedTempPaths instance. Tests that share state are intentionally left sequential.
util/rand/rand_test.go	Tests call random string helpers and assert length/content properties only. They do not mutate shared seed/global state.
util/security/path_traversal_test.go	The test only calls EnforceToCurrentRoot(...) with local string inputs and checks returned values/errors.

I intentionally did not parallelize the tests that could be order- or lifecycle-sensitive:

Test	Reason
TestGetPathIfExists	Its subtests share the same RandomizedTempPaths instance and depend on order: "does not exist" must run before "does exist" populates the path.
TestGetPaths_no_race	It is a race smoke test that starts goroutines using testing.T; keeping it sequential avoids changing its lifecycle behavior.

I also verified the touched package set with race detection and repeated shuffled runs:

make TEST_MODULE="./util/collections ./util/io ./util/io/path ./util/rand ./util/security" test-race-local
go test ./util/collections ./util/io ./util/io/path ./util/rand ./util/security -shuffle=on -count=10

[blakepettersson]

LGTM, but for the next time I would love to see more impactful changes in terms of test runtime. Bring on some bigger changes! 😄

[blakepettersson]

@nitishfy mind removing your "request changes"?

[GuruduGanesh]

Hi @nitishfy, just following up on this. I’ve addressed your concern about race safety with per-file reasoning and verified the touched packages using race detection and shuffled repeated runs.

Since the PR now has approvals and auto-merge is enabled, could you please take another look and remove the “request changes” if there are no remaining concerns?

Thanks again for your review and guidance.