test(nodes): add unit tests for text_chunker, telemetry_out, compositor config, and audio pacer#506
Conversation
…or config, and audio pacer Signed-off-by: streamkit-devin <devin@streamkit.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| #[allow( | ||
| clippy::unwrap_used, | ||
| clippy::expect_used, | ||
| clippy::cast_possible_truncation, | ||
| clippy::cast_sign_loss | ||
| )] |
There was a problem hiding this comment.
🟡 Module-level Clippy suppression has no rationale
The new test module suppresses multiple Clippy lints but does not include the required rationale comment. AGENTS.md requires suppressions to be avoided unless necessary and, when used, to include an explaining comment (AGENTS.md:54-56, AGENTS.md:204-205), so this new attribute violates the repository’s mandatory linting discipline.
| #[allow( | |
| clippy::unwrap_used, | |
| clippy::expect_used, | |
| clippy::cast_possible_truncation, | |
| clippy::cast_sign_loss | |
| )] | |
| #[allow( | |
| clippy::unwrap_used, | |
| clippy::expect_used, | |
| clippy::cast_possible_truncation, | |
| clippy::cast_sign_loss | |
| )] // Test fixtures intentionally use unwrap/expect and fixed-size numeric casts for concise assertions. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in fdd4045 — added rationale comment.
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::unwrap_used, clippy::expect_used)] |
There was a problem hiding this comment.
🟡 Module-level Clippy suppression has no rationale
The new test module suppresses unwrap_used and expect_used without the required rationale comment. AGENTS.md requires every lint suppression to include a reason (AGENTS.md:54-56, AGENTS.md:204-205), so this newly added #[allow] violates the repository rule.
| #[allow(clippy::unwrap_used, clippy::expect_used)] | |
| #[allow(clippy::unwrap_used, clippy::expect_used)] // Tests use unwrap/expect to keep assertion setup concise. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in fdd4045 — added rationale comment.
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::unwrap_used, clippy::expect_used)] |
There was a problem hiding this comment.
🟡 Module-level Clippy suppression has no rationale
The new test module suppresses unwrap_used and expect_used without the required rationale comment. AGENTS.md requires every lint suppression to include a reason (AGENTS.md:54-56, AGENTS.md:204-205), so this newly added #[allow] violates the repository rule.
| #[allow(clippy::unwrap_used, clippy::expect_used)] | |
| #[allow(clippy::unwrap_used, clippy::expect_used)] // Tests use unwrap/expect to keep assertion setup concise. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in fdd4045 — added rationale comment.
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::unwrap_used, clippy::expect_used)] |
There was a problem hiding this comment.
🟡 Module-level Clippy suppression has no rationale
The new test module suppresses unwrap_used and expect_used without the required rationale comment. AGENTS.md requires every lint suppression to include a reason (AGENTS.md:54-56, AGENTS.md:204-205), so this newly added #[allow] violates the repository rule.
| #[allow(clippy::unwrap_used, clippy::expect_used)] | |
| #[allow(clippy::unwrap_used, clippy::expect_used)] // Tests use unwrap/expect to keep assertion setup concise. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in fdd4045 — added rationale comment.
| use super::*; | ||
| use streamkit_core::types::AudioFrame; | ||
|
|
||
| // --- calculate_audio_duration --- |
There was a problem hiding this comment.
🟡 Section-divider comments violate the repository comment rules
The new test module uses // --- ... --- comments as section dividers. AGENTS.md explicitly disallows section dividers and says to use blank lines or code structure instead (AGENTS.md:184-185). The same pattern appears throughout this added module at crates/nodes/src/audio/pacer.rs:407, crates/nodes/src/audio/pacer.rs:432, crates/nodes/src/audio/pacer.rs:463, and crates/nodes/src/audio/pacer.rs:506.
Prompt for agents
Remove the section-divider comments from the new AudioPacer tests in crates/nodes/src/audio/pacer.rs. AGENTS.md disallows comments such as `// --- ... ---`; use blank lines, test names, or nested modules if extra structure is needed.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in fdd4045 — all section dividers removed.
| } | ||
| } | ||
|
|
||
| // --- Default impls --- |
There was a problem hiding this comment.
🟡 Section-divider comments violate the repository comment rules
The new test module uses // --- ... --- comments as section dividers. AGENTS.md explicitly disallows section dividers and says to use blank lines or code structure instead (AGENTS.md:184-185). The same pattern appears throughout this added module at crates/nodes/src/video/compositor/config.rs:624, crates/nodes/src/video/compositor/config.rs:631, crates/nodes/src/video/compositor/config.rs:664, crates/nodes/src/video/compositor/config.rs:673, crates/nodes/src/video/compositor/config.rs:697, crates/nodes/src/video/compositor/config.rs:716, crates/nodes/src/video/compositor/config.rs:742, crates/nodes/src/video/compositor/config.rs:768, crates/nodes/src/video/compositor/config.rs:791, and crates/nodes/src/video/compositor/config.rs:817.
Prompt for agents
Remove the section-divider comments from the new compositor config tests in crates/nodes/src/video/compositor/config.rs. AGENTS.md disallows comments such as `// --- ... ---`; use blank lines, test names, or nested modules if extra structure is needed.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in fdd4045 — all section dividers removed.
| tokio::time::sleep(std::time::Duration::from_millis(50)).await; | ||
|
|
||
| let packets = mock_sender.get_packets_for_pin("out").await; | ||
| assert_eq!(packets.len(), 1); | ||
| match &packets[0] { | ||
| Packet::Text(t) => assert_eq!(t.as_ref(), "First sentence."), | ||
| _ => panic!("Expected text packet"), | ||
| } |
There was a problem hiding this comment.
🟡 Fixed sleep makes the async chunker test timing-dependent
This test waits a fixed 50 ms before checking for the first emitted packet. On a slow or contended CI worker, the spawned node may not process the input before the sleep expires, causing a flaky failure even though the implementation is correct; AGENTS.md also calls out tests updated by adding sleeps as a workaround signal (AGENTS.md:242-243). The existing test helper provides MockOutputSender::recv_timeout for waiting on output deterministically (crates/nodes/src/test_utils.rs:128-137).
| tokio::time::sleep(std::time::Duration::from_millis(50)).await; | |
| let packets = mock_sender.get_packets_for_pin("out").await; | |
| assert_eq!(packets.len(), 1); | |
| match &packets[0] { | |
| Packet::Text(t) => assert_eq!(t.as_ref(), "First sentence."), | |
| _ => panic!("Expected text packet"), | |
| } | |
| let (_, pin, packet) = mock_sender | |
| .recv_timeout(std::time::Duration::from_secs(1)) | |
| .await | |
| .expect("Expected first chunk"); | |
| assert_eq!(pin, "out"); | |
| match packet { | |
| Packet::Text(t) => assert_eq!(t.as_ref(), "First sentence."), | |
| _ => panic!("Expected text packet"), | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| #[cfg(test)] | ||
| #[allow( | ||
| clippy::unwrap_used, | ||
| clippy::expect_used, | ||
| clippy::cast_possible_truncation, | ||
| clippy::cast_sign_loss | ||
| )] | ||
| mod tests { |
There was a problem hiding this comment.
📝 Info: Test-only PR does not change runtime behavior
All changed hunks append #[cfg(test)] modules after existing production code; the production implementations for AudioPacer, TelemetryOut, TextChunker, and compositor config validation are unchanged in this PR. That means concerns such as AudioPacer pacing semantics, TelemetryOut event emission behavior, and compositor validation rules are not newly introduced runtime behavior changes here, so I avoided flagging pre-existing edge cases unless the new tests themselves made them problematic.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| fn filter_dot_star_glob() { | ||
| let params = serde_json::json!({ "event_type_filter": ["vad.*"] }); | ||
| let node = TelemetryOutNode::new(Some(params)).unwrap(); | ||
| assert!(node.matches_event_type_filter("vad.speech_start")); | ||
| assert!(node.matches_event_type_filter("vad.speech_end")); | ||
| assert!(!node.matches_event_type_filter("stt.result")); | ||
| } |
There was a problem hiding this comment.
🚩 Telemetry filter tests codify broad prefix matching
The vad.* test only checks that vad.speech_start and vad.speech_end match and stt.result does not. The implementation slices vad.* to the prefix vad, so vad_something would also match the dot-star form, not just dotted VAD events (crates/nodes/src/core/telemetry_out.rs:93-99). I did not report this as a bug because it is pre-existing production behavior and the configuration docs only describe these as “glob-style prefix patterns,” but reviewers may want to decide whether the new tests should explicitly capture or reject that broader behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Good catch. The vad.* glob strips to prefix vad and matches vad_something too — this is pre-existing production behavior, not introduced by these tests. The filter_star_glob test already explicitly covers that vad* matches vad_something, but worth deciding if vad.* should be stricter. Filed #507 to track.
| let packets = mock_sender.get_packets_for_pin("out").await; | ||
| assert_eq!(packets.len(), 1); | ||
| match &packets[0] { | ||
| Packet::Text(t) => assert_eq!(t.as_ref(), "First sentence."), | ||
| _ => panic!("Expected text packet"), | ||
| } | ||
|
|
||
| drop(input_tx); | ||
| assert_state_stopped(&mut state_rx).await; | ||
| handle.await.unwrap().unwrap(); | ||
|
|
||
| let remaining = mock_sender.get_packets_for_pin("out").await; | ||
| assert_eq!(remaining.len(), 1); |
There was a problem hiding this comment.
📝 Info: TextChunker output collection drains the mock queue intentionally
MockOutputSender::get_packets_for_pin drains all currently queued packets via collect_packets (crates/nodes/src/test_utils.rs:140-155). In run_chunks_text_and_flushes_remainder, the first collection happens before the input channel is closed and the second happens after shutdown, so the test is intentionally asserting the first emitted chunk separately from the final flush rather than inspecting a persistent packet history. This is worth noting because changing the helper to retain history would alter the meaning of the final remaining.len() assertion.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Signed-off-by: streamkit-devin <devin@streamkit.dev>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #506 +/- ##
==========================================
+ Coverage 78.84% 79.39% +0.55%
==========================================
Files 232 232
Lines 66117 66904 +787
Branches 1909 1909
==========================================
+ Hits 52127 53117 +990
+ Misses 13984 13781 -203
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: streamkit-devin <devin@streamkit.dev>
2 participants
Summary
crates/nodes/as part of the Tier 1 coverage sprint.text_chunker.rs— sentence/clause/word splitting, min_length gating, trailing punctuation, CJK boundaries,run()integration (chunking + flush).telemetry_out.rs— packet type filtering (case-insensitive), glob-style event type matching, text truncation, transcription→telemetry JSON structure, custom event type routing (TELEMETRY/VAD passthrough, prefix logic, fallback).compositor/config.rs— validation of canvas dimensions, fps, layer/overlay opacity/rotation/crop, font size and text length limits, output format parsing, and all Default impls.audio/pacer.rs— audio duration calculation, silence frame generation (20ms, correct sample counts), silence cache hit/miss/eviction, speed adjustment, and factory validation (all error paths).Review & Validation
cargo test -p streamkit-nodespasses (109 new tests, 0 failures)just lintpasses cleanlyLink to Devin session: https://staging.itsdev.in/sessions/ec531b4ec13c47059861684e92092583
Requested by: @streamer45
Devin Review
742683c(HEAD isa83596f)