test(nodes): add unit tests for json_serialize, sink, bytes_input, bytes_output, and streaming_utils#504
Conversation
…tes_output, and streaming_utils 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}
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::unwrap_used, clippy::expect_used)] |
There was a problem hiding this comment.
🔴 Lint suppression lacks the required rationale
AGENTS.md requires every lint suppression to include a comment explaining the rationale (AGENTS.md:54-56), but this newly added #[allow(clippy::unwrap_used, clippy::expect_used)] has no rationale. This violates the repository's mandatory linting discipline for PR changes.
| #[allow(clippy::unwrap_used, clippy::expect_used)] | |
| #[allow(clippy::unwrap_used, clippy::expect_used)] // Test assertions use unwrap/expect to fail loudly. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed — added rationale comment in b866884.
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::unwrap_used, clippy::expect_used)] |
There was a problem hiding this comment.
🔴 Lint suppression lacks the required rationale
AGENTS.md requires every lint suppression to include a comment explaining the rationale (AGENTS.md:54-56), but this newly added #[allow(clippy::unwrap_used, clippy::expect_used)] has no rationale. This violates the repository's mandatory linting discipline for PR changes.
| #[allow(clippy::unwrap_used, clippy::expect_used)] | |
| #[allow(clippy::unwrap_used, clippy::expect_used)] // Test assertions use unwrap/expect to fail loudly. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed — added rationale comment in b866884.
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::unwrap_used, clippy::expect_used)] |
There was a problem hiding this comment.
🔴 Lint suppression lacks the required rationale
AGENTS.md requires every lint suppression to include a comment explaining the rationale (AGENTS.md:54-56), but this newly added #[allow(clippy::unwrap_used, clippy::expect_used)] has no rationale. This violates the repository's mandatory linting discipline for PR changes.
| #[allow(clippy::unwrap_used, clippy::expect_used)] | |
| #[allow(clippy::unwrap_used, clippy::expect_used)] // Test assertions use unwrap/expect to fail loudly. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed — added rationale comment in b866884.
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::unwrap_used, clippy::expect_used)] |
There was a problem hiding this comment.
🔴 Lint suppression lacks the required rationale
AGENTS.md requires every lint suppression to include a comment explaining the rationale (AGENTS.md:54-56), but this newly added #[allow(clippy::unwrap_used, clippy::expect_used)] has no rationale. This violates the repository's mandatory linting discipline for PR changes.
| #[allow(clippy::unwrap_used, clippy::expect_used)] | |
| #[allow(clippy::unwrap_used, clippy::expect_used)] // Test assertions use unwrap/expect to fail loudly. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed — added rationale comment in b866884.
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::unwrap_used, clippy::expect_used)] |
There was a problem hiding this comment.
🔴 Lint suppression lacks the required rationale
AGENTS.md requires every lint suppression to include a comment explaining the rationale (AGENTS.md:54-56), but this newly added #[allow(clippy::unwrap_used, clippy::expect_used)] has no rationale. This violates the repository's mandatory linting discipline for PR changes.
| #[allow(clippy::unwrap_used, clippy::expect_used)] | |
| #[allow(clippy::unwrap_used, clippy::expect_used)] // Test assertions use unwrap/expect to fail loudly. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed — added rationale comment in b866884.
| #[test] | ||
| fn new_with_config_ignores_unknown_fields() { | ||
| let (tx, _rx) = mpsc::channel(10); | ||
| let params = serde_json::json!({"unknown": 42}); | ||
| let node = BytesOutputNode::new_with_config(tx, Some(¶ms)).unwrap(); | ||
| assert!(node.configured_content_type().is_none()); | ||
| } |
There was a problem hiding this comment.
📝 Info: Unknown-field config tests document fallback behavior, not strict validation
These newly added tests may look surprising because the config structs use #[serde(deny_unknown_fields)], but BytesOutputNode::new_with_config calls config_helpers::parse_config_optional, whose implementation returns unwrap_or_default() on any deserialization failure (crates/core/src/helpers.rs:25-30). That means an unknown field causes the entire optional config to fall back to defaults instead of returning an error, so the test expectation matches existing shared helper semantics rather than being a new functional bug. If strict unknown-field rejection is desired for these nodes, the root behavior is the shared helper/constructor contract rather than these tests alone.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| #[tokio::test] | ||
| async fn run_serializes_text_to_json_binary() { | ||
| let (input_tx, input_rx) = mpsc::channel(10); | ||
| let mut inputs = HashMap::new(); | ||
| inputs.insert("in".to_string(), input_rx); | ||
| let (context, mock_sender, mut state_rx) = create_test_context(inputs, 10); | ||
|
|
||
| let node = JsonSerialize::new(None).unwrap(); | ||
| let handle = tokio::spawn(async move { Box::new(node).run(context).await }); | ||
|
|
||
| assert_state_running(&mut state_rx).await; | ||
|
|
||
| input_tx.send(Packet::Text("hello".into())).await.unwrap(); | ||
| drop(input_tx); | ||
|
|
||
| assert_state_stopped(&mut state_rx).await; | ||
| handle.await.unwrap().unwrap(); |
There was a problem hiding this comment.
📝 Info: JsonSerialize uses running/stopped state only, matching pre-existing behavior
The new tests for JsonSerialize wait for Running and Stopped but not Initializing. I checked the implementation and it emits only state_helpers::emit_running at startup and emit_stopped at exit (crates/nodes/src/core/json_serialize.rs:67-106), so this is an intentional reflection of the node's current state lifecycle rather than a missing assertion in the tests. Any desire to standardize lifecycle events across nodes would be a broader consistency change outside this PR's added tests.
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 #504 +/- ##
==========================================
+ Coverage 78.10% 78.27% +0.17%
==========================================
Files 232 232
Lines 65234 65656 +422
Branches 1909 1909
==========================================
+ Hits 50948 51393 +445
+ Misses 14280 14257 -23
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
2 participants
Summary
crates/nodes/:json_serialize,sink,bytes_input,bytes_output, andstreaming_utils.Review & Validation
cargo test -p streamkit-nodespasses (36 new tests)just lintcleanLink to Devin session: https://staging.itsdev.in/sessions/8ef5ea42a4a742749127948ba9ce3d31
Requested by: @streamer45
Devin Review
699ba97(HEAD is6e24f0f)