[DX-4492] Speed up executable Tests#22953
Conversation
CORA - Pending Reviewers
Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
…speedUpExecutable
executable Testsexecutable Tests
There was a problem hiding this comment.
Pull request overview
Risk Rating: HIGH
This PR targets a major reduction in runtime for core/capabilities/remote/executable tests by parallelizing tests and using testing/synctest (via a new internal wrapper), plus some small refactors/optimizations in related remote capability code.
Changes:
- Parallelized many tests with
t.Parallel()and refactored several time-based tests to usesynctest.Test/synctest.Wait. - Added
core/internal/testutils/synctestwrapper to disable synctest usage under-race(workaround for Go issue #76691). - Minor code cleanups/optimizations (e.g.,
min/max, timer reuse instead oftime.After, reduced allocations).
Areas requiring scrupulous human review:
- All production-code concurrency edits (notably the updated
wg.Go(...)usage in server/client/dispatcher/executor paths). synctestwrapper semantics underracevs non-racebuilds and any assumptions tests may now make about time progression.- Any behavior changes in message batching/hash construction (
trigger_publisher.go) and aggregation threshold logic (default_mode.go).
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/test/.agents/skills/fix-flaky-tests/SKILL.md | Updates agent skill flow and guidance text. |
| tools/test/.agents/skills/fix-flaky-tests/references/speed-up-tests.md | New guidance doc for speeding up slow tests. |
| core/internal/testutils/synctest/synctest.go | Adds non-race synctest wrapper implementation. |
| core/internal/testutils/synctest/synctest_race.go | Adds race-build no-bubble synctest wrapper implementation. |
| core/capabilities/remote/utils_test.go | Adds t.Parallel() to unit tests. |
| core/capabilities/remote/trigger_subscriber_test.go | Refactors expiry test using synctest; parallelizes many subtests; adds test helper. |
| core/capabilities/remote/trigger_registration_load_test.go | Parallelizes load-style tests; uses synctest; switches to t.Context(). |
| core/capabilities/remote/trigger_publisher.go | Small refactors: clearer error vars, min, allocation reduction in hash input. |
| core/capabilities/remote/trigger_publisher_test.go | Parallelizes tests; uses synctest for batching timing; refactors helper signature. |
| core/capabilities/remote/messagecache/message_cache_test.go | Adds t.Parallel() to tests. |
| core/capabilities/remote/executable/server.go | Concurrency refactor in server start loop goroutine management. |
| core/capabilities/remote/executable/server_test.go | Heavy test refactors: parallelization, synctest, atomic counters, response collection changes. |
| core/capabilities/remote/executable/request/server_request_test.go | Adds t.Parallel() and adjusts timing assertions. |
| core/capabilities/remote/executable/request/client_request.go | Replaces time.After with a reusable time.NewTimer. |
| core/capabilities/remote/executable/request/client_request_test.go | Parallelizes subtests; small performance tweaks; replaces an Eventually with bounded sleep loop. |
| core/capabilities/remote/executable/request/client_request_internal_test.go | Parallelizes attestation tests; modernizes loops. |
| core/capabilities/remote/executable/parallel_executor.go | Concurrency refactor in task execution goroutine management. |
| core/capabilities/remote/executable/parallel_executor_test.go | Parallelizes tests; switches to atomic.Int32 patterns and tighter timeouts. |
| core/capabilities/remote/executable/hasher_test.go | Parallelizes tests; minor slice allocation fixes. |
| core/capabilities/remote/executable/endtoend_test.go | Consolidates slow-execution tests; uses synctest harness; refactors helper APIs. |
| core/capabilities/remote/executable/client.go | Concurrency refactor in client background loops goroutine management. |
| core/capabilities/remote/executable/client_test.go | Parallelizes tests; refactors aggregation-grace coverage and fixes shared-state mutation. |
| core/capabilities/remote/dispatcher.go | Concurrency refactor in dispatcher receive loop goroutine management; small Send refactor. |
| core/capabilities/remote/dispatcher_test.go | Parallelizes tests and switches to t.Context(). |
| core/capabilities/remote/combined_client_test.go | Parallelizes tests and switches to t.Context(). |
| core/capabilities/remote/aggregation/signed_report_test.go | Adds //nolint:paralleltest notes for shared logger state. |
| core/capabilities/remote/aggregation/default_mode.go | Simplifies majority threshold computation via max. |
| core/capabilities/remote/aggregation/default_mode_test.go | Parallelizes tests; modernizes loops; adds gosec suppression note. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
tools/test/.agents/skills/fix-flaky-tests/SKILL.md:80
- The loop refers to
<diagnose-parallel-iterations>and a “Smoke” profile, but that section/profile no longer exist in this document (the table now starts at “Quick”). This makes the instructions internally inconsistent.
<loop>
1. If user doesn't have recent results, plan a run with `<diagnose-parallel-iterations>` (default: **Smoke** profile) then execute it. On sandbox errors, follow `<possible_execution_issues>`.
2. If no issues, escalate along `<diagnose-iterations>` (e.g. Smoke → Standard → Deep), increasing `--iterations` and keeping parallelism per `<diagnose-parallel-iterations>`. Ask the user before **Deep** if wall time will be large. If still clean and no fix was needed, end with findings; if a fix was applied, require at least **Standard** before FIXED.
3. If issues detected, focus on the ones the user wants to fix.
|




Speeds up
core/capabilities/remote/executabletests, the 3rd slowest test package incore/.Results
How
t.Parallel()Confirmed tests pass and aren't flaky:
Also includes various linting fixes and
t.Parallel()calls.Important Parts to Review
There are a lot of linting fixes and inconsequential changes here, so focus on the more important bits below.
time.Afterdoesn't get cleaned up until it actually fires, so we were leaking a bunch of these. Using a timer fixes this.synctesthas known issues when running with the-raceflag (should be fixed in Go 1.27). Using it in these tests seriously speeds them up and reduces flakes, but breaks the-raceruns. So we have a wrapper that turnssyncteston and off depending on if-raceis present. A bit kludgey, but functional.