test(api): cover yaml compiler, client_lint, and WS protocol round-trips#464
test(api): cover yaml compiler, client_lint, and WS protocol round-trips#464staging-devin-ai-integration[bot] wants to merge 2 commits into
Conversation
Add #[cfg(test)] mod tests blocks to crates/api/src/yaml/compiler.rs,
crates/api/src/yaml/client_lint.rs, and crates/api/src/lib.rs — each
file previously had zero inline tests.
- compiler.rs: cover compile() dispatch, compile_steps sequential
step_{i} naming and consecutive-step edges, compile_dag needs
resolution (single/Vec/Map), unknown-target errors, client
propagation, and the audio::mixer num_inputs auto-injection path.
- client_lint.rs: table-driven test that each rule documented in the
module rustdoc fires for the documented input and not for clean
dynamic/oneshot baselines. Pins that the documented but
unimplemented `publish-no-media` rule is never emitted.
- lib.rs: MessageType lowercase serde round-trips, Request/Response/
Event envelope round-trips, BatchOperation/ValidationErrorType/
EngineMode serde, Connection `mode` skip-on-default, Pipeline
round-trip, and re-export sanity for NodeDefinition / NodeState.
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}
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
🟡 New test modules trigger clippy::expect_used under the required lint task
The added test modules call .expect(...)/.expect_err(...) without a scoped allowance or rationale, but this workspace configures expect_used = "warn" (Cargo.toml:101-102) and just lint-skit promotes all clippy warnings to errors (justfile:277-282). As a result, the required lint task fails on the new tests in both crates/api/src/lib.rs:729 and crates/api/src/yaml/compiler.rs:274, violating the repository's linting requirements from AGENTS.md/CONTRIBUTING.md.
Prompt for agents
Add a scoped clippy allowance with a one-line rationale to the new test modules that intentionally use expect/expect_err for fixture setup and serde assertions. The affected modules are crates/api/src/lib.rs::tests and crates/api/src/yaml/compiler.rs::tests. Keep the allowance limited to test code and include the rationale required by AGENTS.md's linting discipline rule, matching the existing style used in crates/api/src/yaml/tests.rs.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
This claim is incorrect. crates/api/Cargo.toml does not have a [lints] section and does not inherit the workspace lints (lints.workspace = true is absent), so expect_used = "warn" from the workspace Cargo.toml:101-102 does not apply to this crate.
Confirmed empirically:
cargo clippy -p streamkit-api --all-targets -- -D warnings→ clean locally.just lint-skit→ clean locally.- CI
Skit / Lint (fmt + clippy)passed on this PR (job 76419346448).
The style anchor PR (#424) uses .unwrap() in crates/core/src/control.rs tests for the same reason — crates/core/Cargo.toml also doesn't inherit workspace lints. No #[allow] annotation is needed.
| // The `publish-no-media` rule documented at the top of this file is not | ||
| // implemented: `PublishConfig` has no `audio`/`video` boolean fields, | ||
| // only a `tracks` array. Pin the current behaviour so the rustdoc/code | ||
| // drift surfaces if anyone tries to revive the rule without re-reading | ||
| // the model. See PR description "Follow-ups / observations". | ||
| #[test] | ||
| fn publish_no_media_rule_is_not_emitted() { |
There was a problem hiding this comment.
🚩 publish-no-media documentation drift is intentionally pinned rather than fixed
The added test explicitly asserts that publish-no-media is not emitted even though the rustdoc still lists that rule. Existing model types now represent publish media via PublishConfig.tracks rather than audio/video booleans (crates/api/src/yaml/mod.rs:123-136), and the existing test suite already checks the practical replacement rule as empty-tracks (crates/api/src/yaml/tests.rs:875-881). This looks like an intentional compatibility/documentation-drift pin rather than a changed runtime bug, but reviewers may still want to decide whether the rustdoc should be updated in a follow-up.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Correct — intentional pin, already noted as a follow-up in the PR description. Per the task spec this is a test-only PR; the rustdoc update belongs in a separate change so it's reviewable on its own.
| #[test] | ||
| fn compile_dag_oneshot_audio_mixer_auto_injects_num_inputs() { | ||
| let nodes = dag_nodes(&[ | ||
| ("a", user_node("core::source", Needs::None)), | ||
| ("b", user_node("core::source", Needs::None)), | ||
| ( | ||
| "mixer", | ||
| user_node("audio::mixer", Needs::Multiple(vec![simple_dep("a"), simple_dep("b")])), | ||
| ), | ||
| ]); | ||
| let compiled = compile(dag_pipeline(nodes, None, EngineMode::OneShot)).expect("compile"); | ||
| let mixer = compiled.nodes.get("mixer").expect("mixer present"); | ||
| let params = mixer.params.as_ref().expect("params auto-injected"); | ||
| assert_eq!(params.get("num_inputs").and_then(|v| v.as_u64()), Some(2)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn compile_dag_dynamic_audio_mixer_does_not_inject_num_inputs() { | ||
| let nodes = dag_nodes(&[ | ||
| ("a", user_node("core::source", Needs::None)), | ||
| ("b", user_node("core::source", Needs::None)), | ||
| ( | ||
| "mixer", | ||
| user_node("audio::mixer", Needs::Multiple(vec![simple_dep("a"), simple_dep("b")])), | ||
| ), | ||
| ]); | ||
| let compiled = compile(dag_pipeline(nodes, None, EngineMode::Dynamic)).expect("compile"); | ||
| let mixer = compiled.nodes.get("mixer").expect("mixer present"); | ||
| assert!(mixer.params.is_none(), "dynamic mode skips auto-injection: {:?}", mixer.params); | ||
| } | ||
|
|
||
| #[test] | ||
| fn compile_dag_audio_mixer_preserves_explicit_num_inputs() { | ||
| let mut params = serde_json::Map::new(); | ||
| params.insert("num_inputs".into(), serde_json::Value::Number(7.into())); | ||
| let mixer = UserNode { | ||
| kind: "audio::mixer".to_string(), | ||
| params: Some(serde_json::Value::Object(params)), | ||
| needs: Needs::Multiple(vec![simple_dep("a"), simple_dep("b")]), | ||
| }; | ||
| let nodes = dag_nodes(&[ | ||
| ("a", user_node("core::source", Needs::None)), | ||
| ("b", user_node("core::source", Needs::None)), | ||
| ("mixer", mixer), | ||
| ]); | ||
| let compiled = compile(dag_pipeline(nodes, None, EngineMode::OneShot)).expect("compile"); | ||
| let mixer = compiled.nodes.get("mixer").expect("mixer present"); | ||
| let n = mixer.params.as_ref().and_then(|p| p.get("num_inputs")).and_then(|v| v.as_u64()); | ||
| assert_eq!(n, Some(7), "user-provided num_inputs should win"); | ||
| } |
There was a problem hiding this comment.
📝 Info: New compiler tests cover only the auto-injection branch for audio::mixer
The added tests exercise the existing compile_dag branch that injects num_inputs for audio::mixer in non-dynamic mode when multiple incoming connections exist. I did not flag this as a production issue because the PR adds coverage but does not change the compiler logic. One edge case remains untested: when params is present but not an object (for example a scalar/null), line 245 only injects when params.is_none(), so non-object params bypass injection and will later fail or behave according to node config deserialization. That may be acceptable because non-object node params are invalid for this node, but it is worth being aware of if these tests are intended to fully document the injection contract.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Good catch on the non-object params edge case. Pinned in 9a7a140 with compile_dag_oneshot_audio_mixer_skips_inject_when_params_is_non_object — feeds Value::String("scalar") as params and asserts the value survives unchanged (no auto-injection attempted).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #464 +/- ##
==========================================
+ Coverage 65.91% 66.42% +0.51%
==========================================
Files 217 217
Lines 57530 58372 +842
Branches 1597 1597
==========================================
+ Hits 37922 38776 +854
+ Misses 19602 19590 -12
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Adds a regression-pin for the existing compile_dag branch that skips num_inputs auto-injection in OneShot mode when params is present but not an object (e.g. a string/scalar). Surfaces the rustdoc/code drift flagged in Devin Review. Signed-off-by: Staging-Devin AI <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
1 participant
Summary
Phase 3 coverage initiative — Stream A. Adds
#[cfg(test)] mod testsblocks to three previously-zero-inline-test files incrates/api:crates/api/src/yaml/compiler.rs— 20 tests covering:compile()dispatch to both Steps and Dag armscompile_stepssequentialstep_{i}naming and edges between consecutive steps (incl. empty / single-step boundary)compile_stepspropagation ofclient,name,description,modecompile_dagneedsresolution forSingle,Multiple(numberedin_{i}pins),Map(named pin keys), and thenodereserved-key rejectioncompile_dagerror on unknownneedstargetcompile_dagpreservation ofConnectionMode::BestEffortfrom aWithModedepcompile_dagpin-specifier parsing innode.pindep labelscompile_dagaudio::mixernum_inputsauto-injection in OneShot mode, skipped in Dynamic, preserved when user-provided, and skipped whenparamsis present but not an object (regression-pin added in response to PR review)crates/api/src/yaml/client_lint.rs— 7 tests including a table-driveneach_documented_rule_fires_for_its_documented_inputthat exercises every rule listed in the module rustdoc (mode-mismatch-dynamic,mode-mismatch-oneshot,missing-gateway,watch-no-media,input-none-with-accept,input-trigger-with-accept,field-hints-no-input,asset-tags-no-input,text-no-placeholder,empty-broadcast,duplicate-broadcast,empty-tracks,kind-source-mismatch,duplicate-source,empty-track-broadcast). Paired clean-baseline tests confirm none of those rules fire on a valid dynamic or oneshot config.crates/api/src/lib.rs— 22 tests covering:MessageTypelowercase serde round-trip (incl. rejection of capitalized variants)Request/Response/Eventenvelope round-trips with / withoutcorrelation_id(verifyingskip_serializing_if)RequestPayload/ResponsePayload/EventPayloadlowercaseaction/eventtagsBatchOperationtag round-tripValidationErrorTypelowercase serdeEngineModelowercase serde with theoneshotrename andDynamicdefaultConnectionmodedefault skip-on-serialize, non-default emittedPipelinedefault + populated round-trip (incl.stateskip-on-None)PermissionsInforound-trippub usere-exports:NodeDefinitionandNodeState(unit + struct variants) JSON round-trip through this crate's boundaryAll new tests are inline
#[cfg(test)] mod testsblocks at the bottom of the respective files, table-driven where it improves clarity, and follow PR #424's style anchor.Review & Testing Checklist for Human
each_documented_rule_fires_for_its_documented_inputcases match the intent of the rustdoc — in particular that the rule name and the "documented input" each row builds are the intended pairing.publish_no_media_rule_is_not_emittedpin matches the Follow-ups note below (i.e. you agree this is a docs/code drift to track separately rather than a behaviour to fix in this test-only PR).cargo test -p streamkit-apilocally and verify it reports174 passed.Notes
cargo test -p streamkit-api→174 passed; 0 failedlocally.just lint-skitclean. All 28 CI checks passing on the latest commit.devin/1779042395-cov-phase3-api-yaml. Conventional Commits + DCO sign-off.test: add unit tests for zero-coverage crates/core modules).Follow-ups / observations
publish-no-mediarule is documented in theclient_lint.rsrustdoc at the top of the file ("publish block sets both audio and video to false") but is not implemented —PublishConfighas noaudio/videoboolean fields, only atracksarray, so the documented condition cannot be expressed. The "no media in publish" surface is now covered byempty-tracksinstead. This PR pins the current behaviour withpublish_no_media_rule_is_not_emittedand an inline comment; consider either updating the rustdoc to drop the stale rule entry or implementing it differently in a follow-up.audio::mixernum_inputsauto-injection incompile_dagsilently no-ops whenparamsis present but not a JSON object (e.g. a scalar). Pinned bycompile_dag_oneshot_audio_mixer_skips_inject_when_params_is_non_object— flagging in case the intent is to validate / reject non-object params at compile time instead.Link to Devin session: https://staging.itsdev.in/sessions/4b498101ae944a8aafc0aabf4aba7771
Requested by: @streamer45
Devin Review
5e2dc9c(HEAD is9a7a140)