tests: parallel-safe sqlite test paths (#21) - #45
Conversation
9b6f68c to
3580a89
Compare
|
Hi, @ai-hpc , |
|
Hi, @ai-hpc , |
8103c4e to
bd0a138
Compare
|
@hunnyboy1217 Thanks for the contribution. This issue is related to the beta version and is currently blocked until the alpha version is completed first. |
Two CI regressions surfaced after the most recent `Merge branch 'main' into tests/21-harness-reliability` (6372d5f). 1. **Coverage job had orphan steps.** The merge mashed three steps from upstream's old `test` job (`cargo build --workspace`, `cargo test --workspace`, `cargo test --doc`) into the bottom of PR GeniePod#21's `coverage` job, plus a stray `shared-key: ci-test` key on the `upload-artifact` step's `with:` block where it does not belong. The orphan `cargo test --workspace` re-ran every test, including `binary_size_budget` (which on the coverage runner measured the genie-core release binary at 5.24 MB and panicked against the 5 MB budget), then failed the job — even though the actual `cargo llvm-cov` measurement and artifact upload above had succeeded. Remove the orphan steps and the stray key. The `cargo test --doc` coverage that the upstream `test` job contributed is now a step on the existing `check-and-test` matrix job so doctests still run on every PR. 2. **`--no-default-features` axis could not compile the voice integration test.** PR GeniePod#62 (merged in via 6372d5f) added a new `no-default-features` job that builds `genie-core` + `genie-ctl` with `voice` (and `telegram`) disabled. PR GeniePod#45's `tests/voice_loop_integration.rs` imports `genie_core::voice::*` and `genie_core::voice_loop::*` unconditionally, but those modules are `#[cfg(feature = "voice")]` in `lib.rs`, so the no-default axis fails with five E0432/E0433 "configured out" errors. Gate the whole file with `#![cfg(feature = "voice")]` so it is empty under `--no-default-features` and compiles cleanly. The voice axis still runs every assertion via the matrix + coverage jobs.
ai-hpc
left a comment
There was a problem hiding this comment.
The work splits cleanly into "valuable for #21" and "conflicts with main's CI evolution". Asking for the second part to be dropped on a rebase so the first part can land.
Keep — these are real value-add for #21
crates/genie-core/tests/voice_loop_integration.rs(+511) — the integration test that #21's acceptance criterion B specifically asks for. This is the high-value piece of the PR.crates/genie-core/src/llm/mock.rs(+153) andmod.rs(+14) — the test mock that lets the integration test run without a real LLM. Clean and reusable. Right design point.crates/genie-core/src/memory/mod.rs(+22/-13) — the parallel-safe sqlite test paths that #21's title specifically calls out. Most relevant to the project's actual stability.- **
crates/genie-core/src/voice_loop.rs(+102/-9),voice/stt.rs(+79),voice/tts.rs(+36)** — supporting changes for the integration test. Need to look at whether these have any interaction with PR #61 (real streaming TTS, just merged) or PR #87 (concurrent server /LocalSet`), but the test-only changes should rebase cleanly. tools/parser.rs(+8) — small assist; should rebase.- CHANGELOG entry (+112) — most of it can stay once split into the right
## Unreleasedlocation.
Drop on rebase — .github/workflows/ci.yml rewrite
#21's CI acceptance criterion (C — "CI runs on every PR" — and IS-5 — "matrix runs cargo check / cargo test --lib / cargo clippy --deny warnings") has already been delivered on main by a different shape:
- PR #37 (
8ad9d11) landed the fmt / clippy / test workflow. - PR #62 (
e8b0ef1) added the--no-default-featuresaxis specifically to guard the chat-only build for PR #57'svoiceCargo feature. This PR's rewrite removes that axis — that's a real regression on existing functionality. - PR #89 (
8045d35) split out aContribution / PR body checklistworkflow withpull_request_targetso the check fires on every PR including stale-base ones. - PR #91 (
6158951) added the AI-attribution rule on PR bodies.
Plus, this PR's check-and-test matrix adds a macos-latest axis. macOS isn't a deployment target — GenieClaw is a Jetson appliance — so the macOS leg adds maintenance burden without reflecting the production stack. The coverage (cargo-llvm-cov) job is interesting as a follow-up, but adding it now alongside the matrix rewrite that conflicts with main is too many things at once.
Concretely:
git fetch origin
git checkout -b feat/issue-21-resubmit origin/main
git checkout perf/21-sqlite-test-paths -- \
crates/genie-core/tests/voice_loop_integration.rs \
crates/genie-core/src/llm/mock.rs \
crates/genie-core/src/llm/mod.rs \
crates/genie-core/src/memory/mod.rs \
crates/genie-core/src/tools/parser.rs \
crates/genie-core/src/voice/stt.rs \
crates/genie-core/src/voice/tts.rs \
crates/genie-core/src/voice_loop.rs \
crates/genie-core/tests/tool_dispatch_test.rs
# Reconcile the voice_loop.rs changes against PR #61 (just merged — real
# streaming TTS) and PR #87's `LocalSet` server. Carry just the parts of
# the CHANGELOG entry that match what's actually changing.
# DO NOT touch .github/workflows/ci.yml — that file is now owned by the
# CI evolution that landed across PRs #37 / #62 / #89 / #91.
cargo fmt --all
cargo clippy --workspace --all-targets -- -D warnings
cargo test --workspace --lib
# … if green, open as a fresh PR.Re: the cargo-llvm-cov coverage job
If you still want to land that as a separate piece of work after the integration test is in, it's clearly worth its own PR — and the existing CI is structurally fine to graft a coverage job onto without rewriting the fmt / clippy / test shape.
Soft norm note (carries over from #61 era — not blocking)
Four of five commits on this branch are authored as Hunnyboy1217@gmail.com (personal Gmail) rather than the GitHub-noreply form. Future PRs would be cleaner with git config --global user.email "<numeric-id>+hunnyboy1217@users.noreply.github.com". Same single-vector flag as PR #61 (which I merged anyway); just for next time.
Why this isn't a close
PR #61 was excellent — SentenceStreamer was genuine engineering — so this contributor is clearly capable of delivering quality work. The integration test in this PR is the same quality. The CI rewrite is a casualty of timing (this PR predates the CI evolution on main), not of effort. A clean re-submission against current main should be a quick re-review and merge.
…iePod#21) Re-submission against current `main` per review feedback on the prior revision. Drops the `.github/workflows/ci.yml` rewrite that conflicted with the CI evolution landed across PRs GeniePod#37 / GeniePod#62 / GeniePod#89 / GeniePod#91, drops the macOS matrix axis (Jetson is the deployment target), and drops the coverage job (worth its own PR). Keeps the high-value content: - `voice_loop::process_transcript` — extracted from `voice_cycle` so the post-record orchestration (intent gate, speaker identity, memory recall, quick-tool fast path, LLM streaming + TTS, tool dispatch, conversation persistence, latency banner, memory extract) can be driven with mocks. `ProcessTranscriptInputs` carries `wav_path: Option<&str>` and `tts_engine_override: Option<&TtsEngine>` so tests pass `None` / `Some(&silent_tts)` without leaking test-only ceremony into production. `voice_cycle` is now a thin wrapper around the audio-bound prelude + delegation. - `TtsEngine::snapshot()` — config-only copy so a borrowed `&TtsEngine` can be wrapped in `Arc<TtsEngine>` for `streaming::stream_and_speak`, which is `Arc`-typed since PR GeniePod#61. - `MockLlmBackend` (`LlmClient::mock(replies)`), `SttEngine::mock(transcripts)` + `MockTranscript`, `TtsEngine::silent()` — the test doubles the integration test needs. All three drop into the existing public surface. - `crates/genie-core/tests/voice_loop_integration.rs` — ten `#[tokio::test]` cases. The canonical case `process_transcript_drives_full_voice_cycle_with_mocks` calls `process_transcript` with mock LLM (tool-call reply then summary reply), silent TTS, real `Memory`, real `ConversationStore`, and a `ToolDispatcher` wired to a tool-audit JSONL, then asserts on the three AC-B observables. - `crates/genie-core/src/memory/mod.rs` — every test gets its own `${tmpdir}/geniepod-mem-${label}-${pid}-${id}-${nanos}/` parent dir so `Memory::open`'s `canonical_dir = path.parent().join("memory")` derivation no longer collides under parallel execution. Fixes the `promotion_redacts_person_memory_in_namespace_note` flake. - `crates/genie-core/src/tools/parser.rs` — the `try_tool_call_executes_single_key_system_info_shape` test is now `#[cfg(target_os = "linux")]` because the assertion shape (`Memory available:`) only renders on Linux where `tegrastats::mem_available_mb()` can read `/proc/meminfo`.
cbb0135 to
1820b2b
Compare
|
Hi, @ai-hpc , |
|
Merged at 4eb867a Thanks @hunnyboy1217 |
Closes #21.
Summary
voice_loop::process_transcriptextracted fromvoice_cycle, so the integration test can drive the orchestration end-to-end with mocks.SttEngine::mock,LlmClient::mock(newMockLlmBackendimpl of the existingLlmBackendClienttrait), andTtsEngine::silenttest doubles.tools::parserMemory available:(Linux-only via#[cfg(target_os = "linux")]) andmemory::promotion_redacts_person_memory_in_namespace_note(root cause was sharedcanonical_dir = path.parent().join("memory")— every memory test now gets its own parent dir)..github/workflows/ci.ymlwith a Linux + macOS matrix (fmt, check, clippy-D warnings,cargo test --lib,cargo test --tests,cargo test --doc) plus a separatecargo-llvm-covcoverage job.Scope
This PR closes #21 in full. Intentionally out of scope, tracked under #34:
aarch64-unknown-linux-gnucross-compile job.cargo-auditandcargo-denyjobs.Tests
cargo check -p genie-corecargo test -p genie-core --libcargo test -p genie-core --tests voice_loop_integrationcargo clippy --workspace --all-targets -- -D warningscargo fmt --all -- --checkReal Behavior Proof
What I ran
This branch was prepared without a local Rust toolchain (the system
cargofrom apt is 1.75; the workspace is on edition 2024 and needs rustc ≥1.85). Verification was therefore driven through CI per CONTRIBUTING.md option 2 + 3:Cross-compile (aarch64 / Jetson)job — builds the real aarch64 release binaries (genie-core,genie-ctl,genie-governor,genie-health,genie-api) with the samemake jetsonrecipe that ships to Orin Nano.CI / fmt—cargo fmt --all -- --check.CI / ubuntu-latest / check + clippy + test—cargo check --workspace --all-targets,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace --lib,cargo test --workspace --tests,cargo test --workspace --doc.CI / macos-latest / check + clippy + test— same matrix on macOS so the "fails on macOS" class of bug surfaces on every PR.CI / cargo clippy + test (--no-default-features)— chat-only build axis withvoice+telegramdisabled, scoped to-p genie-core -p genie-ctl.CI / coverage (cargo-llvm-cov)— non-blocking baseline coverage report,lcov.infouploaded as a 14-day artifact.What I observed
voice_loop_integration.rs(10#[tokio::test]cases) drivesprocess_transcriptwith mocks and asserts on the three beta: invest in harness reliability (mock voice cycle, CI matrix, sqlite test isolation) #21 AC-B observables: the user transcript text appears in the conversation history, an assistant message is appended after the LLM call, and aget_timeevent lands in thetool-audit.jsonlaudit log.tools::parserMemory available:andmemory::promotion_redacts_person_memory_in_namespace_note) now pass reliably on every CI run — the parser test only runs on Linux (where/proc/meminfoexists), and every memory test gets its own${tmpdir}/geniepod-mem-${label}-${pid}-${id}-${nanos}/parent dir so the canonical-dir markdown pipeline cannot collide under parallel execution.Cross-compile (aarch64 / Jetson)job has been green on every push to this branch, including this one — concrete proof the changes still build for the deployment target even though the integration test file is now gated behind#![cfg(feature = "voice")].process_transcriptextraction on real hardware before merge; the extraction is a pure refactor (no behavior change invoice_cycle's audio prelude → STT → process_transcript split), but mics in the loop are the kind of thing that surprises you.