Refactor: split routine_heartbeat.rs into focused submodules (#25)#116
Merged
leynos merged 4 commits intoApr 7, 2026
Merged
Conversation
Split the 514-line tests/e2e_traces/routine_heartbeat.rs into five focused submodules by concern: - routine_cron.rs: cron-triggered routine tests - routine_event.rs: event-triggered routine tests - routine_system_event.rs: system event trigger tests - routine_cooldown.rs: routine cooldown behaviour tests - heartbeat.rs: heartbeat runner tests Extract shared helpers into tests/support/routines.rs: - create_test_db(), create_workspace() - make_routine(), make_test_incoming_message() - make_minimal_engine() - SystemEventSpec, register_github_issue_routine() - assert_system_event_count() Add compile-time signature assertions in tests/support/mod.rs to catch API drift early. Updates tests/e2e_traces.rs to wire new submodules via #[path] attributes following the established harness pattern. Closes #25 Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Contributor
Refactor: split routine_heartbeat.rs into focused submodulesReorganised the monolithic 514-line tests/e2e_traces/routine_heartbeat.rs into five focused submodules organised by concern, extracted shared test helpers into a reusable support module (feature-gated), and added compile-time signature assertions to detect test-support API drift. Test Submodules
Shared Test Support
Quality, hygiene and reviewer feedback
Documentation and guidance
Integration & CI
Labels: size: XL, risk: low, contributor: core WalkthroughSplit the monolithic test suite into focused modules and add shared test-support helpers: new heartbeat, routine_cooldown, routine_cron, routine_event, and routine_system_event tests; remove the original Changes
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size: XL
Contributor
Reviewer's GuideRefactors the monolithic routine_heartbeat E2E test into focused routine- and heartbeat-related test modules, extracts shared helpers into a new test support module with compile‑time signature assertions, and rewires the e2e harness to include the new modules. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e_traces/heartbeat.rs`:
- Around line 74-75: Rename the unused-style binding `_db` to `db` so the
variable accurately reflects that it's used: in the test setup where you call
create_test_db().await and then pass the result into create_workspace(& _db),
change the binding name `_db` to `db` and update the subsequent use in
create_workspace(&db) to remove the misleading underscore prefix.
In `@tests/e2e_traces/routine_cron.rs`:
- Around line 41-60: The test duplicates RoutineEngine construction to access
notify_rx; update the helper make_minimal_engine in routines.rs to accept an
option to return the notification receiver (or return a tuple including the
receiver) so tests can receive notifications: change make_minimal_engine
signature to provide either (engine, notify_rx) or an Option<Receiver> and wire
up the tokio::sync::mpsc::channel creation inside make_minimal_engine (creating
notify_tx passed into RoutineEngine::new and returning notify_rx to the caller);
update callers to destructure the returned receiver when needed and keep
existing behavior for callers that ignore the channel.
In `@tests/e2e_traces/routine_event.rs`:
- Around line 55-56: Replace the fixed
tokio::time::sleep(Duration::from_millis(500)).await with a deterministic
synchronization: either await the handle of the spawned routine if the test
captures the JoinHandle (e.g., await the spawn's JoinHandle), or poll/retry a
condition (with a short backoff loop and timeout) that asserts the routine's
side-effect has completed before proceeding; update references around the spawn
call in tests/e2e_traces/routine_event.rs to use the JoinHandle or the retry
loop instead of an arbitrary 500ms sleep.
In `@tests/support/mod.rs`:
- Around line 359-379: The function routines_symbol_refs is never referenced so
its compile-time signature assertions are optimized away; add a compile-time
const reference to force checking by adding a line that assigns
routines_symbol_refs to a zero-arg function const (e.g., const _: fn() =
routines_symbol_refs;) so the Rust compiler will validate the assertions for
routines::create_workspace, routines::make_routine,
routines::make_test_incoming_message, and routines::make_minimal_engine.
In `@tests/support/routines.rs`:
- Around line 116-117: The helper currently creates a notification channel with
let (notify_tx, _notify_rx) = tokio::sync::mpsc::channel(16) and drops the
receiver, preventing tests from reusing notify_rx (e.g., routine_cron.rs);
modify the helper (the function that builds llm/TraceLlm::from_trace and returns
notify_tx) to also return the receiver (notify_rx) or add an alternative
constructor that exposes both notify_tx and notify_rx so callers can receive
notifications without duplicating engine construction; ensure the returned types
(Arc<TraceLlm>, notify_tx, notify_rx) match existing usage and update callers to
accept the extra receiver.
- Around line 7-8: Replace the crate-level allow attribute with narrowly scoped
expect attributes: remove #![allow(dead_code)] and add #[expect(dead_code,
reason = "used only in e2e_traces")] directly on the specific items
(functions/structs/consts) that are only referenced by e2e_traces; if you cannot
identify each item immediately, apply the expect attribute to the smallest
enclosing module or individual items rather than a crate-wide attribute so the
lint is scoped and includes the given reason.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc33d56b-168c-4dba-b755-4ea2cc46329f
📒 Files selected for processing (9)
tests/e2e_traces.rstests/e2e_traces/heartbeat.rstests/e2e_traces/routine_cooldown.rstests/e2e_traces/routine_cron.rstests/e2e_traces/routine_event.rstests/e2e_traces/routine_heartbeat.rstests/e2e_traces/routine_system_event.rstests/support/mod.rstests/support/routines.rs
💤 Files with no reviewable changes (1)
- tests/e2e_traces/routine_heartbeat.rs
Address code review findings: - heartbeat.rs: Rename _db to db since the variable is used - routine_cron.rs: Use make_minimal_engine helper instead of duplicating RoutineEngine construction; destructure tuple to access notify_rx - routine_event.rs: Replace fixed 500ms sleep with polling loop that checks for routine completion with timeout - support/mod.rs: Add const reference to force compile-time checking of routines_symbol_refs function - support/routines.rs: Replace crate-level #![allow(dead_code)] with narrowly scoped #[allow(dead_code)] attributes on individual items - support/routines.rs: Modify make_minimal_engine to return both the engine and the notification receiver as a tuple All quality gates pass: - cargo fmt -- --check: OK - cargo clippy --tests --features libsql -- -D warnings: OK - cargo test --test e2e_traces --features libsql: 6/6 tests pass Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e_traces/routine_cooldown.rs`:
- Around line 56-70: Replace the fixed tokio::time::sleep with a polling loop
that repeatedly queries the DB until the spawned routine's run appears (same
pattern used in routine_event.rs): use a short tokio::time::sleep inside the
loop, call the DB method that fetches runs for the routine (poll for a run with
routine.id), break when a run is found or time out, then call
db.update_routine_runtime(RoutineRuntimeUpdate { id: routine.id, last_run_at:
Utc::now(), ... }) as before; ensure the loop uses a reasonable timeout to avoid
hanging tests.
In `@tests/e2e_traces/routine_cron.rs`:
- Around line 53-64: Replace the fixed
tokio::time::sleep(Duration::from_millis(500)).await with a bounded polling loop
that repeatedly calls db.list_routine_runs(routine.id, 10).await (the same
approach used in routine_event.rs): retry for a fixed number of attempts with a
short sleep between attempts, break early when runs is non-empty, and assert
failure only after exhausting retries; update the code around the existing
list_routine_runs call and assert to implement this retry/polling pattern.
In `@tests/e2e_traces/routine_system_event.rs`:
- Around line 43-52: Replace the fixed tokio::time::sleep(...) before calling
db.list_routine_runs with a bounded polling loop that repeatedly calls
db.list_routine_runs(routine.id, 10).await (e.g., loop with small delay and max
attempts or overall timeout), returning early when runs is non-empty and failing
the test if the timeout is reached; ensure you await the delay between attempts
and preserve the existing expect/assert behavior when the loop exits.
In `@tests/support/mod.rs`:
- Around line 362-385: The two forbidden #[allow(...)] attributes in the
routines_symbol_refs function should be replaced with narrowly scoped
#[expect(..., reason = "...")] attributes: change #[allow(dead_code)] above
routines_symbol_refs to #[expect(dead_code, reason = "compile-time symbol
reference used for API stability checks in tests")] and change
#[allow(clippy::type_complexity)] immediately before the complex const type
assertion for routines::make_minimal_engine to #[expect(clippy::type_complexity,
reason = "explicit compile-time type assertion for RoutineEngine/Receiver tuple
in tests")]; keep the attributes local to the function and ensure the attribute
names and reasons reference the symbols involved (routines_symbol_refs,
routines::create_workspace, routines::make_minimal_engine,
routines::make_routine, routines::make_test_incoming_message) so the intent is
clear.
In `@tests/support/routines.rs`:
- Around line 46-58: The helper create_test_db currently panics via multiple
.expect() calls; change its signature to return a Result<(Arc<dyn Database>,
TempDir), Box<dyn std::error::Error>> (or your crate's Result type) and
propagate errors with ? instead of .expect(); specifically replace
tempfile::tempdir().expect("tempdir"),
LibSqlBackend::new_local(&db_path).await.expect("LibSqlBackend"), and
backend.run_migrations().await.expect("migrations") with ?-propagation, keeping
the function name create_test_db, the use of LibSqlBackend, run_migrations, and
returning the (db, temp_dir) tuple on Ok so callers can use ? to handle errors.
- Around line 27-43: The file uses forbidden #[allow(dead_code)] attributes;
replace each with #[expect(dead_code, reason = "...")] and supply a brief
justification string. Specifically update the struct and impl for
SystemEventSpec (symbols: SystemEventSpec, new) and every other occurrence
mentioned (the attributes on other structs/functions at lines noted in the
review) to use #[expect(dead_code, reason = "<why this is intentionally unused
in tests>"] so the lint is narrowly scoped and documents the rationale. Ensure
each attribute is converted individually and the reason text is meaningful for
that symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56a305c6-ee6d-4409-aba7-6a3d5fb7a148
📒 Files selected for processing (7)
tests/e2e_traces/heartbeat.rstests/e2e_traces/routine_cooldown.rstests/e2e_traces/routine_cron.rstests/e2e_traces/routine_event.rstests/e2e_traces/routine_system_event.rstests/support/mod.rstests/support/routines.rs
Owner
Author
|
@coderabbitai test support code must be placed in scoped and cfg guarded modules such that only the code that is needed by the test is included in the given test compilation rather than spray and pray lint suppressions. Please create a new GitHub issue for the following so this can be rectified: In
In
.. |
…on (#25) Address additional code review findings: - routine_cooldown.rs: Replace fixed 300ms sleep with polling loop that queries db.list_routine_runs until the routine run appears - routine_cron.rs: Replace fixed 500ms sleep with bounded polling loop that repeatedly calls db.list_routine_runs with early exit on success - routine_system_event.rs: Replace fixed 300ms sleep with bounded polling loop for routine completion detection - support/routines.rs: Change create_test_db signature to return Result<(Arc<dyn Database>, TempDir), Box<dyn std::error::Error>> and propagate errors with ? instead of .expect() - Update all callers of create_test_db to handle the Result with .expect() All quality gates pass: - cargo fmt -- --check: OK - cargo clippy --tests --features libsql -- -D warnings: OK - cargo test --test e2e_traces --features libsql: 6/6 tests pass Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for the following concern and provide an AI coding agent prompt for the fix: ❌ Failed checks (2 warnings)
. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 44
It covers:
✏️ Learnings added
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 282407
Here’s a concrete, low-risk set of changes to clear both warnings, followed by an AI coding agent prompt you can drop into your workflow. Fixes to address “Testing” warning
Patch tests/support/mod.rs (local, function-scoped; annotate complex type assertion inline) --- a/tests/support/mod.rs
+++ b/tests/support/mod.rs
@@
#[cfg(feature = "libsql")]
pub mod routines;
#[cfg(feature = "libsql")]
const _: fn() = routines_symbol_refs;
-#[allow(dead_code)]
-fn routines_symbol_refs() {
+#[expect(
+ dead_code,
+ reason = "compile-time symbol reference used for API stability checks in tests (routines_symbol_refs)"
+)]
+fn routines_symbol_refs() {
use std::sync::Arc;
- // enforce signatures: create_workspace, make_routine, make_test_incoming_message
+ // Enforce signatures: routines::create_workspace, routines::make_routine, routines::make_test_incoming_message
let _: fn(&Arc<dyn ironclaw::db::Database>) -> Arc<ironclaw::Workspace> =
routines::create_workspace;
let _: fn(&str, ironclaw::routines::Trigger, &str) -> ironclaw::routines::Routine =
routines::make_routine;
let _: fn(&str) -> ironclaw::channels::IncomingMessage =
routines::make_test_incoming_message;
- #[allow(clippy::type_complexity)]
+ #[expect(
+ clippy::type_complexity,
+ reason = "explicit compile-time type assertion for RoutineEngine/Receiver tuple in tests (routines::make_minimal_engine)"
+ )]
let _: fn(
tests::trace_llm::LlmTrace,
Arc<dyn ironclaw::db::Database>,
Arc<ironclaw::Workspace>,
) -> (
Arc<ironclaw::routines::RoutineEngine>,
tokio::sync::mpsc::Receiver<ironclaw::channels::OutgoingResponse>,
) = routines::make_minimal_engine;
}Patch tests/support/routines.rs (remove the crate-level allow and narrow to per-item expects with reasons) --- a/tests/support/routines.rs
+++ b/tests/support/routines.rs
@@
-#![allow(dead_code)]
+// No crate-wide allows. Use per-item #[expect(...)] with reasons.
@@
-/// Describes a system event for tests.
-pub struct SystemEventSpec<'a> {
+#[expect(dead_code, reason = "used only in e2e_traces system-event tests (SystemEventSpec)")]
+pub struct SystemEventSpec<'a> {
pub source: &'a str,
pub event_type: &'a str,
pub payload: serde_json::Value,
}
@@
-impl<'a> SystemEventSpec<'a> {
- pub fn new(source: &'a str, event_type: &'a str, payload: serde_json::Value) -> Self {
+impl<'a> SystemEventSpec<'a> {
+ #[expect(dead_code, reason = "constructor is test-only in e2e_traces (SystemEventSpec::new)")]
+ pub fn new(source: &'a str, event_type: &'a str, payload: serde_json::Value) -> Self {
Self { source, event_type, payload }
}
}
@@
-// Creates a temporary libSQL DB for tests.
-pub async fn create_test_db() -> (Arc<dyn Database>, TempDir) {
+#[expect(dead_code, reason = "helper is referenced by specific e2e_traces; can be unused in other slices (create_test_db)")]
+pub async fn create_test_db() -> (Arc<dyn Database>, TempDir) {
/* existing body */
}
@@
-// Builds a Workspace handle from DB.
-pub fn create_workspace(db: &Arc<dyn Database>) -> Arc<Workspace> {
+#[expect(dead_code, reason = "helper is referenced by specific e2e_traces; can be unused in other slices (create_workspace)")]
+pub fn create_workspace(db: &Arc<dyn Database>) -> Arc<Workspace> {
/* existing body */
}
@@
-// Constructs a Routine record for tests.
-pub fn make_routine(name: &str, trigger: Trigger, prompt: &str) -> Routine {
+#[expect(dead_code, reason = "helper is referenced by specific e2e_traces (make_routine)")]
+pub fn make_routine(name: &str, trigger: Trigger, prompt: &str) -> Routine {
/* existing body */
}
@@
-// Minimal incoming message for event-trigger tests.
-pub fn make_test_incoming_message(content: &str) -> IncomingMessage {
+#[expect(dead_code, reason = "helper is referenced by specific e2e_traces (make_test_incoming_message)")]
+pub fn make_test_incoming_message(content: &str) -> IncomingMessage {
/* existing body */
}
@@
-// Makes a minimal engine and returns engine + notify receiver.
-pub fn make_minimal_engine(
+#[expect(dead_code, reason = "helper is referenced by specific e2e_traces (make_minimal_engine)")]
+pub fn make_minimal_engine(
trace: LlmTrace,
db: Arc<dyn Database>,
ws: Arc<Workspace>,
) -> (Arc<RoutineEngine>, tokio::sync::mpsc::Receiver<ironclaw::channels::OutgoingResponse>) {
/* existing body */
}
@@
-// Registers a canned GitHub issue routine for system-event tests.
-pub async fn register_github_issue_routine(db: &Arc<dyn Database>, engine: &RoutineEngine) -> Routine {
+#[expect(dead_code, reason = "helper is referenced by specific e2e_traces (register_github_issue_routine)")]
+pub async fn register_github_issue_routine(db: &Arc<dyn Database>, engine: &RoutineEngine) -> Routine {
/* existing body */
}
@@
-// Asserts that emitting `spec` fires exactly `expected` routines.
-pub async fn assert_system_event_count(
+#[expect(dead_code, reason = "assertion helper is test-only in e2e_traces (assert_system_event_count)")]
+pub async fn assert_system_event_count(
engine: &RoutineEngine,
spec: SystemEventSpec<'_>,
expected: usize,
msg: &str,
) {
/* existing body */
}
Add a tiny helper you can localise to tests/support/routines.rs (scoped, not global). Use it wherever a test currently sleeps to “wait for runs.” #[cfg(feature = "libsql")]
pub async fn poll_until<F, Fut>(
timeout: std::time::Duration,
interval: std::time::Duration,
mut check: F,
) -> anyhow::Result<()>
where
F: FnMut() -> Fut,
Fut: std::future::Future<Output = bool>,
{
use tokio::time::{sleep, Instant};
let deadline = Instant::now() + timeout;
while Instant::now() < deadline {
if check().await {
return Ok(());
}
sleep(interval).await;
}
anyhow::bail!("poll_until timeout after {:?}", timeout);
}Example usage in tests/e2e_traces/routine_cron.rs: // Before:
// tokio::time::sleep(Duration::from_millis(150)).await;
// After:
poll_until(Duration::from_secs(3), Duration::from_millis(50), || async {
!db.list_routine_runs(routine.id, 1).await.unwrap_or_default().is_empty()
})
.await
.unwrap();
Pattern (example for create_test_db; replicate for other fallible helpers) --- a/tests/support/routines.rs
+++ b/tests/support/routines.rs
@@
-pub async fn create_test_db() -> (Arc<dyn Database>, TempDir) {
- let dir = tempfile::tempdir().expect("tempdir");
- let db = LibsqlDatabase::connect(dir.path()).await.expect("connect");
- apply_migrations(&db).await.expect("migrate");
- (Arc::new(db), dir)
-}
+pub type TestResult<T = ()> = anyhow::Result<T>;
+
+pub async fn create_test_db() -> TestResult<(Arc<dyn Database>, TempDir)> {
+ let dir = tempfile::tempdir()?;
+ let db = LibsqlDatabase::connect(dir.path()).await?;
+ apply_migrations(&db).await?;
+ Ok((Arc::new(db), dir))
+}Then update call sites in e2e_traces modules to return anyhow::Result<()> from #[tokio::test] and use ? instead of .expect(...).
Create tests/support_unit_tests/routines_helpers_tests.rs: #![cfg(feature = "libsql")]
use std::sync::Arc;
use crate::support::routines::*;
#[tokio::test]
async fn create_test_db_produces_usable_conn() -> anyhow::Result<()> {
let (db, _tmp) = create_test_db().await?;
// Sanity: listing zero recent runs shouldn’t error
let _ = db.list_recent_threads(Arc::new(Default::default()), 1).await?;
Ok(())
}
#[test]
fn system_event_spec_new_populates_fields() {
let p = serde_json::json!({"k": "v"});
let s = SystemEventSpec::new("github", "issue.opened", p.clone());
assert_eq!(s.source, "github");
assert_eq!(s.event_type, "issue.opened");
assert_eq!(s.payload, p);
}Fixes to address “Developer Documentation” warning
Patch docs/testing-strategy.md (append near the end) --- a/docs/testing-strategy.md
+++ b/docs/testing-strategy.md
@@
+## 8. Rust E2E test helpers (libSQL-gated)
+
+The `tests/support/routines.rs` module (behind `cfg(feature = "libsql")`) exposes a minimal set of re‑usable helpers used by `tests/e2e_traces/*.rs`:
+
+- `create_test_db() -> anyhow::Result<(Arc<dyn Database>, TempDir)>` — sets up a local libSQL database with migrations applied.
+- `create_workspace(db: &Arc<dyn Database>) -> Arc<Workspace>` — produces a `Workspace` bound to that DB.
+- `make_routine(name, trigger, prompt) -> Routine` — constructs a test `Routine`.
+- `make_test_incoming_message(content) -> IncomingMessage` — convenient message factory for event triggers.
+- `make_minimal_engine(trace, db, ws) -> (Arc<RoutineEngine>, mpsc::Receiver<OutgoingResponse>)` — returns a test engine plus notify receiver.
+- `register_github_issue_routine(...)` and `assert_system_event_count(...)` — helpers for system‑event tests.
+- `poll_until(timeout, interval, check)` — a bounded polling loop; prefer this over fixed `sleep()` in tests.
+
+Conventions:
+- Helpers return `anyhow::Result<_>` and use `?` internally; tests should adopt `#[tokio::test] async fn …() -> anyhow::Result<()>` and propagate errors.
+- Where we guard API drift, compile‑time signature checks live in `tests/support/mod.rs` and are wired via `const _: fn() = routines_symbol_refs;`. Lints are documented with narrowly scoped `#[expect(..., reason = "...")]` on local statements, not crate‑wide `#![allow]`.AI coding agent prompt (ready to run) Would you like me to open a follow‑up issue to track the attribute‑hygiene refactor and the helper‑unit tests landing? I can create it on GitHub and link this PR and comment.
✏️ Learnings added
The requested changes have been implemented and a pull request has been created: View PR |
Owner
Author
|
@coderabbitai please revise your suggestion to use correctly scoped conditional compilation and narrow inline module rather than lint suppression. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20945
|
| Problem | Old fix | New fix |
|---|---|---|
dead_code on routines_symbol_refs |
#[allow(dead_code)] |
Removed — function is not dead (const _ references it) |
clippy::type_complexity on make_minimal_engine assertion |
#[allow(clippy::type_complexity)] |
Local type MinimalEngineFn = ... alias |
dead_code on system-event helpers |
#[allow(dead_code)] per item |
Narrow pub mod system_events { } inline block — only compiled when imported |
dead_code on shared helpers |
#[allow(dead_code)] per item |
Removed — items are genuinely referenced by callers |
✏️ Learnings added
Learnt from: leynos
URL: https://github.com/leynos/axinite/pull/116
Timestamp: 2026-04-07T14:53:29.320Z
Learning: In the leynos/axinite repository, dead-code and complexity lints on test support items must be resolved via correctly scoped conditional compilation (#[cfg(...)]) and narrow inline `pub mod` blocks, not via #[allow(...)] or #[expect(...)] suppressions. Items only consumed by one submodule should be grouped in a dedicated inline sub-module. The clippy::type_complexity lint on complex function-pointer const assertions should be eliminated by extracting a local `type` alias rather than suppressing the lint.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Owner
Author
|
@coderabbitai please create a new GitHub issue for the following:
|
) Add section 2.6 'Routine and heartbeat test helpers' to docs/testing-strategy.md documenting the public test helper functions and structs in tests/support/routines.rs: - create_test_db: temp libSQL database with migrations - create_workspace: workspace backed by test database - make_routine: routine factory with sensible defaults - make_test_incoming_message: minimal incoming message builder - make_minimal_engine: routine engine with notification receiver - SystemEventSpec: system event description for tests - register_github_issue_routine: GitHub issue routine helper - assert_system_event_count: system event assertion helper Include E2E test writing patterns with polling loop example for deterministic synchronization without fixed sleeps. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 44
It covers both required changes:
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/support/routines.rs (2)
144-161:⚠️ Potential issue | 🟠 MajorPropagate database errors from
register_github_issue_routine.Stop panicking at Line 159 in shared fixture code. Return
Resultand propagate with?, then handle it at call sites (for exampletests/e2e_traces/routine_system_event.rs, Line 30).♻️ Propagate errors instead of panicking
pub async fn register_github_issue_routine( db: &Arc<dyn Database>, engine: &RoutineEngine, -) -> Routine { +) -> Result<Routine, Box<dyn std::error::Error>> { @@ - db.create_routine(&routine).await.expect("create_routine"); + db.create_routine(&routine).await?; engine.refresh_event_cache().await; - routine + Ok(routine) }As per coding guidelines: "
.expect()and.unwrap()are forbidden outside of tests. Errors must be propagated."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/support/routines.rs` around lines 144 - 161, Change register_github_issue_routine to return a Result<Routine, E> (e.g., anyhow::Result<Routine> or your crate's Error) instead of panicking: update its signature, replace db.create_routine(&routine).await.expect("create_routine") with db.create_routine(&routine).await? and propagate any error from engine.refresh_event_cache().await? as well; update call sites (e.g., tests/e2e_traces/routine_system_event.rs) to handle the Result (use ? or map_err) so errors are propagated rather than causing an expect/unwrap panic.
27-166:⚠️ Potential issue | 🟠 MajorReplace forbidden dead-code suppressions with narrowly scoped expectations.
Remove
#[allow(dead_code)]from helper items (Line 27, Line 34, Line 46, Line 59, Line 65, Line 96, Line 113, Line 143, Line 165). Use item-level#[expect(dead_code, reason = "...")]only where selective test-binary usage truly triggers dead code.🔧 Apply the required lint-hygiene pattern
-#[allow(dead_code)] +#[expect(dead_code, reason = "used only by selected libsql e2e trace modules")] pub struct SystemEventSpec<'a> { @@ -#[allow(dead_code)] +#[expect(dead_code, reason = "used only by selected libsql e2e trace modules")] impl<'a> SystemEventSpec<'a> { @@ -#[allow(dead_code)] +#[expect(dead_code, reason = "used only by selected libsql e2e trace modules")] pub async fn create_test_db() -> Result<(Arc<dyn Database>, TempDir), Box<dyn std::error::Error>> {As per coding guidelines: "
#[allow]is forbidden. Only narrowly scoped#[expect(lint, reason = \"...\")]is allowed."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/support/routines.rs` around lines 27 - 166, Replace the blanket #[allow(dead_code)] usages with narrowly scoped expectations: remove the attribute from the items SystemEventSpec, its impl, create_test_db, create_workspace, make_routine, make_test_incoming_message, make_minimal_engine, register_github_issue_routine, and assert_system_event_count and instead add item-level #[expect(dead_code, reason = "used only in tests / test binary helper")] only on those specific items that truly need it (e.g., helpers referenced only by test binaries); update the attributes directly on the struct, impl, and each function (SystemEventSpec, new impl, create_test_db, create_workspace, make_routine, make_test_incoming_message, make_minimal_engine, register_github_issue_routine, assert_system_event_count) so the lint is narrowly scoped per item.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/support/routines.rs`:
- Around line 144-161: Change register_github_issue_routine to return a
Result<Routine, E> (e.g., anyhow::Result<Routine> or your crate's Error) instead
of panicking: update its signature, replace
db.create_routine(&routine).await.expect("create_routine") with
db.create_routine(&routine).await? and propagate any error from
engine.refresh_event_cache().await? as well; update call sites (e.g.,
tests/e2e_traces/routine_system_event.rs) to handle the Result (use ? or
map_err) so errors are propagated rather than causing an expect/unwrap panic.
- Around line 27-166: Replace the blanket #[allow(dead_code)] usages with
narrowly scoped expectations: remove the attribute from the items
SystemEventSpec, its impl, create_test_db, create_workspace, make_routine,
make_test_incoming_message, make_minimal_engine, register_github_issue_routine,
and assert_system_event_count and instead add item-level #[expect(dead_code,
reason = "used only in tests / test binary helper")] only on those specific
items that truly need it (e.g., helpers referenced only by test binaries);
update the attributes directly on the struct, impl, and each function
(SystemEventSpec, new impl, create_test_db, create_workspace, make_routine,
make_test_incoming_message, make_minimal_engine, register_github_issue_routine,
assert_system_event_count) so the lint is narrowly scoped per item.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6503e6bb-9c53-4153-b54d-8bdfafcb379a
📒 Files selected for processing (7)
docs/testing-strategy.mdtests/e2e_traces/heartbeat.rstests/e2e_traces/routine_cooldown.rstests/e2e_traces/routine_cron.rstests/e2e_traces/routine_event.rstests/e2e_traces/routine_system_event.rstests/support/routines.rs
leynos
added a commit
that referenced
this pull request
Apr 10, 2026
) * Refactor: split routine_heartbeat.rs into focused submodules (#25) Split the 514-line tests/e2e_traces/routine_heartbeat.rs into five focused submodules by concern: - routine_cron.rs: cron-triggered routine tests - routine_event.rs: event-triggered routine tests - routine_system_event.rs: system event trigger tests - routine_cooldown.rs: routine cooldown behaviour tests - heartbeat.rs: heartbeat runner tests Extract shared helpers into tests/support/routines.rs: - create_test_db(), create_workspace() - make_routine(), make_test_incoming_message() - make_minimal_engine() - SystemEventSpec, register_github_issue_routine() - assert_system_event_count() Add compile-time signature assertions in tests/support/mod.rs to catch API drift early. Updates tests/e2e_traces.rs to wire new submodules via #[path] attributes following the established harness pattern. Closes #25 Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * Refactor: address review feedback for routine_heartbeat split (#25) Address code review findings: - heartbeat.rs: Rename _db to db since the variable is used - routine_cron.rs: Use make_minimal_engine helper instead of duplicating RoutineEngine construction; destructure tuple to access notify_rx - routine_event.rs: Replace fixed 500ms sleep with polling loop that checks for routine completion with timeout - support/mod.rs: Add const reference to force compile-time checking of routines_symbol_refs function - support/routines.rs: Replace crate-level #![allow(dead_code)] with narrowly scoped #[allow(dead_code)] attributes on individual items - support/routines.rs: Modify make_minimal_engine to return both the engine and the notification receiver as a tuple All quality gates pass: - cargo fmt -- --check: OK - cargo clippy --tests --features libsql -- -D warnings: OK - cargo test --test e2e_traces --features libsql: 6/6 tests pass Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * Refactor: address review feedback - polling loops and error propagation (#25) Address additional code review findings: - routine_cooldown.rs: Replace fixed 300ms sleep with polling loop that queries db.list_routine_runs until the routine run appears - routine_cron.rs: Replace fixed 500ms sleep with bounded polling loop that repeatedly calls db.list_routine_runs with early exit on success - routine_system_event.rs: Replace fixed 300ms sleep with bounded polling loop for routine completion detection - support/routines.rs: Change create_test_db signature to return Result<(Arc<dyn Database>, TempDir), Box<dyn std::error::Error>> and propagate errors with ? instead of .expect() - Update all callers of create_test_db to handle the Result with .expect() All quality gates pass: - cargo fmt -- --check: OK - cargo clippy --tests --features libsql -- -D warnings: OK - cargo test --test e2e_traces --features libsql: 6/6 tests pass Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * Docs: add routine test helpers documentation to testing-strategy.md (#25) Add section 2.6 'Routine and heartbeat test helpers' to docs/testing-strategy.md documenting the public test helper functions and structs in tests/support/routines.rs: - create_test_db: temp libSQL database with migrations - create_workspace: workspace backed by test database - make_routine: routine factory with sensible defaults - make_test_incoming_message: minimal incoming message builder - make_minimal_engine: routine engine with notification receiver - SystemEventSpec: system event description for tests - register_github_issue_routine: GitHub issue routine helper - assert_system_event_count: system event assertion helper Include E2E test writing patterns with polling loop example for deterministic synchronization without fixed sleeps. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --------- Co-authored-by: leynos <leynos@Rocky-1001-amd64-base> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Split the 514-line tests/e2e_traces/routine_heartbeat.rs into five focused submodules by concern:
Extract shared helpers into tests/support/routines.rs:
Add compile-time signature assertions in tests/support/mod.rs to catch API drift early.
Updates tests/e2e_traces.rs to wire new submodules via #[path] attributes following the established harness pattern.
Closes #25
Summary by Sourcery
Refactor routine and heartbeat E2E tests into focused modules and introduce shared test support utilities for routines.
Enhancements:
Tests: