test(plugin-wasm): add unit tests for conversions and wrapper#467
test(plugin-wasm): add unit tests for conversions and wrapper#467staging-devin-ai-integration[bot] wants to merge 2 commits into
Conversation
Cover previously-untested pure-logic surfaces of crates/plugin-wasm: - conversions.rs: all wit_types <-> streamkit_core::types::Packet variants (Audio, Text, Binary, Custom valid + invalid JSON, Transcription/Video flattened to Binary) plus every PacketType mapping including the OpusAudio -> EncodedAudio(Opus, codec_private=None) translation. - wrapper.rs: serialize_params_to_json (None / Null / object / primitive) and receive_from_any_input (empty / pending packet / all closed / closed-then-live). - lib.rs: PluginRuntimeConfig defaults, runtime construction, namespaced_kind branches, PLUGIN_KIND_PREFIX constant, and load_plugins_from_directory behavior on a missing dir and a dir with mixed non-wasm/invalid-wasm files. Two tests pin current broken behavior with inline comments and no fix (per agent_docs/coverage.md): - namespaced_kind: the 'core::' reserved-prefix branch is unreachable because the '::' separator guard catches it first. - PluginRuntime::new: 'enable_simd: false' configurations always fail because wasmtime enables the relaxed-simd proposal by default. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Signed-off-by: Staging-Devin AI <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 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}
| fn plugin_runtime_new_rejects_disabled_simd_due_to_relaxed_simd_default() { | ||
| // BUG: PluginRuntimeConfig exposes `enable_simd: false`, but wasmtime enables the | ||
| // relaxed-simd proposal by default, which requires the base SIMD proposal. The | ||
| // resulting config error ("cannot disable the simd proposal but enable the relaxed | ||
| // simd proposal") means every `enable_simd: false` config — including the threads | ||
| // combination — fails initialization. Either the field should be honored end-to-end | ||
| // (also disabling relaxed_simd) or removed from the public config. Pin current | ||
| // (broken) behavior until fixed. | ||
| for enable_threads in [false, true] { | ||
| let cfg = PluginRuntimeConfig { | ||
| max_memory_bytes: 16 * 1024 * 1024, | ||
| enable_simd: false, | ||
| enable_threads, | ||
| }; | ||
| assert!( | ||
| PluginRuntime::new(cfg).is_err(), | ||
| "expected init to fail when enable_simd=false (enable_threads={enable_threads})" | ||
| ); |
There was a problem hiding this comment.
🟡 Disabled-SIMD test locks in a public configuration bug
PluginRuntimeConfig exposes enable_simd, and PluginRuntime::new forwards it to Wasmtime via wasm_simd(config.enable_simd) without also disabling relaxed SIMD (crates/plugin-wasm/src/lib.rs:73-79). The new test now asserts that every enable_simd: false configuration must fail, even though its own comment calls that behavior broken, so CI will reject the correct runtime fix and the public config option remains unusable for callers that need SIMD disabled.
Prompt for agents
Fix the runtime contract instead of pinning the current failure. In crates/plugin-wasm/src/lib.rs, PluginRuntime::new should either fully honor PluginRuntimeConfig.enable_simd=false by configuring all dependent Wasmtime SIMD proposals consistently, or remove/replace the public option if disabling SIMD is intentionally unsupported. After the runtime behavior is corrected, update plugin_runtime_new_rejects_disabled_simd_due_to_relaxed_simd_default so it asserts the intended contract rather than expecting initialization failure.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Acknowledging the underlying bug is real — but this PR is scoped as test-only per agent_docs/coverage.md §"When you spot a bug while writing tests": pin current behavior + open a tracking issue, do not bundle a fix into a test-only PR. Filed as #469 with the repro and suggested fix. Inline comment now references the issue. When #469 lands, this test should be flipped to assert the intended contract.
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::expect_used, clippy::unwrap_used)] |
There was a problem hiding this comment.
🟡 Test module suppression lacks the required lint rationale
AGENTS.md requires every lint suppression to include an explanatory rationale (Linting discipline / Lint suppression rationales), but this new module-level #[allow(clippy::expect_used, clippy::unwrap_used)] has no // reason. That makes the PR violate the repository’s mandatory lint-suppression rule.
| #[allow(clippy::expect_used, clippy::unwrap_used)] | |
| #[allow(clippy::expect_used, clippy::unwrap_used)] // Tests use expect/unwrap to fail fast with readable assertion context. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in 8a8af40 — added a rationale comment above the #[allow].
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::expect_used, clippy::unwrap_used)] |
There was a problem hiding this comment.
🟡 WASM runtime test suppression lacks the required lint rationale
AGENTS.md requires every lint suppression to include an explanatory rationale (Linting discipline / Lint suppression rationales), but this new module-level #[allow(clippy::expect_used, clippy::unwrap_used)] has no // reason. That makes the PR violate the repository’s mandatory lint-suppression rule.
| #[allow(clippy::expect_used, clippy::unwrap_used)] | |
| #[allow(clippy::expect_used, clippy::unwrap_used)] // Tests use expect/unwrap to fail fast with readable assertion context. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in 8a8af40 — added a rationale comment above the #[allow].
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::expect_used, clippy::unwrap_used)] |
There was a problem hiding this comment.
🟡 WASM wrapper test suppression lacks the required lint rationale
AGENTS.md requires every lint suppression to include an explanatory rationale (Linting discipline / Lint suppression rationales), but this new module-level #[allow(clippy::expect_used, clippy::unwrap_used)] has no // reason. That makes the PR violate the repository’s mandatory lint-suppression rule.
| #[allow(clippy::expect_used, clippy::unwrap_used)] | |
| #[allow(clippy::expect_used, clippy::unwrap_used)] // Tests use expect/unwrap to fail fast with readable assertion context. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in 8a8af40 — added a rationale comment above the #[allow].
| // Ensure it doesn't exist | ||
| let _ = fs::remove_dir_all(&missing); |
There was a problem hiding this comment.
🟡 Temp-dir cleanup comment violates the repository comment rules
AGENTS.md explicitly forbids line-narration comments that just restate the next line. This new comment only says that the following remove_dir_all ensures the directory does not exist, so it violates the mandatory comment guideline and should be removed.
| // Ensure it doesn't exist | |
| let _ = fs::remove_dir_all(&missing); | |
| let _ = fs::remove_dir_all(&missing); |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in 8a8af40 — removed the line-narration comment.
| fn into_wit_transcription_packet_serializes_to_binary_json() { | ||
| let trans = TranscriptionData { | ||
| text: "hello world".to_string(), | ||
| segments: vec![], | ||
| language: Some("en".to_string()), | ||
| metadata: None, | ||
| }; | ||
| let wit = wit_types::Packet::from(Packet::Transcription(Arc::new(trans))); | ||
| let wit_types::Packet::Binary(bytes) = wit else { | ||
| panic!("expected transcription to be flattened to Binary"); | ||
| }; | ||
| let parsed: serde_json::Value = | ||
| serde_json::from_slice(&bytes).expect("transcription JSON parses"); | ||
| assert_eq!(parsed["text"], "hello world"); | ||
| assert_eq!(parsed["language"], "en"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn into_wit_video_packet_flattens_to_binary_dropping_metadata() { | ||
| use streamkit_core::types::{PixelFormat, VideoFrame}; | ||
| // Smallest valid Rgba8 frame: 1x1 = 4 bytes | ||
| let frame = VideoFrame::new(1, 1, PixelFormat::Rgba8, vec![0x11, 0x22, 0x33, 0x44]) | ||
| .expect("valid frame"); | ||
| let wit = wit_types::Packet::from(Packet::Video(frame)); | ||
| match wit { | ||
| wit_types::Packet::Binary(bytes) => { | ||
| assert_eq!(bytes, vec![0x11, 0x22, 0x33, 0x44]); | ||
| }, | ||
| other => panic!("expected video to flatten to Binary, got {other:?}"), | ||
| } |
There was a problem hiding this comment.
📝 Info: Video/transcription flattening tests codify existing lossy WASM boundary behavior
The new conversion tests explicitly assert that Packet::Transcription and Packet::Video become WIT Binary packets. That matches the existing conversion implementation in crates/plugin-wasm/src/conversions.rs:61-95, including the one-time warning for video metadata loss, so I did not treat this as a PR-introduced behavior bug. The practical implication is that these tests will make the current lossy ABI behavior visible and will need updating when the WIT interface gains structured transcription/video types.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| fn namespaced_kind_rejects_reserved_core_prefix() { | ||
| let err = namespaced_kind("core::audio").expect_err("must reject `core::` kinds"); | ||
| // The `core::` check is unreachable today because any string containing `::` is | ||
| // already rejected by the namespace-separator guard. Pin that behavior: the error | ||
| // message references the namespace-separator rule, not the reserved-prefix rule. | ||
| // If `namespaced_kind` is ever refactored to check the reserved prefix first, | ||
| // this assertion will start failing and signal that the contract should be revisited. | ||
| assert!( | ||
| err.contains("reserved for namespace prefixes"), | ||
| "expected namespace-separator error, got: {err}" | ||
| ); |
There was a problem hiding this comment.
📝 Info: The core-prefix validation branch remains intentionally unreachable
The namespaced_kind("core::audio") test asserts the namespace-separator error rather than the reserved-prefix error. This is consistent with the current guard order in crates/plugin-wasm/src/lib.rs:283-302, where any value containing :: is rejected before the core:: prefix check can run. I did not report this as a bug because the test comments make the pinned behavior explicit, but reviewers should be aware that the documented core::-specific error path is still not exercised by any input containing core::.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| #[tokio::test] | ||
| async fn receive_from_any_input_skips_closed_input_and_returns_from_live_input() { | ||
| let (tx_closed, rx_closed) = tokio::sync::mpsc::channel::<Packet>(1); | ||
| drop(tx_closed); | ||
| let (tx_live, rx_live) = tokio::sync::mpsc::channel::<Packet>(1); | ||
| tx_live.send(Packet::Text(Arc::from("from-live"))).await.expect("send succeeds"); | ||
|
|
||
| let mut inputs = vec![("closed".to_string(), rx_closed), ("live".to_string(), rx_live)]; | ||
|
|
||
| let (pin, packet) = | ||
| receive_from_any_input(&mut inputs).await.expect("live input must yield packet"); | ||
| assert_eq!(pin, "live"); | ||
| assert!(matches!(packet, Packet::Text(t) if t.as_ref() == "from-live")); | ||
| assert_eq!(inputs.len(), 1, "closed input must be removed"); | ||
| assert_eq!(inputs[0].0, "live"); |
There was a problem hiding this comment.
📝 Info: receive_from_any_input preserves liveness after removing closed receivers
I checked whether swap_remove could make the helper skip or lose a still-open receiver. Because the function loops back after removing a closed receiver and re-polls the updated vector, the live receiver moved into the removed slot is still considered before returning; the new test covering a closed input before a live input exercises that behavior. This means the removal order change is not a packet-loss bug, although the helper still provides no fairness guarantee across always-ready inputs because it scans from index 0 each poll.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
- Add rationale comments to module-level clippy::expect_used / clippy::unwrap_used allows in conversions.rs, wrapper.rs, lib.rs (required by AGENTS.md 'Linting discipline'). - Remove line-narration comment in lib.rs. - Reference tracking issues #469 (enable_simd=false bug) and #470 (unreachable core:: branch in namespaced_kind) from the pinning tests' BUG comments. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Signed-off-by: Staging-Devin AI <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #467 +/- ##
==========================================
+ Coverage 65.91% 66.32% +0.41%
==========================================
Files 217 217
Lines 57530 57827 +297
Branches 1597 1597
==========================================
+ Hits 37922 38356 +434
+ Misses 19602 19465 -137
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
Summary
Phase 3 coverage initiative — Stream B. Adds the first unit tests to
crates/plugin-wasm(previously 0% — 0 tests across 3 files). 35 new#[test]blocks across#[cfg(test)] mod testssections at the bottom of each target file, following the style anchor from #424 (table-driven where reasonable,serde_json::json!for JSON fixtures, no production-code changes).Coverage breakdown:
crates/plugin-wasm/src/conversions.rs— every conversion variant:TryFrom<wit_types::Packet> for streamkit_core::types::Packet: Audio, Text, Binary, Custom (valid JSON success path), Custom (invalid JSON error path withInvalid custom JSON:message).From<streamkit_core::types::Packet> for wit_types::Packet: Audio, Text, Binary (dropscontent_typeandmetadata), Custom (JSON round-trip), Transcription (serialized to Binary JSON), Video (flattened to opaque Binary).From<&wit_types::PacketType> for streamkit_core::types::PacketType: RawAudio (bothFloat32andS16Le),OpusAudio→EncodedAudio(Opus, codec_private=None), Text, Binary, Custom (preservestype_id), Any.crates/plugin-wasm/src/wrapper.rs— only the pure-logic surfaces (no realwasmtime::component::Componentinvolved):serialize_params_to_json: None /Value::Null/ non-null object / non-null primitive.receive_from_any_input: empty input vec, pending packet with correct pin name, all-closed receivers drained out, closed-then-live ordering.crates/plugin-wasm/src/lib.rs— only the parts that don't need a real.wasmartifact:PluginRuntimeConfig::defaultvalues.PluginRuntime::newwith default and custom-memory-limit configs.namespaced_kindbranches (simple name → prefixed, already-prefixed idempotency,::rejection,core::reserved-prefix behavior).PLUGIN_KIND_PREFIXconstant.load_plugins_from_directoryon a missing directory and on a directory mixing non-.wasmfiles with a malformed.wasmfile.WasmNodeWrapper::input_pins/output_pinsand the rest ofrun(),LoadedPlugin::create_node, andregister_pluginsare intentionally left for an integration-test follow-up — they all require instantiating a real WASM component, which is out of scope for this PR per the task spec.Follow-ups / observations
Two tests pin currently-broken behavior with inline comments and do NOT fix it in this PR, per
agent_docs/coverage.md§"When you spot a bug while writing tests". Both have tracking issues so the pins flip back to assertions of intended behavior when fixed:namespaced_kind—core::reserved-prefix branch is unreachable (tracked in plugin-wasm: namespaced_kindcore::reserved-prefix branch is unreachable #470). The function checkskind.contains("::")beforekind.starts_with("core::"), so anycore::*input is rejected by the namespace-separator guard and never reaches the reserved-prefix check.tests::namespaced_kind_rejects_reserved_core_prefixpins this behavior.PluginRuntime::newrejects everyenable_simd: falseconfig (tracked in plugin-wasm: PluginRuntimeConfig.enable_simd=false fails initialization due to default relaxed-simd proposal #469). Wasmtime enables the relaxed-simd proposal by default, which requires the base SIMD proposal. As a resultPluginRuntimeConfig { enable_simd: false, .. }always errors out withcannot disable the simd proposal but enable the relaxed simd proposal, regardless ofenable_threads.tests::plugin_runtime_new_rejects_disabled_simd_due_to_relaxed_simd_defaultpins this behavior.Review & Testing Checklist for Human
core::should be a distinct error andenable_simd: falseshould work, the pinning tests will start failing when the fixes in plugin-wasm: PluginRuntimeConfig.enable_simd=false fails initialization due to default relaxed-simd proposal #469 / plugin-wasm: namespaced_kindcore::reserved-prefix branch is unreachable #470 land (which is the intended signal).OpusAudio → EncodedAudio(Opus, codec_private=None)mapping inpacket_type_opus_audio_maps_to_encoded_opus_without_codec_privateis the contract you want pinned, since this is the only place that translation is exercised.cargo test -p streamkit-plugin-wasm→ 35 passed,just lint-skit→ ok).Notes
#[cfg(test)] mod testsat the bottom of each file with#[allow(clippy::expect_used, clippy::unwrap_used)]becausecrates/plugin-wasmopts into the workspace lints via[lints] workspace = true(unlikecrates/core, where test: add unit tests for zero-coverage crates/core modules #424 didn't need the allow). A short rationale comment accompanies each suppression per AGENTS.md "Lint suppression rationales".Link to Devin session: https://staging.itsdev.in/sessions/7223a199dcb3428ebd3fd9d6e431e9f7
Requested by: @streamer45
Devin Review
bb9a5d1(HEAD is8a8af40)