perf: Reduce test sleep times to prevent CI timeouts #1
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -92,7 +92,7 @@ teardown() { | |||||||||
|
|
||||||||||
| @test "future_status: returns running for active future" { | ||||||||||
| local fid | ||||||||||
| fid=$(future_run sleep 0.5) | ||||||||||
| local status | ||||||||||
| status=$(future_status "$fid") | ||||||||||
| [[ "$status" == "running" ]] | ||||||||||
|
|
@@ -119,7 +119,7 @@ teardown() { | |||||||||
|
|
||||||||||
| @test "future_status: returns cancelled for cancelled future" { | ||||||||||
| local fid | ||||||||||
| fid=$(future_run sleep 0.5) | ||||||||||
| future_cancel "$fid" | ||||||||||
| local status | ||||||||||
| status=$(future_status "$fid") | ||||||||||
|
|
@@ -167,7 +167,7 @@ teardown() { | |||||||||
|
|
||||||||||
| @test "future_await: returns 124 on timeout" { | ||||||||||
| local fid | ||||||||||
| fid=$(future_run sleep 0.8) | ||||||||||
|
There was a problem hiding this comment. Suggestion: The timeout test now uses Severity Level: Critical 🚨- ❌ Unit test "future_await" stops validating timeout behavior.
- ⚠️ CI test suite falsely passes timeout assumptions.
- ⚠️ Test regression affects AWAIT AND TIMEOUT category.
- ⚠️ Flaky CI reporting and reduced coverage for timeout paths.
Suggested change
Steps of Reproduction ✅1. Run the unit test suite including the test defined in tests/unit/futures.bats at the
AWAIT AND TIMEOUT section (file: tests/unit/futures.bats). The relevant test is
"future_await: returns 124 on timeout" whose code creates a future with fid=$(future_run
sleep 0.8) (line 170 in the PR hunk) and then calls run future_await "$fid" 1 (line 171).
2. The command run is future_run sleep 0.8 (tests/unit/futures.bats:170). That background
command will typically complete in ≈0.8s.
3. The test immediately calls future_await "$fid" 1 (tests/unit/futures.bats:171) — asking
future_await to wait at most 1 second. Since the background sleep finishes in ~0.8s (less
than 1s), future_await will return the child's exit status (success of sleep which is 0)
rather than timing out.
4. The test assertion on the next line expects [[ "$status" -eq 124 ]]
(tests/unit/futures.bats:172). Because future_await returned success instead of timing
out, the test will fail (status != 124). This reproduces deterministically on a normal
Linux/macOS runner without special timing manipulation.
Note: The reproduction is based on the final file state in tests/unit/futures.bats where
the lines creating the future and calling future_await are present as shown; no external
callers are required.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** tests/unit/futures.bats
**Line:** 170:170
**Comment:**
*Logic Error: The timeout test now uses `sleep 0.8` while calling `future_await` with a 1-second timeout, but since the underlying command completes in less than the timeout, `future_await` will observe the future as completed and return success instead of timing out with status 124, causing this test to fail and no longer actually validate the timeout path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment. P2: The timeout test no longer guarantees a timeout because the sleep duration (0.8s) is shorter than the 1s timeout, making the expected 124 status incorrect or flaky. Use a sleep greater than the timeout to ensure the timeout path is exercised. Prompt for AI agents
Suggested change
|
||||||||||
| run future_await "$fid" 1 | ||||||||||
| [[ "$status" -eq 124 ]] | ||||||||||
| future_cancel "$fid" | ||||||||||
|
|
@@ -238,7 +238,7 @@ teardown() { | |||||||||
|
|
||||||||||
| @test "future_cancel: cancels running future" { | ||||||||||
| local fid | ||||||||||
| fid=$(future_run sleep 0.5) | ||||||||||
| future_cancel "$fid" | ||||||||||
| local status | ||||||||||
| status=$(future_status "$fid") | ||||||||||
|
|
@@ -247,7 +247,7 @@ teardown() { | |||||||||
|
|
||||||||||
| @test "future_cancel: returns 0 on success" { | ||||||||||
| local fid | ||||||||||
| fid=$(future_run sleep 0.5) | ||||||||||
| future_cancel "$fid" | ||||||||||
| [[ $? -eq 0 ]] | ||||||||||
| } | ||||||||||
|
|
@@ -262,7 +262,7 @@ teardown() { | |||||||||
|
|
||||||||||
| @test "future_cancel: kills the background process" { | ||||||||||
| local fid | ||||||||||
| fid=$(future_run sleep 0.5) | ||||||||||
| local dir="${TMPDIR:-/tmp}/mainframe/futures/$fid" | ||||||||||
| local pid | ||||||||||
| pid=$(<"$dir/pid") | ||||||||||
|
|
@@ -324,7 +324,7 @@ teardown() { | |||||||||
|
|
||||||||||
| @test "future_cleanup: preserves running futures" { | ||||||||||
| local fid | ||||||||||
| fid=$(future_run sleep 0.5) | ||||||||||
| local dir="${TMPDIR:-/tmp}/mainframe/futures/$fid" | ||||||||||
|
|
||||||||||
| future_cleanup | ||||||||||
|
|
@@ -335,7 +335,7 @@ teardown() { | |||||||||
|
|
||||||||||
| @test "future_cleanup --all: removes cancelled futures" { | ||||||||||
| local fid | ||||||||||
| fid=$(future_run sleep 0.5) | ||||||||||
| future_cancel "$fid" | ||||||||||
| local dir="${TMPDIR:-/tmp}/mainframe/futures/$fid" | ||||||||||
|
|
||||||||||
|
|
@@ -382,7 +382,7 @@ teardown() { | |||||||||
|
|
||||||||||
| @test "future_wait_any: returns first completed ID" { | ||||||||||
| local fid1 fid2 | ||||||||||
| fid1=$(future_run sleep 0.5) | ||||||||||
| fid2=$(future_run echo "quick") | ||||||||||
|
|
||||||||||
| local first | ||||||||||
|
|
@@ -394,7 +394,7 @@ teardown() { | |||||||||
|
|
||||||||||
| @test "future_wait_any: returns 0 for successful first completion" { | ||||||||||
| local fid1 fid2 | ||||||||||
| fid1=$(future_run sleep 0.5) | ||||||||||
| fid2=$(future_run true) | ||||||||||
|
|
||||||||||
| future_wait_any "$fid1" "$fid2" >/dev/null | ||||||||||
|
|
@@ -405,7 +405,7 @@ teardown() { | |||||||||
|
|
||||||||||
| @test "future_wait_any: returns 1 for failed first completion" { | ||||||||||
| local fid1 fid2 | ||||||||||
| fid1=$(future_run sleep 0.5) | ||||||||||
| fid2=$(future_run false) | ||||||||||
|
|
||||||||||
| run bash -c "source '$MAINFRAME_ROOT/lib/common.sh'; source '$MAINFRAME_ROOT/lib/futures.sh'; future_wait_any '$fid1' '$fid2'" | ||||||||||
|
|
@@ -542,7 +542,7 @@ teardown() { | |||||||||
| @test "future: handles long-running commands" { | ||||||||||
| skip "Skipping long-running test in unit suite" | ||||||||||
| local fid | ||||||||||
| fid=$(future_run sleep 0.8) | ||||||||||
| future_await "$fid" 10 | ||||||||||
| [[ $? -eq 0 ]] | ||||||||||
| } | ||||||||||
|
|
||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -240,7 +240,7 @@ teardown() { | |||||||||
| } | ||||||||||
|
|
||||||||||
| @test "with_timeout: times out slow command" { | ||||||||||
| run with_timeout 1 sleep 2 | ||||||||||
| [[ $status -eq 124 ]] | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -392,7 +392,7 @@ teardown() { | |||||||||
| circuit_breaker_call "test_svc" false || true | ||||||||||
|
|
||||||||||
| # Wait for timeout | ||||||||||
| sleep 0.5 | ||||||||||
|
There was a problem hiding this comment. Suggestion: The circuit breaker implementation only reports a transition from open to half-open after the elapsed time is greater than or equal to the configured timeout; with Severity Level: Critical 🚨- ❌ tests/unit/retry.bats "open→half-open" assertion fails.
- ⚠️ CI test suite shows flakiness / failing test runs.
- ⚠️ Blocks merges until test stability restored.
Suggested change
Steps of Reproduction ✅1. Open tests/unit/retry.bats and locate the "open transitions to half-open after timeout"
test (lines ~389-400). The test calls:
- circuit_breaker_init "test_svc" --threshold 2 --timeout 1 (lines 390-391)
- two failing calls: circuit_breaker_call "test_svc" false || true (lines 391-392)
- then sleeps for 0.5s (line 395) before checking state via
state=$(circuit_breaker_state "test_svc") (lines 394-399).
2. Inspect circuit breaker implementation: Grep shows circuit_breaker_state() and
circuit_breaker_call() implementations in lib/retry.sh (usage comments at lines ~567-569)
and additional implementation in lib/resilience.sh (matches at lines reported by Grep).
The code comments and implementation compute elapsed >= timeout to transition to half-open
(see comment in tests and log messages referenced by Grep: "_retry_log debug
\"...transitioning to half-open\"").
3. Real execution: with --timeout 1 the implementation requires at least ~1 second elapsed
since the circuit opened for circuit_breaker_state() to report "half-open". The test only
sleeps 0.5s at line 395, so calling circuit_breaker_state at line 397 will still return
"open" (not "half-open") causing the assertion [[ "$state" == "half-open" ]] to fail.
4. Reproduce locally: run the single test (for example via bats tests/unit/retry.bats with
a -t to run that test) and observe failure: after the two failing calls the state check
will remain "open" because only 0.5s elapsed; increasing sleep to a value >1s (e.g., 1.2s)
makes the elapsed >= timeout and the state becomes "half-open".
Note: I verified test locations and function definitions via Grep results pointing to
lib/retry.sh and lib/resilience.sh; the test uses --timeout 1, so 0.5s is concretely
insufficient per the implementation's >= timeout check.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** tests/unit/retry.bats
**Line:** 395:395
**Comment:**
*Logic Error: The circuit breaker implementation only reports a transition from open to half-open after the elapsed time is greater than or equal to the configured timeout; with `--timeout 1`, sleeping for only 0.5 seconds is insufficient, so `circuit_breaker_state` will still report `open` and this test will fail its expectation of `half-open`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment. P2: Sleep is shorter than the circuit breaker timeout, so the state may still be "open" instead of transitioning to "half-open". This can make the test flaky on slower CI machines. Prompt for AI agents
Suggested change
|
||||||||||
|
|
||||||||||
| local state | ||||||||||
| state=$(circuit_breaker_state "test_svc") | ||||||||||
|
|
@@ -405,7 +405,7 @@ teardown() { | |||||||||
| circuit_breaker_call "test_svc" false || true | ||||||||||
|
|
||||||||||
| # Wait for timeout to transition to half-open | ||||||||||
| sleep 0.5 | ||||||||||
|
There was a problem hiding this comment. Suggestion: For the half-open success test, the circuit breaker must have been open for at least the configured timeout before the next Severity Level: Critical 🚨- ❌ tests/unit/retry.bats "half-open closes" assertion fails.
- ⚠️ CI run reports failing retry tests.
- ⚠️ Increases developer friction due to test instability.
Suggested change
Steps of Reproduction ✅1. Open tests/unit/retry.bats and find the "half-open success closes circuit" test (lines
~402-416). The test sets:
- circuit_breaker_init "test_svc" --threshold 2 --timeout 1 --half-open-max 1 (line
403)
- two failing calls to trip the breaker (lines 404-405)
- then sleeps for 0.5s at line 408 before invoking a success call: circuit_breaker_call
"test_svc" true (lines 407-411).
2. Based on the implementation (circuit_breaker_state / circuit_breaker_call in
lib/retry.sh and lib/resilience.sh as discovered by Grep), the circuit only enters
half-open when elapsed >= timeout (timeout set to 1s). The test's 0.5s sleep is therefore
too short for the call at line 411 to be treated under half-open semantics; the call will
be rejected as open instead of being executed to close the circuit.
3. Reproduce concretely: run only this test (bats -t or similar) — after the two failures
the immediate success call will be rejected (exit status indicating open), so the state
check at lines 413-415 expecting "closed" will fail. Increasing the sleep to >1s (e.g.,
1.2s) ensures the call happens in half-open and can close the circuit.
4. Evidence: Grep results show logging paths and comments in lib/retry.sh referencing
transitions to half-open and rejecting calls when OPEN (lines around the
circuit_breaker_call implementation). The test's use of --timeout 1 is explicit in the
test file; therefore 0.5s is concrete insufficient waiting time per the real code.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** tests/unit/retry.bats
**Line:** 408:408
**Comment:**
*Logic Error: For the half-open success test, the circuit breaker must have been open for at least the configured timeout before the next `circuit_breaker_call` is treated as half-open; with `--timeout 1` and only `sleep 0.5`, the third call is still rejected as open rather than executed in half-open state, so the test expecting the circuit to close will fail.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||
|
|
||||||||||
| # Success in half-open should close | ||||||||||
| circuit_breaker_call "test_svc" true | ||||||||||
|
|
@@ -527,7 +527,7 @@ teardown() { | |||||||||
| local marker="${TEST_TEMP_DIR}/ready" | ||||||||||
|
|
||||||||||
| # Create the marker after 2 seconds | ||||||||||
| (sleep 0.5 && touch "$marker") & | ||||||||||
|
|
||||||||||
| run wait_for --timeout 5 --interval 1 test -f "$marker" | ||||||||||
| assert_success | ||||||||||
|
|
@@ -548,7 +548,7 @@ teardown() { | |||||||||
| @test "wait_for_file: waits for file creation" { | ||||||||||
| local file="${TEST_TEMP_DIR}/delayed.txt" | ||||||||||
|
|
||||||||||
| (sleep 0.5 && touch "$file") & | ||||||||||
|
|
||||||||||
| run wait_for_file "$file" 5 | ||||||||||
| assert_success | ||||||||||
|
|
@@ -699,7 +699,7 @@ teardown() { | |||||||||
| [[ $status -eq 1 ]] | ||||||||||
|
|
||||||||||
| # Wait well beyond refill period (1s period + margin) | ||||||||||
| sleep 1.5 | ||||||||||
|
|
||||||||||
| # Should have tokens again | ||||||||||
| run rate_limit_acquire "test_rl" --no-wait | ||||||||||
|
|
||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -118,7 +118,7 @@ teardown() { | |||||
|
|
||||||
| # Submit a long-running job | ||||||
| local job_id | ||||||
| job_id=$(pool_submit "test_pool" sleep 0.8) | ||||||
|
|
||||||
| # Give it time to start | ||||||
| sleep 0.2 | ||||||
|
|
@@ -423,7 +423,7 @@ EOF | |||||
| pool_create "test_pool" | ||||||
|
|
||||||
| local job_id | ||||||
| job_id=$(pool_submit "test_pool" sleep 0.5) | ||||||
|
There was a problem hiding this comment. Suggestion: The reduced sleep duration in this test makes it likely that the background job will occasionally complete before Severity Level: Major
|
||||||
| job_id=$(pool_submit "test_pool" sleep 0.5) | |
| job_id=$(pool_submit "test_pool" sleep 0.8) |
Steps of Reproduction ✅
1. In the PR final file, open tests/unit/workpool.bats and locate the pool-related tests
around line 426 (file: tests/unit/workpool.bats). The test submits a job with:
`job_id=$(pool_submit "test_pool" sleep 0.5)` (line 426).
2. The test then sleeps 0.2s to "Let it start" and immediately calls `run pool_result
"test_pool" "$job_id"` (see the "pool_result: fails for running job" test earlier in the
file where the same pattern is used; the immediate check path is pool_result ->
lib/workpool.sh functions invoked by tests).
3. On a fast CI runner or low-load local machine the background process running `sleep
0.5` may complete within the 0.2s + scheduling window before `pool_result` is executed;
when that happens pool_result returns success (job done) and the test expecting failure
(`[[ "$status" -eq 1 ]]`) fails.
4. Reproduce locally by running only this test: BATS filter to that test in the repo
(e.g., bats tests/unit/workpool.bats -f "pool_result: fails for running job") on a fast
host — observe intermittent pass/fail depending on timing. The relevant entry/pipeline is
the test file itself and the pool_result call path that queries job state in
$POOL_STATE_DIR/test_pool/jobs/$job_id.
5. The suggested change increases the job runtime to 0.8s so the window between "sleep
0.2" and `pool_result` is less likely to see the job finish — verifying the flakiness
mitigation by repeating the single test run multiple times and observing removal of
intermittent failures.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit/workpool.bats
**Line:** 426:426
**Comment:**
*Logic Error: The reduced sleep duration in this test makes it likely that the background job will occasionally complete before `pool_result` is called, causing the assertion that it fails for a running job to intermittently fail and introducing test flakiness instead of reliably verifying behavior for an in-progress job.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The TTL test now sleeps for only 0.5 seconds after setting a 1-second TTL, but the underlying file backend uses second-resolution timestamps and only considers a key expired when the current time is strictly greater than the stored expiry, so this wait is shorter than the effective TTL and will cause the test to intermittently see the key still present instead of expired. Increasing the sleep to be safely longer than the TTL (accounting for timestamp granularity and scheduling jitter) restores deterministic behavior while still reducing test runtime compared to the original 2-second sleep. [logic error]
Severity Level: Major⚠️
Steps of Reproduction ✅
Prompt for AI Agent 🤖