chore: fix unit test data races (#3478)

[jgwest]

Fixes #3478

See parent issue for reproduction steps.

With this set of changes, go test -race (...) now passes:

go test -race `go list ./... | grep -v ./test/cmd/metrics-plugin-sample`

(I also used this shell script to run the tests over and over for several hours, to ensure there are not remaining intermittent failures).

Data races fixed

• Unsynchronized read/write to 'callbacks' within pkg/kubectl-argo-rollouts/viewcontroller/viewcontroller.go

  • Fix: Add mutexes where needed.

• Unsynchronized read/writes to 'rolloutUpdates' within pkg/kubectl-argo-rollouts/cmd/get/get_rollout.go:

  • Fix: Add mutexes where needed.

• Unsynchronized read/write to 'enqueuedObjects' in various test functions

  • Fix: Add mutexes where needed.

• Unsynchronized read/write to utils/time's Now global variable, which is used by unit tests to simulate time changes by replacing default time.Now()

  • Fix: Preserve the logic, but use a mutex to control read/write of the function var.

• Ambassador unit test's fake client was allowing unintended modification of the mock object in memory, via k8s client Get() function

  • Fix: DeepCopy the mock object in Get() before returning it to callers

• APISix unit tests cannot be parallelized, due to the use of 4 global test variables that are shared between test threads in rollout/trafficrouting/apisix/mocks/apisix.go:

var (
	ApisixRouteObj                   *unstructured.Unstructured
	SetHeaderApisixRouteObj          *unstructured.Unstructured
	DuplicateSetHeaderApisixRouteObj *unstructured.Unstructured
	ErrorApisixRouteObj              *unstructured.Unstructured
)

• Fix:

  • Remove parallelism from these tests

• APIsix unit tests cannot share a single, global reference to client *mocks.FakeClient, when parallelized

  • Fix: Remove global reference, and switch to test-local reference to FakeClient

• Traefik unit tests also cannot share a single, global reference to client *mocks.FakeClient, when parallelized

  • Fix: Remove global reference, and switch to test-local reference to FakeClient

• Traefik unit tests cannot be parallelized, due to the use of 2 global test variables that are shared between test threads in rollout/trafficrouting/traefik/mocks/traefik.go:

var (
	TraefikServiceObj      *unstructured.Unstructured
	ErrorTraefikServiceObj *unstructured.Unstructured
)

• Fix:

  • Remove parallelism from these tests

• Unsynchronized read/write to events in FakeEventRecorder

  • Fix: Add a mutex, plus convenience functions that use that mutex for read/write

• A small number of miscellaneous tests that were not using mutexes when read/writing between go routines

  • Fix: Add mutexes where needed.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
• The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
• I've signed my commits with DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My builds are green. Try syncing with master if they are not.
• My organization is added to USERS.md.

[github-actions]

Go Published Test Results

2 144 tests   2 144 ✅  2m 51s ⏱️
  119 suites      0 💤
    1 files        0 ❌

Results for commit c8cff22.

♻️ This comment has been updated with latest results.

[github-actions]

E2E Tests Published Test Results

  4 files    4 suites   3h 46m 4s ⏱️
110 tests 102 ✅  6 💤 2 ❌
448 runs  416 ✅ 24 💤 8 ❌

For more details on these failures, see this check.

Results for commit c8cff22.

♻️ This comment has been updated with latest results.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
8.3% Duplication on New Code

See analysis details on SonarCloud

[codecov]

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 82.78%. Comparing base (8405f2e) to head (c8cff22).
Report is 58 commits behind head on master.

Files	Patch %	Lines
utils/record/record.go	81.81%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3479      +/-   ##
==========================================
+ Coverage   81.83%   82.78%   +0.94%     
==========================================
  Files         135      135              
  Lines       20688    17025    -3663     
==========================================
- Hits        16931    14094    -2837     
+ Misses       2883     2041     -842     
- Partials      874      890      +16     

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