test(samples): guard dynamic samples with parse/compile + node-availability checks#543
Conversation
…bility checks Add two registry-independent tests that catch drift between the shipped dynamic sample pipelines and the YAML/compiler + node-registry contracts: - crates/api: every sample in samples/pipelines/dynamic must parse and compile (schema, needs references, pin resolution, cycle rules). - apps/skit: every node kind a sample references is either registered in the default build or a known external kind (marketplace plugin or a codec behind a non-default cargo feature / dedicated hardware). The compiler is registry-agnostic, so a renamed or mistyped node kind compiles fine yet silently fails at runtime when the engine asks the registry to build the node. The node-availability test closes that gap in CI without requiring a GPU, plugins, or models. 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}
| //! (`plugin::…`) or a codec behind a non-default cargo feature / dedicated | ||
| //! hardware. Anything else (e.g. a typo) fails the test. | ||
|
|
||
| #![allow(clippy::expect_used, clippy::panic)] |
There was a problem hiding this comment.
🟡 Lint suppression is missing the required rationale
AGENTS.md requires every lint suppression to include a rationale comment ("Always include ... // reason (#[allow])"), but this new crate-level #[allow] suppresses expect_used/panic without explaining why the suppression is necessary. This violates the repository's mandatory linting-discipline rule for new code.
| #![allow(clippy::expect_used, clippy::panic)] | |
| // Test fixture checks should fail fast with contextual assertion messages. | |
| #![allow(clippy::expect_used, clippy::panic)] |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Done — added the rationale comment to this crate-level suppression in 1c73ee0.
| /// *availability* (feature flags, plugins) is checked separately against a live | ||
| /// registry, since the compiler is registry-agnostic. | ||
| #[test] | ||
| #[allow(clippy::unwrap_used)] |
There was a problem hiding this comment.
🟡 New unwrap suppression lacks the required rationale
AGENTS.md requires lint suppressions to explain their rationale, but this new #[allow(clippy::unwrap_used)] has no accompanying reason. Even if the unwraps are acceptable in this fixture-style test, the suppression itself violates the repository's mandatory linting-discipline rule.
| #[allow(clippy::unwrap_used)] | |
| // Sample fixture traversal should fail fast so failures identify the offending file. | |
| #[allow(clippy::unwrap_used)] |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Done — added a rationale comment above this #[allow(clippy::unwrap_used)] in 1c73ee0.
| const FEATURE_GATED_KINDS: &[&str] = &[ | ||
| "video::svt_av1::encoder", | ||
| "video::dav1d::decoder", | ||
| "video::nv::av1_encoder", | ||
| "video::vaapi::av1_encoder", | ||
| "video::vaapi::h264_encoder", | ||
| "video::vulkan_video::h264_encoder", | ||
| ]; |
There was a problem hiding this comment.
📝 Info: Registry availability test intentionally validates only the default server feature set
The availability test constructs a fresh NodeRegistry and calls streamkit_nodes::register_nodes with GlobalNodeConstraints::new(), so it verifies what the streamkit-server test target can register under the feature set used for that test. This means samples using svt_av1, dav1d, nvcodec, vaapi, or vulkan_video are expected to remain allowlisted rather than validated by registry.contains. That is consistent with the test’s stated scope and with the feature-gated registrations in crates/nodes/src/video/mod.rs:677-693, but reviewers should update FEATURE_GATED_KINDS whenever new non-default dynamic sample kinds are added.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| // Every shipped sample either uses default nodes or a recognized external | ||
| // kind; `unverified_external` records the latter for debugging context. | ||
| assert!( | ||
| unverified_external.iter().all(|k| is_known_external(k)), | ||
| "unexpected external kinds: {unverified_external:?}" | ||
| ); |
There was a problem hiding this comment.
📝 Info: The final external-kind assertion is redundant with the insertion guard
unverified_external is only populated after is_known_external(&kind) has already returned true at lines 89-91, so the later all(|k| is_known_external(k)) assertion cannot catch any additional failure unless the code above is changed incorrectly. This is harmless and may provide future-edit guardrails, but it does not currently add independent validation beyond the main failure collection.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Intentional — it's a future-edit guardrail (cheap, no runtime cost). If someone later changes the insertion logic so an unknown kind reaches unverified_external, this catches it. Leaving as-is.
| match parse_yaml(&yaml) { | ||
| Ok(pipeline) => { | ||
| if let Err(e) = compile(pipeline) { | ||
| failures.push(format!("{name}: compile failed: {e}")); | ||
| } | ||
| }, | ||
| Err(e) => failures.push(format!("{name}: parse failed: {e}")), |
There was a problem hiding this comment.
📝 Info: The API-level sample compilation test deliberately remains registry-agnostic
test_all_dynamic_samples_parse_and_compile exercises parse_yaml and compile, and compile only performs structural validation such as dependency existence, pin mapping, cycle checks, and special mixer parameter injection; it does not consult NodeRegistry (crates/api/src/yaml/compiler.rs:9-17). That means this test will pass for unknown node kinds as long as the graph is structurally valid, which is intentional given the separate server registry test, not a coverage gap in this file.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Add the rationale comments required by AGENTS.md linting discipline to the #[allow] suppressions in the dynamic-sample parse/compile and node-availability tests. Signed-off-by: streamkit-devin <devin@streamkit.dev>
test_overlay_controls.yml is a Playwright fixture (the colorbars->core::sink pipeline driving e2e/tests/overlay-controls.spec.ts), not a user-facing sample, so it was being listed in the Stream view sample picker. Move it to e2e/fixtures/overlay-controls.yaml and load it into the YAML editor directly in the spec instead of selecting it from the sample list. The dynamic-samples loader scans samples/pipelines/dynamic at runtime, so the relocation drops it from /api/v1/samples/dynamic with no loader change. The sample-guard tests auto-discover the directory and now cover 40 samples. Signed-off-by: streamkit-devin <devin@streamkit.dev>
The overlay-controls fixture has no MoQ transport, but editing the YAML editor directly (rather than picking a sample) leaves the broadcast names from the auto-selected first sample in the store, so the post-create auto-connect attempted a MoQ session and failed with a QUIC console error in CI. Clear the input/output broadcast fields after loading the fixture. Signed-off-by: streamkit-devin <devin@streamkit.dev>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #543 +/- ##
==========================================
+ Coverage 79.96% 79.99% +0.02%
==========================================
Files 234 234
Lines 68061 68061
Branches 1846 1933 +87
==========================================
+ Hits 54426 54443 +17
+ Misses 13629 13612 -17
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ssue Replace the tautological final assertion in the node-availability test with a real invariant: every FEATURE_GATED_KINDS entry must be exercised by at least one dynamic sample, so a stale allowlist entry left after a sample is removed fails the test. Reference issue #550 (Stream view doesn't re-derive MoQ settings on direct YAML edits) next to the broadcast-clearing workaround in the overlay-controls spec. Signed-off-by: streamkit-devin <devin@streamkit.dev>
2 participants
Summary
samples/pipelines/dynamic/pipelines to the YAML/compiler + node-registry contracts, so future drift is caught in CI rather than at runtime.crates/api(test_all_dynamic_samples_parse_and_compile): every dynamic sample mustparse_yaml+compilecleanly (schema,needsreferences, pin resolution, cycle rules).apps/skit(every_dynamic_sample_node_kind_is_available_or_known): every nodekinda sample references is either registered in the default build or a recognized external kind — a marketplace plugin (plugin::…) or a codec behind a non-default cargo feature / dedicated GPU (svt_av1,dav1d,nvcodec,vaapi,vulkan_video).test_overlay_controls.ymlout of the shipped samples. It's a Playwright fixture (colorbars→core::sinkdrivingoverlay-controls.spec.ts), not a user-facing sample, yet it appeared in the Stream view sample picker as "Test: Overlay Controls". Moved toe2e/fixtures/overlay-controls.yaml; the spec now loads the YAML straight into the Stream view's editor instead of clicking the sample card. The loader scanssamples/pipelines/dynamicat runtime, so this drops it from/api/v1/samples/dynamicwith no loader/samples.rschange.Review & Validation
cargo test -p streamkit-api --lib test_all_dynamic_samples_parse_and_compilecargo test -p streamkit-server --test dynamic_samples_node_availability_testjust e2e-external http://localhost:4545→overlay-controls.spec.tsstill passes (now loads the relocated fixture).FEATURE_GATED_KINDSallowlist in the new skit test matches the codec/HW kinds intended to be absent from the default build.Notes
e2e/and passes against a local MoQ server; these headless guards complement it.Link to Devin session: https://staging.itsdev.in/sessions/18d695809f504cea85bd63b318684b3d
Requested by: @streamer45
Devin Review
29b7353(HEAD isf9159ac)