Skip to content

Throttle worker retries in tests #49

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.

Sign up for GitHub

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

Merged
leynos merged 2 commits into from
Aug 4, 2025
Merged

Throttle worker retries in tests #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2fdcdc1
Throttle worker retries in tests
leynos Aug 4, 2025
2792c5f
Add simple test to ci
leynos Aug 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ jobs:
run: make check-fmt
- name: Lint
run: make lint
- name: Test Only
run: make test
Comment on lines +25 to +26
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 4, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Eliminate or justify the duplicated test run

make test is now executed in this “Test Only” step and (implicitly) again inside the “Test and Measure Coverage” step. Running the full suite twice:

• adds several minutes to CI wall-clock time,
• recompiles the crate graph twice (different rustc flags → cache miss),
• burns extra GitHub Actions minutes.

If the intention is a fail-fast guard, either (a) drop test execution from the coverage action or (b) gate this step behind a condition (e.g. only on PRs) to avoid the double run on main. Provide a comment explaining the rationale so future maintainers do not treat it as accidental.

🤖 Prompt for AI Agents 
In .github/workflows/ci.yml around lines 25 to 26, the "Test Only" step runs
`make test` which duplicates test execution already done in the "Test and
Measure Coverage" step, causing unnecessary CI time and resource usage. To fix
this, either remove the `make test` command from the "Test Only" step or add a
condition to run it only on pull requests or specific branches to avoid double
runs on main. Also, add a comment explaining the chosen approach to clarify the
intent for future maintainers.

- name: Test and Measure Coverage
uses: leynos/shared-actions/.github/actions/generate-coverage@c6559452842af6a83b83429129dccaf910e34562
with:
Expand Down
10 changes: 8 additions & 2 deletions crates/comenqd/src/daemon.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,8 @@ mod tests {
async fn setup_run_worker(status: u16) -> (MockServer, Arc<Config>, Receiver, Arc<Octocrab>) {
let dir = tempdir().expect("tempdir");
let cfg = Arc::new(Config {
cooldown_period_seconds: 0,
// Use a positive cooldown to ensure retries are throttled.
cooldown_period_seconds: 1,
..temp_config(&dir)
Comment on lines +337 to 339
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 4, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Use a sub-second cooldown to avoid lengthening the test suite

Setting cooldown_period_seconds to 1 eliminates the busy-spin, but it also injects a full-second sleep into every retry loop. When a test exercises multiple retries this quickly balloons overall runtime. Throttle without slowing the suite by switching to a millisecond-scale delay, e.g.

-cooldown_period_seconds: 1,
+cooldown_period_seconds: 0, // keep struct value
+// …
+tokio::time::sleep(Duration::from_millis(5)).await; // apply during test only

or keep the field but assign 0 here and patch run_worker in tests to inject the delay.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In crates/comenqd/src/daemon.rs around lines 337 to 339, the
cooldown_period_seconds is set to 1, causing a full-second delay on each retry
which slows down the test suite. Change this to a sub-second value, such as
using milliseconds instead of seconds, or set cooldown_period_seconds to 0 here
and modify the run_worker function in tests to apply the desired delay. This
will throttle retries without significantly increasing test runtime.

});
let (mut sender, rx) = channel(&cfg.queue_path).expect("channel");
Expand All @@ -352,7 +353,12 @@ mod tests {
let server = MockServer::start().await;
Mock::given(method("POST"))
.and(path("/repos/o/r/issues/1/comments"))
.respond_with(ResponseTemplate::new(status).set_body_raw("{}", "application/json"))
.respond_with(
ResponseTemplate::new(status).set_body_json(&serde_json::json!({
"id": 1,
"body": "b",
})),
)
.mount(&server)
.await;

Expand Down
Loading