Unit tests for secure-container-runtime
#22
Conversation
…er runtime Note: Lower coverage on client.rs, ws_client.rs, ws_transport.rs, and broker.rs is expected as these contain async I/O code (Unix sockets, WebSocket networking, Docker API calls) that requires infrastructure for integration testing. All synchronous, unit-testable code has comprehensive coverage.
|
Warning Rate limit exceeded@cuteolaf has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds comprehensive unit test coverage across six modules of the secure-container-runtime crate. Tests validate ContainerConfigBuilder methods, SecurityPolicy validation rules, protocol request/response handling, type serialization/deserialization, client constructors, and transport configuration. No production logic is modified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/secure-container-runtime/src/protocol.rs (3)
330-347: Avoid brittlejson.contains(...)assertions; assert via decode + match instead.
contains("create")/contains("challenge-1")can pass even if the JSON shape is wrong (or if values appear in unexpected fields). Prefer decoding and matching the variant/fields.Proposed change
#[test] fn test_create_request() { let config = ContainerConfig { image: "ghcr.io/platformnetwork/test:latest".to_string(), challenge_id: "challenge-1".to_string(), owner_id: "owner-1".to_string(), ..Default::default() }; - let request = Request::Create { - config, - request_id: "req-1".to_string(), - }; - - let json = encode_request(&request); - assert!(json.contains("challenge-1")); - assert!(json.contains("create")); + let request = Request::Create { config, request_id: "req-1".to_string() }; + let json = encode_request(&request); + let decoded = decode_request(&json).unwrap(); + + match decoded { + Request::Create { config, request_id } => { + assert_eq!(request_id, "req-1"); + assert_eq!(config.challenge_id, "challenge-1"); + } + other => panic!("expected Request::Create, got {other:?}"), + } }
349-470: Good coverage, but consider table-driven cases to reduce duplication and ease future enum growth.
The long list ofassert_eq!(...request_type(), "...")and the request/response round-trip loops are correct, but they’ll get noisy as variants evolve. A single table per concept (type/accessor/round-trip) would be easier to extend.
472-492: Prefer a fixed timestamp overchrono::Utc::now()in tests.
It’s not currently asserted on, but using a stable value prevents accidental flakiness if later assertions/serialization snapshots get added.Proposed change
let info = ContainerInfo { id: "c1".into(), name: "test".into(), challenge_id: "ch1".into(), owner_id: "owner1".into(), image: "alpine".into(), state: ContainerState::Running, - created_at: chrono::Utc::now(), + created_at: chrono::DateTime::parse_from_rfc3339("2020-01-01T00:00:00Z") + .unwrap() + .with_timezone(&chrono::Utc), ports: HashMap::new(), endpoint: None, labels: HashMap::new(), };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/secure-container-runtime/src/client.rscrates/secure-container-runtime/src/policy.rscrates/secure-container-runtime/src/protocol.rscrates/secure-container-runtime/src/types.rscrates/secure-container-runtime/src/ws_client.rscrates/secure-container-runtime/src/ws_transport.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/secure-container-runtime/src/policy.rs (1)
crates/secure-container-runtime/src/types.rs (2)
default(64-71)default(88-94)
crates/secure-container-runtime/src/ws_client.rs (2)
crates/secure-container-runtime/src/client.rs (3)
new(19-23)new(554-563)env(575-578)crates/secure-container-runtime/src/protocol.rs (2)
challenge_id(149-155)owner_id(158-164)
🔇 Additional comments (8)
crates/secure-container-runtime/src/ws_transport.rs (1)
362-432: LGTM! Comprehensive test coverage for WebSocket transport.The test suite effectively validates:
- Token generation with different TTLs and expiration calculations
- Default WsConfig values and environment-based configuration
- WsClaims and AuthMessage serialization/deserialization
- Custom configuration construction
All assertions are correct and the tests add valuable coverage for the WebSocket authentication flow.
crates/secure-container-runtime/src/client.rs (1)
681-807: LGTM! Excellent test coverage for ContainerConfigBuilder and result types.The test suite comprehensively validates:
- All builder methods including env, envs, memory, memory_gb, cpu, pids, network configuration, mounts, and labels
- Memory conversion from GB to bytes (line 727)
- Client constructors with custom and default socket paths
- Result type field assignments and helper methods (CleanupResult.success(), etc.)
The builder test (lines 681-719) is particularly thorough, exercising the full fluent API. All assertions are accurate and the tests provide strong coverage for configuration building and client initialization.
crates/secure-container-runtime/src/ws_client.rs (1)
268-364: LGTM! Well-structured tests for WsContainerClient construction.The test suite effectively validates:
- Direct constructor with all field assignments (lines 268-281)
- Environment-based construction with proper fallback logic:
- Returns
Nonewhen required env vars are missing- Prefers
CHALLENGE_UUIDoverCHALLENGE_ID(lines 323-343)- Falls back to "unknown-challenge" and "unknown-owner" when optional vars are absent (lines 345-364)
Good test hygiene with environment variable cleanup before and after each test. The tests thoroughly exercise the
from_env()logic and edge cases.crates/secure-container-runtime/src/types.rs (1)
273-519: LGTM! Comprehensive test coverage for core types.The test suite provides excellent validation of:
- Default implementations for ContainerConfig, ResourceLimits, and NetworkConfig
- Serialization/deserialization round-trips for all enums and structs
- Field assignments and structural integrity
- Error formatting and display for all ContainerError variants
- Label constant values
Particularly good coverage of:
- All enum variants (NetworkMode, ContainerState, AuditAction) with serde round-trips
- Complex types with nested structures (ContainerInfo, ContainerConfig, AuditEntry)
- Edge cases like empty collections and optional fields
All assertions are accurate and the tests provide strong confidence in the type definitions and serde annotations.
crates/secure-container-runtime/src/policy.rs (2)
294-294: LGTM! Necessary import for test assertions.The HashMap import is required for constructing test fixtures in the network validation tests (lines 547, 554).
393-586: LGTM! Comprehensive test coverage for security policy validation.The test suite provides excellent validation of:
- Policy variants (strict, development) and their configurations
- Image whitelist enforcement with
allow_all_images()andwith_image_prefix()- Required field validation (owner_id, challenge_id)
- Resource limit enforcement for CPU, memory, and PIDs
- Mount path validation:
- All forbidden paths (docker.sock variants, /etc/passwd, /etc/shadow, etc.)
- Allowed prefixes (/tmp/, /var/lib/platform/, /var/lib/docker/volumes/)
- Special handling for paths containing "docker.sock"
- Network configuration validation
- Container and owner limits at boundary conditions
Particularly thorough coverage of:
- Boundary testing for limits (lines 564-567: 99 ok, 100 err; lines 574-577: 199 ok, 200 err)
- Systematic validation of all forbidden mount paths (lines 481-499)
- Edge cases like empty mount lists and paths containing "docker.sock"
All assertions correctly reflect the policy logic and default values. The tests provide strong confidence in the security policy enforcement.
crates/secure-container-runtime/src/protocol.rs (2)
303-304: Nice: explicitHashMapimport keeps the tests clean.
No concerns here.
494-498: Good negative-path coverage for decode failures.
These are the right “cheap” cases to ensure the serde tagged-enum contract is enforced.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.