diff --git a/crates/libmlx/src/device/cmd/device/args.rs b/crates/libmlx/src/device/cmd/device/args.rs index 1a702ca8ab..45e61acc4b 100644 --- a/crates/libmlx/src/device/cmd/device/args.rs +++ b/crates/libmlx/src/device/cmd/device/args.rs @@ -210,3 +210,381 @@ mod tests { )); } } + +#[cfg(test)] +mod coverage_tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + + use super::*; + + // Projection of a DeviceFilter into PartialEq pieces, since DeviceFilter + // itself is not PartialEq. (field, values, match_mode) captures every + // observable output of the parsers under test. + type FilterParts = (DeviceField, Vec, MatchMode); + + fn parts(f: &DeviceFilter) -> FilterParts { + (f.field.clone(), f.values.clone(), f.match_mode.clone()) + } + + fn owned(values: &[&str]) -> Vec { + values.iter().map(|v| v.to_string()).collect() + } + + // parse_device_field: every accepted alias maps to its DeviceField arm, + // case-insensitively, and any unknown token is rejected. + #[test] + fn parse_device_field_cases() { + check_cases( + [ + Case { + scenario: "device_type canonical", + input: "device_type", + expect: Yields(DeviceField::DeviceType), + }, + Case { + scenario: "type alias", + input: "type", + expect: Yields(DeviceField::DeviceType), + }, + Case { + scenario: "device_type uppercased (lowercased internally)", + input: "DEVICE_TYPE", + expect: Yields(DeviceField::DeviceType), + }, + Case { + scenario: "part_number canonical", + input: "part_number", + expect: Yields(DeviceField::PartNumber), + }, + Case { + scenario: "part alias", + input: "part", + expect: Yields(DeviceField::PartNumber), + }, + Case { + scenario: "firmware_version canonical", + input: "firmware_version", + expect: Yields(DeviceField::FirmwareVersion), + }, + Case { + scenario: "firmware alias", + input: "firmware", + expect: Yields(DeviceField::FirmwareVersion), + }, + Case { + scenario: "fw alias", + input: "fw", + expect: Yields(DeviceField::FirmwareVersion), + }, + Case { + scenario: "mac_address canonical", + input: "mac_address", + expect: Yields(DeviceField::MacAddress), + }, + Case { + scenario: "mac alias", + input: "mac", + expect: Yields(DeviceField::MacAddress), + }, + Case { + scenario: "description canonical", + input: "description", + expect: Yields(DeviceField::Description), + }, + Case { + scenario: "desc alias", + input: "desc", + expect: Yields(DeviceField::Description), + }, + Case { + scenario: "pci_name canonical", + input: "pci_name", + expect: Yields(DeviceField::PciName), + }, + Case { + scenario: "pci alias", + input: "pci", + expect: Yields(DeviceField::PciName), + }, + Case { + scenario: "status canonical", + input: "status", + expect: Yields(DeviceField::Status), + }, + Case { + scenario: "unknown field rejected", + input: "bogus", + expect: FailsWith( + "Unknown field 'bogus'. Valid fields: device_type, part_number, \ + firmware_version, mac_address, description, pci_name, status" + .to_string(), + ), + }, + Case { + scenario: "empty field rejected", + input: "", + expect: FailsWith( + "Unknown field ''. Valid fields: device_type, part_number, \ + firmware_version, mac_address, description, pci_name, status" + .to_string(), + ), + }, + ], + parse_device_field, + ); + } + + // parse_match_mode: each accepted mode (case-insensitive) maps to its + // MatchMode arm; anything else is rejected with the canonical message. + #[test] + fn parse_match_mode_cases() { + check_cases( + [ + Case { + scenario: "regex", + input: "regex", + expect: Yields(MatchMode::Regex), + }, + Case { + scenario: "exact", + input: "exact", + expect: Yields(MatchMode::Exact), + }, + Case { + scenario: "prefix", + input: "prefix", + expect: Yields(MatchMode::Prefix), + }, + Case { + scenario: "uppercased prefix (lowercased internally)", + input: "PREFIX", + expect: Yields(MatchMode::Prefix), + }, + Case { + scenario: "unknown mode rejected", + input: "fuzzy", + expect: FailsWith( + "Unknown match mode 'fuzzy'. Valid modes: regex, exact, prefix".to_string(), + ), + }, + Case { + scenario: "empty mode rejected", + input: "", + expect: FailsWith( + "Unknown match mode ''. Valid modes: regex, exact, prefix".to_string(), + ), + }, + ], + parse_match_mode, + ); + } + + // parse_filter_expression success paths: 2-part defaults to Regex, + // 3-part honors the explicit mode, comma-separated values become an OR + // list, and surrounding whitespace on values is trimmed while empties drop. + #[test] + fn parse_filter_expression_ok_cases() { + check_cases( + [ + Case { + scenario: "two parts default to regex", + input: "device_type:ConnectX-6", + expect: Yields(( + DeviceField::DeviceType, + owned(&["ConnectX-6"]), + MatchMode::Regex, + )), + }, + Case { + scenario: "three parts with explicit prefix mode", + input: "part_number:MCX:prefix", + expect: Yields((DeviceField::PartNumber, owned(&["MCX"]), MatchMode::Prefix)), + }, + Case { + scenario: "comma-separated OR values", + input: "status:ok,fail,warn:exact", + expect: Yields(( + DeviceField::Status, + owned(&["ok", "fail", "warn"]), + MatchMode::Exact, + )), + }, + Case { + scenario: "values are trimmed and empties dropped", + input: "fw: 22.32 , ,1010 :exact", + expect: Yields(( + DeviceField::FirmwareVersion, + owned(&["22.32", "1010"]), + MatchMode::Exact, + )), + }, + Case { + scenario: "alias field with default mode", + input: "mac:00:11", + // Note: three colon-parts here -> third part parsed as mode. + // "00:11" splits to ["mac","00","11"]; "11" is not a mode. + expect: Fails, + }, + ], + |expr| parse_filter_expression(expr).map(|f| parts(&f)), + ); + } + + // parse_filter_expression rejection paths: too few / too many colon parts, + // unknown field propagated, unknown mode propagated, and a values list + // that collapses to empty after trimming. + #[test] + fn parse_filter_expression_err_cases() { + check_cases( + [ + Case { + scenario: "single part (no colon) rejected", + input: "device_type", + expect: FailsWith( + "Invalid filter expression 'device_type'. Expected format: \ + field:value[,value2,value3] or \ + field:value[,value2,value3]:match_mode" + .to_string(), + ), + }, + Case { + scenario: "four parts rejected", + input: "a:b:c:d", + expect: FailsWith( + "Invalid filter expression 'a:b:c:d'. Expected format: \ + field:value[,value2,value3] or \ + field:value[,value2,value3]:match_mode" + .to_string(), + ), + }, + Case { + scenario: "unknown field propagated", + input: "nope:value", + expect: FailsWith( + "Unknown field 'nope'. Valid fields: device_type, part_number, \ + firmware_version, mac_address, description, pci_name, status" + .to_string(), + ), + }, + Case { + scenario: "unknown mode propagated", + input: "device_type:val:bogus", + expect: FailsWith( + "Unknown match mode 'bogus'. Valid modes: regex, exact, prefix".to_string(), + ), + }, + Case { + scenario: "values empty after trim/drop", + input: "device_type: , :exact", + expect: FailsWith( + "No valid values found in filter expression 'device_type: , :exact'" + .to_string(), + ), + }, + ], + |expr| parse_filter_expression(expr).map(|f| parts(&f)), + ); + } + + // build_filter_set_from_filter_args: empty input yields an empty set; + // multiple valid expressions accumulate in order; any single invalid + // expression aborts the whole build. + #[test] + fn build_filter_set_ok_cases() { + check_cases( + [ + Case { + scenario: "empty input -> empty set", + input: vec![], + expect: Yields(vec![]), + }, + Case { + scenario: "single valid expression", + input: vec!["device_type:ConnectX:exact".to_string()], + expect: Yields(vec![( + DeviceField::DeviceType, + owned(&["ConnectX"]), + MatchMode::Exact, + )]), + }, + Case { + scenario: "multiple expressions accumulate in order", + input: vec!["part:MCX:prefix".to_string(), "status:ok".to_string()], + expect: Yields(vec![ + (DeviceField::PartNumber, owned(&["MCX"]), MatchMode::Prefix), + (DeviceField::Status, owned(&["ok"]), MatchMode::Regex), + ]), + }, + ], + |exprs| { + build_filter_set_from_filter_args(exprs) + .map(|set| set.filters.iter().map(parts).collect::>()) + }, + ); + } + + // build_filter_set_from_filter_args aborts on the first bad expression and + // surfaces that expression's error verbatim. + #[test] + fn build_filter_set_propagates_first_error() { + check_cases( + [ + Case { + scenario: "invalid field aborts the build", + input: vec!["device_type:ok".to_string(), "nope:value".to_string()], + expect: FailsWith( + "Unknown field 'nope'. Valid fields: device_type, part_number, \ + firmware_version, mac_address, description, pci_name, status" + .to_string(), + ), + }, + Case { + scenario: "malformed expression aborts the build", + input: vec!["justonepart".to_string()], + expect: FailsWith( + "Invalid filter expression 'justonepart'. Expected format: \ + field:value[,value2,value3] or \ + field:value[,value2,value3]:match_mode" + .to_string(), + ), + }, + ], + |exprs| { + build_filter_set_from_filter_args(exprs) + .map(|set| set.filters.iter().map(parts).collect::>()) + }, + ); + } + + // OutputFormat round-trips through clap's ValueEnum string forms, covering + // every variant's kebab-cased name. + #[test] + fn output_format_value_enum_round_trip() { + check_values( + [ + Check { + scenario: "ascii-table", + input: "ascii-table", + expect: true, + }, + Check { + scenario: "json", + input: "json", + expect: true, + }, + Check { + scenario: "yaml", + input: "yaml", + expect: true, + }, + Check { + scenario: "unknown format not accepted", + input: "xml", + expect: false, + }, + ], + |name| OutputFormat::from_str(name, true).is_ok(), + ); + } +} diff --git a/crates/libmlx/src/embedded/cmd/cmds.rs b/crates/libmlx/src/embedded/cmd/cmds.rs index e0ba73fd09..cd2d55acb7 100644 --- a/crates/libmlx/src/embedded/cmd/cmds.rs +++ b/crates/libmlx/src/embedded/cmd/cmds.rs @@ -1170,3 +1170,88 @@ fn build_credentials( Ok(None) } } + +#[cfg(test)] +mod tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + + use super::*; + + // parse_assignments splits each "VAR=value" on the first '=', trims and skips + // blank entries, and reports precise errors for malformed input. + #[test] + fn parse_assignments_cases() { + fn run(args: &[&str]) -> Result, String> { + parse_assignments(&args.iter().map(|s| s.to_string()).collect::>()) + } + check_cases( + [ + Case { + scenario: "single assignment", + input: &["SRIOV_EN=1"][..], + expect: Yields(vec![("SRIOV_EN".to_string(), "1".to_string())]), + }, + Case { + scenario: "value with '=' splits on the first only", + input: &["KEY=a=b"][..], + expect: Yields(vec![("KEY".to_string(), "a=b".to_string())]), + }, + Case { + scenario: "whitespace trimmed and blank entries skipped", + input: &[" X=1 ", "", " ", "Y=2"][..], + expect: Yields(vec![ + ("X".to_string(), "1".to_string()), + ("Y".to_string(), "2".to_string()), + ]), + }, + Case { + scenario: "missing '=' is rejected", + input: &["NOEQUALS"][..], + expect: FailsWith( + "Invalid assignment format: 'NOEQUALS'. Expected 'variable=value'" + .to_string(), + ), + }, + Case { + scenario: "empty variable name is rejected", + input: &["=value"][..], + expect: FailsWith("Variable name cannot be empty".to_string()), + }, + Case { + scenario: "all-blank input yields no assignments", + input: &["", " "][..], + expect: FailsWith("No valid assignments found".to_string()), + }, + ], + run, + ); + } + + // parse_mlx_show_confs pulls `VAR= description` definitions out of mlxconfig + // show-confs text, skipping the header and section lines. + #[test] + fn parse_mlx_show_confs_extracts_variable_names() { + check_values( + [ + Check { + scenario: "two variables under a section", + input: "List of configurations:\n POWER SETTINGS:\n SRIOV_EN= Enable SR-IOV\n NUM_OF_VFS= Number of virtual functions\n", + expect: vec!["SRIOV_EN".to_string(), "NUM_OF_VFS".to_string()], + }, + Check { + scenario: "header only yields no variables", + input: "List of configurations:\n", + expect: Vec::::new(), + }, + ], + |content| { + parse_mlx_show_confs(content) + .variable_names() + .iter() + .map(|s| s.to_string()) + .collect::>() + }, + ); + } +} diff --git a/crates/libmlx/src/firmware/source.rs b/crates/libmlx/src/firmware/source.rs index 9a664932ca..27796e33e1 100644 --- a/crates/libmlx/src/firmware/source.rs +++ b/crates/libmlx/src/firmware/source.rs @@ -480,3 +480,325 @@ mod tests { Ok(()) } } + +#[cfg(test)] +mod coverage_tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + + use super::*; + + // from_url routes a URL string to the right source variant. We project the + // parsed source to description() (a total, deterministic projection) so each + // row pins both the chosen variant and the carried fields. FirmwareError is + // not PartialEq, so rejection rows use Fails + map_err(drop). Every ssh row + // that yields uses an explicit user so the description is whoami-independent. + #[test] + fn from_url_routes_to_the_right_variant() { + check_cases( + [ + Case { + scenario: "https URL becomes an Http source", + input: "https://example.com/fw.bin", + expect: Yields("http:https://example.com/fw.bin".to_string()), + }, + Case { + scenario: "http URL becomes an Http source", + input: "http://example.com/fw.bin", + expect: Yields("http:http://example.com/fw.bin".to_string()), + }, + Case { + scenario: "file:// prefix is stripped to a Local path", + input: "file:///abs/path/fw.bin", + expect: Yields("local:/abs/path/fw.bin".to_string()), + }, + Case { + scenario: "file:// with a relative remainder", + input: "file://relative/fw.bin", + expect: Yields("local:relative/fw.bin".to_string()), + }, + Case { + scenario: "bare absolute path is a Local source", + input: "/plain/path/fw.bin", + expect: Yields("local:/plain/path/fw.bin".to_string()), + }, + Case { + scenario: "bare relative path is a Local source", + input: "relative/fw.bin", + expect: Yields("local:relative/fw.bin".to_string()), + }, + Case { + scenario: "ssh with explicit user and absolute path", + input: "ssh://deploy@host.example:/abs/fw.bin", + expect: Yields("ssh://deploy@host.example:22:/abs/fw.bin".to_string()), + }, + Case { + scenario: "ssh with explicit user and relative path", + input: "ssh://deploy@host.example:rel/fw.bin", + expect: Yields("ssh://deploy@host.example:22:rel/fw.bin".to_string()), + }, + Case { + scenario: "ssh URL with no colon separator is rejected", + input: "ssh://hostonly", + expect: Fails, + }, + Case { + scenario: "ssh URL with an empty remote path is rejected", + input: "ssh://deploy@host.example:", + expect: Fails, + }, + Case { + scenario: "ssh URL with an empty host is rejected", + input: "ssh://:/abs/fw.bin", + expect: Fails, + }, + ], + |url| { + FirmwareSource::from_url(url) + .map(|s| s.description()) + .map_err(drop) + }, + ); + } + + // An ssh:// URL with no explicit user still parses (username defaults to the + // current user); we only assert that it succeeds since whoami is environment + // dependent. + #[test] + fn from_url_defaults_ssh_user() { + Case { + scenario: "ssh without an explicit user parses", + input: "ssh://host.example:/abs/fw.bin", + expect: Yields(()), + } + .check(|url| FirmwareSource::from_url(url).map(|_| ()).map_err(drop)); + } + + // parse_ssh_url splits an SCP-style ssh:// URL into (host, username, + // remote_path). We project to (host, remote_path) so the assertion stays + // whoami-independent even on the default-user rows. Rejection rows use Fails + // (ConfigError is not PartialEq). + #[test] + fn parse_ssh_url_splits_components() { + check_cases( + [ + Case { + scenario: "user, host, absolute path", + input: "ssh://deploy@host.example:/abs/fw.bin", + expect: Yields(("host.example".to_string(), "/abs/fw.bin".to_string())), + }, + Case { + scenario: "user, host, relative path", + input: "ssh://deploy@host.example:rel/fw.bin", + expect: Yields(("host.example".to_string(), "rel/fw.bin".to_string())), + }, + Case { + scenario: "no user, host, absolute path", + input: "ssh://host.example:/abs/fw.bin", + expect: Yields(("host.example".to_string(), "/abs/fw.bin".to_string())), + }, + Case { + scenario: "path containing a colon keeps the rest after the first colon", + input: "ssh://host.example:/abs:with:colons", + expect: Yields(("host.example".to_string(), "/abs:with:colons".to_string())), + }, + Case { + scenario: "not an ssh URL (missing prefix)", + input: "https://host.example/fw.bin", + expect: Fails, + }, + Case { + scenario: "missing the colon separator", + input: "ssh://hostonly", + expect: Fails, + }, + Case { + scenario: "empty remote path", + input: "ssh://deploy@host.example:", + expect: Fails, + }, + Case { + scenario: "empty host with a user", + input: "ssh://deploy@:/abs/fw.bin", + expect: Fails, + }, + Case { + scenario: "empty host without a user", + input: "ssh://:/abs/fw.bin", + expect: Fails, + }, + ], + |url| { + parse_ssh_url(url) + .map(|(host, _user, path)| (host, path)) + .map_err(drop) + }, + ); + } + + // description() formats each variant. Builders set deterministic fields so the + // ssh rows are whoami-independent. Local/Http carry their raw field verbatim. + #[test] + fn description_formats_each_variant() { + check_values( + [ + Check { + scenario: "local path", + input: FirmwareSource::local("/tmp/fw.bin"), + expect: "local:/tmp/fw.bin".to_string(), + }, + Check { + scenario: "http url", + input: FirmwareSource::http("https://example.com/fw.bin"), + expect: "http:https://example.com/fw.bin".to_string(), + }, + Check { + scenario: "ssh with explicit user, port, host, path", + input: FirmwareSource::ssh("host.example", "/abs/fw.bin") + .with_username("deploy") + .with_port(2222), + expect: "ssh://deploy@host.example:2222:/abs/fw.bin".to_string(), + }, + ], + |src| src.description(), + ); + } + + // The ssh() constructor defaults to port 22; with_port overrides it and only + // affects Ssh sources. We read the port back through description(). + #[test] + fn ssh_constructor_and_builders_set_fields() { + // Default port is 22. + Check { + scenario: "ssh defaults to port 22", + input: FirmwareSource::ssh("h", "/p").with_username("u"), + expect: "ssh://u@h:22:/p".to_string(), + } + .check(|src| src.description()); + + // with_port and with_username are no-ops on non-ssh sources, so the + // description is unchanged. + check_values( + [ + Check { + scenario: "with_port is a no-op on Local", + input: FirmwareSource::local("/tmp/fw.bin").with_port(9999), + expect: "local:/tmp/fw.bin".to_string(), + }, + Check { + scenario: "with_username is a no-op on Http", + input: FirmwareSource::http("https://x/fw").with_username("ignored"), + expect: "http:https://x/fw".to_string(), + }, + ], + |src| src.description(), + ); + } + + // with_credentials attaches a credential to Http and Ssh sources and is a + // no-op on Local. We project to whether credentials ended up Some so the row + // pins the variant-specific behavior without exposing secrets. + #[test] + fn with_credentials_targets_http_and_ssh_only() { + check_values( + [ + Check { + scenario: "Http source stores the credential", + input: FirmwareSource::http("https://x/fw") + .with_credentials(Credentials::bearer_token("t")), + expect: true, + }, + Check { + scenario: "Ssh source stores the credential", + input: FirmwareSource::ssh("h", "/p") + .with_credentials(Credentials::ssh_agent()), + expect: true, + }, + Check { + scenario: "Local source ignores the credential", + input: FirmwareSource::local("/tmp/fw.bin") + .with_credentials(Credentials::bearer_token("t")), + expect: false, + }, + ], + |src| match src { + FirmwareSource::Http { credentials, .. } => credentials.is_some(), + FirmwareSource::Ssh(SshSource { credentials, .. }) => credentials.is_some(), + FirmwareSource::Local { .. } => false, + }, + ); + } + + // credential_type_name maps each Credentials variant to a stable, secret-free + // label. One row per variant. + #[test] + fn credential_type_name_covers_every_variant() { + check_values( + [ + Check { + scenario: "bearer token", + input: Credentials::bearer_token("t"), + expect: "bearer_token", + }, + Check { + scenario: "basic auth", + input: Credentials::basic_auth("u", "p"), + expect: "basic_auth", + }, + Check { + scenario: "header", + input: Credentials::header("X-Key", "v"), + expect: "header", + }, + Check { + scenario: "ssh key", + input: Credentials::ssh_key("/k"), + expect: "ssh_key", + }, + Check { + scenario: "ssh agent", + input: Credentials::ssh_agent(), + expect: "ssh_agent", + }, + ], + |cred| credential_type_name(&cred), + ); + } + + // shell_escape single-quotes its argument and escapes embedded single quotes + // as '\'' so the result is safe to splice into an SSH command. Covers the + // no-special-chars, single-quote, leading-quote, and empty cases. + #[test] + fn shell_escape_quotes_and_escapes() { + check_values( + [ + Check { + scenario: "plain path is wrapped in single quotes", + input: "/firmware/fw.bin", + expect: "'/firmware/fw.bin'".to_string(), + }, + Check { + scenario: "empty string", + input: "", + expect: "''".to_string(), + }, + Check { + scenario: "single embedded quote", + input: "a'b", + expect: "'a'\\''b'".to_string(), + }, + Check { + scenario: "leading quote", + input: "'x", + expect: "''\\''x'".to_string(), + }, + Check { + scenario: "two embedded quotes", + input: "a'b'c", + expect: "'a'\\''b'\\''c'".to_string(), + }, + ], + shell_escape, + ); + } +} diff --git a/crates/libmlx/src/lockdown/lockdown.rs b/crates/libmlx/src/lockdown/lockdown.rs index d3018f5124..9d1bfe3f41 100644 --- a/crates/libmlx/src/lockdown/lockdown.rs +++ b/crates/libmlx/src/lockdown/lockdown.rs @@ -231,3 +231,465 @@ impl From for StatusReport { } } } + +#[cfg(test)] +mod coverage_tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + + use super::*; + + // Every LockStatus serde round-trips through its lowercase rename: serialize to + // JSON, deserialize back, and you land on the same variant. This pins all three + // arms of the `rename_all = "lowercase"` encoding via a stable equality. + #[test] + fn lock_status_round_trips_through_json() { + check_cases( + [ + Case { + scenario: "locked", + input: LockStatus::Locked, + expect: Yields(LockStatus::Locked), + }, + Case { + scenario: "unlocked", + input: LockStatus::Unlocked, + expect: Yields(LockStatus::Unlocked), + }, + Case { + scenario: "unknown", + input: LockStatus::Unknown, + expect: Yields(LockStatus::Unknown), + }, + ], + |status: LockStatus| { + let json = serde_json::to_string(&status).map_err(drop)?; + serde_json::from_str(&json).map_err(drop) + }, + ); + } + + // The exact lowercase JSON token each variant serializes to -- this IS the + // wire contract (the `rename_all = "lowercase"`), so plain string equality is + // the right assertion. + #[test] + fn lock_status_serializes_to_lowercase_token() { + check_cases( + [ + Case { + scenario: "locked", + input: LockStatus::Locked, + expect: Yields("\"locked\"".to_string()), + }, + Case { + scenario: "unlocked", + input: LockStatus::Unlocked, + expect: Yields("\"unlocked\"".to_string()), + }, + Case { + scenario: "unknown", + input: LockStatus::Unknown, + expect: Yields("\"unknown\"".to_string()), + }, + ], + |status: LockStatus| serde_json::to_string(&status).map_err(drop), + ); + } + + // Deserializing the lowercase tokens lands on the matching variant; an unknown + // token fails. Covers the deserialize direction for all three arms plus the + // rejection path. + #[test] + fn lock_status_deserializes_each_token() { + check_cases( + [ + Case { + scenario: "locked", + input: "\"locked\"", + expect: Yields(LockStatus::Locked), + }, + Case { + scenario: "unlocked", + input: "\"unlocked\"", + expect: Yields(LockStatus::Unlocked), + }, + Case { + scenario: "unknown", + input: "\"unknown\"", + expect: Yields(LockStatus::Unknown), + }, + Case { + scenario: "an unrecognized token is rejected", + input: "\"bogus\"", + expect: Fails, + }, + ], + |raw: &str| serde_json::from_str::(raw).map_err(drop), + ); + } + + // LockStatus -> LockStatusPb covers every match arm of the protobuf conversion. + // Comparing the resulting pb's i32 discriminant pins the mapping to the proto + // numbers (UNKNOWN=0, LOCKED=1, UNLOCKED=2) without relying on Pb being PartialEq. + #[test] + fn lock_status_into_pb_discriminant() { + check_values( + [ + Check { + scenario: "locked -> 1", + input: LockStatus::Locked, + expect: 1, + }, + Check { + scenario: "unlocked -> 2", + input: LockStatus::Unlocked, + expect: 2, + }, + Check { + scenario: "unknown -> 0", + input: LockStatus::Unknown, + expect: 0, + }, + ], + |status: LockStatus| LockStatusPb::from(status) as i32, + ); + } + + // LockStatusPb -> LockStatus covers every match arm of the reverse conversion. + #[test] + fn lock_status_from_pb() { + check_values( + [ + Check { + scenario: "Locked", + input: LockStatusPb::Locked, + expect: LockStatus::Locked, + }, + Check { + scenario: "Unlocked", + input: LockStatusPb::Unlocked, + expect: LockStatus::Unlocked, + }, + Check { + scenario: "Unknown", + input: LockStatusPb::Unknown, + expect: LockStatus::Unknown, + }, + ], + LockStatus::from, + ); + } + + // LockStatus survives a full pb round-trip (LockStatus -> pb -> LockStatus) for + // every variant. This exercises both From impls together and pins them as + // inverses without depending on the pb type being PartialEq. + #[test] + fn lock_status_round_trips_through_pb() { + check_values( + [ + Check { + scenario: "locked", + input: LockStatus::Locked, + expect: LockStatus::Locked, + }, + Check { + scenario: "unlocked", + input: LockStatus::Unlocked, + expect: LockStatus::Unlocked, + }, + Check { + scenario: "unknown", + input: LockStatus::Unknown, + expect: LockStatus::Unknown, + }, + ], + |status: LockStatus| LockStatus::from(LockStatusPb::from(status)), + ); + } + + // StatusReport -> StatusReportPb copies device_id and timestamp through verbatim + // and encodes status as its i32 discriminant. We project the pb back to a tuple + // of the fields we are sure about, keyed by status variant. + #[test] + fn status_report_into_pb_preserves_fields() { + check_values( + [ + Check { + scenario: "locked", + input: LockStatus::Locked, + expect: ( + "dev-a".to_string(), + 1, + "2026-01-01T00:00:00+00:00".to_string(), + ), + }, + Check { + scenario: "unlocked", + input: LockStatus::Unlocked, + expect: ( + "dev-a".to_string(), + 2, + "2026-01-01T00:00:00+00:00".to_string(), + ), + }, + Check { + scenario: "unknown", + input: LockStatus::Unknown, + expect: ( + "dev-a".to_string(), + 0, + "2026-01-01T00:00:00+00:00".to_string(), + ), + }, + ], + |status: LockStatus| { + let report = StatusReport { + device_id: "dev-a".to_string(), + status, + timestamp: "2026-01-01T00:00:00+00:00".to_string(), + }; + let pb = StatusReportPb::from(report); + (pb.device_id, pb.status, pb.timestamp) + }, + ); + } + + // StatusReportPb -> StatusReport maps a valid status discriminant back to the + // matching variant, and -- the key uncovered branch -- coerces any out-of-range + // discriminant to Unknown via the `unwrap_or(LockStatus::Unknown)`. device_id and + // timestamp pass through unchanged; we project (device_id, status, timestamp). + #[test] + fn status_report_from_pb_maps_status_and_coerces_invalid() { + check_values( + [ + Check { + scenario: "0 -> Unknown", + input: 0, + expect: ("dev-b".to_string(), LockStatus::Unknown, "ts".to_string()), + }, + Check { + scenario: "1 -> Locked", + input: 1, + expect: ("dev-b".to_string(), LockStatus::Locked, "ts".to_string()), + }, + Check { + scenario: "2 -> Unlocked", + input: 2, + expect: ("dev-b".to_string(), LockStatus::Unlocked, "ts".to_string()), + }, + Check { + scenario: "out-of-range discriminant coerces to Unknown", + input: 99, + expect: ("dev-b".to_string(), LockStatus::Unknown, "ts".to_string()), + }, + Check { + scenario: "negative discriminant coerces to Unknown", + input: -1, + expect: ("dev-b".to_string(), LockStatus::Unknown, "ts".to_string()), + }, + ], + |status_i32: i32| { + let pb = StatusReportPb { + device_id: "dev-b".to_string(), + status: status_i32, + timestamp: "ts".to_string(), + }; + let report = StatusReport::from(pb); + (report.device_id, report.status, report.timestamp) + }, + ); + } + + // A StatusReport survives a full pb round-trip for every status variant, with + // device_id and timestamp intact. Exercises both StatusReport <-> StatusReportPb + // From impls as inverses. + #[test] + fn status_report_round_trips_through_pb() { + check_values( + [ + Check { + scenario: "locked", + input: LockStatus::Locked, + expect: ("d".to_string(), LockStatus::Locked, "t".to_string()), + }, + Check { + scenario: "unlocked", + input: LockStatus::Unlocked, + expect: ("d".to_string(), LockStatus::Unlocked, "t".to_string()), + }, + Check { + scenario: "unknown", + input: LockStatus::Unknown, + expect: ("d".to_string(), LockStatus::Unknown, "t".to_string()), + }, + ], + |status: LockStatus| { + let report = StatusReport { + device_id: "d".to_string(), + status, + timestamp: "t".to_string(), + }; + let back = StatusReport::from(StatusReportPb::from(report)); + (back.device_id, back.status, back.timestamp) + }, + ); + } + + // StatusReport::new derives a fresh RFC3339 timestamp and stores the given + // device_id / status verbatim. We can't pin the exact timestamp, so we assert + // the parts we control and that the timestamp is non-empty + RFC3339-parseable. + #[test] + fn status_report_new_sets_fields_and_timestamp() { + let report = StatusReport::new("dev-new".to_string(), LockStatus::Locked); + assert_eq!(report.device_id, "dev-new"); + assert_eq!(report.status, LockStatus::Locked); + assert!(!report.timestamp.is_empty()); + assert!(chrono::DateTime::parse_from_rfc3339(&report.timestamp).is_ok()); + } + + // to_json round-trips: the pretty JSON it produces deserializes back into an + // equivalent report. Pins device_id/status (StatusReport is not PartialEq, so we + // project) and proves the JSON is well-formed for every status variant. + #[test] + fn status_report_to_json_round_trips() { + check_cases( + [ + Case { + scenario: "locked", + input: LockStatus::Locked, + expect: Yields(("dev-j".to_string(), LockStatus::Locked)), + }, + Case { + scenario: "unlocked", + input: LockStatus::Unlocked, + expect: Yields(("dev-j".to_string(), LockStatus::Unlocked)), + }, + Case { + scenario: "unknown", + input: LockStatus::Unknown, + expect: Yields(("dev-j".to_string(), LockStatus::Unknown)), + }, + ], + |status: LockStatus| { + let report = StatusReport::new("dev-j".to_string(), status); + let json = report.to_json().map_err(drop)?; + let back: StatusReport = serde_json::from_str(&json).map_err(drop)?; + Ok::<_, ()>((back.device_id, back.status)) + }, + ); + } + + // to_yaml round-trips through serde_yaml for every status variant. Projects + // device_id/status since StatusReport isn't PartialEq. A YAML parse error would + // surface as the MlxError::ParseError wrapping path, but the happy round-trip is + // the assertion here. + #[test] + fn status_report_to_yaml_round_trips() { + check_cases( + [ + Case { + scenario: "locked", + input: LockStatus::Locked, + expect: Yields(("dev-y".to_string(), LockStatus::Locked)), + }, + Case { + scenario: "unlocked", + input: LockStatus::Unlocked, + expect: Yields(("dev-y".to_string(), LockStatus::Unlocked)), + }, + Case { + scenario: "unknown", + input: LockStatus::Unknown, + expect: Yields(("dev-y".to_string(), LockStatus::Unknown)), + }, + ], + |status: LockStatus| -> Result<(String, LockStatus), ()> { + let report = StatusReport::new("dev-y".to_string(), status); + let yaml = report.to_yaml().map_err(drop)?; + let back: StatusReport = serde_yaml::from_str(&yaml).map_err(drop)?; + Ok((back.device_id, back.status)) + }, + ); + } + + // The manager validates the device id before touching the runner: an empty id + // and an id containing a space are rejected (InvalidDeviceId), while a + // well-formed id passes validation and only then hits the runner. With a dry-run + // runner a valid id reaches DryRun; the invalid ids never get that far. We map to + // a stable token naming the error variant (MlxError isn't PartialEq). + #[test] + fn manager_rejects_invalid_device_ids_before_running() { + fn kind(result: MlxResult) -> &'static str { + match result { + Ok(_) => "ok", + Err(MlxError::InvalidDeviceId(_)) => "InvalidDeviceId", + Err(MlxError::DryRun(_)) => "DryRun", + Err(_) => "other", + } + } + + let runner = FlintRunner::with_path("flint").with_dry_run(true); + let manager = LockdownManager::with_runner(runner); + + // lock_device validates first, then would dry-run. + check_values( + [ + Check { + scenario: "empty id is rejected before the runner", + input: "", + expect: "InvalidDeviceId", + }, + Check { + scenario: "id with a space is rejected before the runner", + input: "dev 0", + expect: "InvalidDeviceId", + }, + Check { + scenario: "a valid id passes validation and reaches the dry-run", + input: "0000:01:00.0", + expect: "DryRun", + }, + ], + |device_id: &str| kind(manager.lock_device(device_id, "12345678")), + ); + } + + // get_status applies the same up-front device-id validation as the other manager + // methods: empty and space-bearing ids are rejected as InvalidDeviceId before the + // runner is queried, while a valid id reaches the dry-run runner (DryRun). + #[test] + fn get_status_validates_device_id() { + fn kind(result: MlxResult) -> &'static str { + match result { + Ok(_) => "ok", + Err(MlxError::InvalidDeviceId(_)) => "InvalidDeviceId", + Err(MlxError::DryRun(_)) => "DryRun", + Err(_) => "other", + } + } + + let runner = FlintRunner::with_path("flint").with_dry_run(true); + let manager = LockdownManager::with_runner(runner); + + check_values( + [ + Check { + scenario: "empty id", + input: "", + expect: "InvalidDeviceId", + }, + Check { + scenario: "id with a space", + input: "bad id", + expect: "InvalidDeviceId", + }, + Check { + scenario: "valid id reaches the dry-run query", + input: "mlx5_0", + expect: "DryRun", + }, + ], + |device_id: &str| kind(manager.get_status(device_id)), + ); + } +} diff --git a/crates/libmlx/src/lockdown/runner.rs b/crates/libmlx/src/lockdown/runner.rs index e31d0a37a4..7bd78a113b 100644 --- a/crates/libmlx/src/lockdown/runner.rs +++ b/crates/libmlx/src/lockdown/runner.rs @@ -334,3 +334,333 @@ impl Default for FlintRunner { Self::new().unwrap_or_else(|_| Self::with_path("flint")) } } + +#[cfg(test)] +mod coverage_tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + + use super::*; + + // err_kind projects an MlxError onto a stable discriminant string so we can + // distinguish *which* error a deterministic path returns without relying on + // MlxError being PartialEq (it is not). Only the variants the deterministic + // (no-flint-spawn) paths in this file can produce are named here. + fn err_kind(e: &MlxError) -> &'static str { + match e { + MlxError::CommandFailed(_) => "CommandFailed", + MlxError::DeviceNotFound(_) => "DeviceNotFound", + MlxError::InvalidDeviceId(_) => "InvalidDeviceId", + MlxError::AlreadyLocked => "AlreadyLocked", + MlxError::AlreadyUnlocked => "AlreadyUnlocked", + MlxError::InvalidKey => "InvalidKey", + MlxError::PermissionDenied => "PermissionDenied", + MlxError::FlintNotFound => "FlintNotFound", + MlxError::ParseError(_) => "ParseError", + MlxError::DryRun(_) => "DryRun", + MlxError::IoError(_) => "IoError", + MlxError::SerializationError(_) => "SerializationError", + } + } + + // is_valid_key accepts exactly 8 ASCII hex digits; anything else is rejected. + // Covers: too short, too long, exactly-8 valid (upper and lower), non-hex + // chars, empty, and embedded whitespace. + #[test] + fn is_valid_key_requires_eight_hex_digits() { + check_values( + [ + Check { + scenario: "eight lowercase hex digits", + input: "0a1b2c3d", + expect: true, + }, + Check { + scenario: "eight uppercase hex digits", + input: "ABCDEF01", + expect: true, + }, + Check { + scenario: "eight digits, all numeric", + input: "12345678", + expect: true, + }, + Check { + scenario: "seven digits is too short", + input: "1234567", + expect: false, + }, + Check { + scenario: "nine digits is too long", + input: "123456789", + expect: false, + }, + Check { + scenario: "empty string", + input: "", + expect: false, + }, + Check { + scenario: "right length but non-hex char 'g'", + input: "1234567g", + expect: false, + }, + Check { + scenario: "right length but contains a space", + input: "1234 678", + expect: false, + }, + Check { + scenario: "0x-prefixed is not bare hex of length 8", + input: "0x123456", + expect: false, + }, + ], + FlintRunner::is_valid_key, + ); + } + + // validate_device_id rejects empty and space-containing IDs, accepts the rest. + // Errors aren't PartialEq, so the rejection rows use Fails + map_err(drop). + #[test] + fn validate_device_id_rejects_empty_and_spaces() { + check_cases( + [ + Case { + scenario: "a PCI-style address is accepted", + input: "0000:01:00.0", + expect: Yields(()), + }, + Case { + scenario: "a simple device name is accepted", + input: "mlx5_0", + expect: Yields(()), + }, + Case { + scenario: "a device path is accepted", + input: "/dev/mst/mt4119_pciconf0", + expect: Yields(()), + }, + Case { + scenario: "empty is rejected", + input: "", + expect: Fails, + }, + Case { + scenario: "a leading space is rejected", + input: " 01:00.0", + expect: Fails, + }, + Case { + scenario: "an interior space is rejected", + input: "01:00 .0", + expect: Fails, + }, + ], + |id| FlintRunner::validate_device_id(id).map_err(drop), + ); + } + + // validate_device_id's two rejection paths carry distinct InvalidDeviceId + // messages; pin which message each path produces. + #[test] + fn validate_device_id_error_messages() { + let empty = FlintRunner::validate_device_id("").unwrap_err(); + assert_eq!(err_kind(&empty), "InvalidDeviceId"); + assert_eq!( + empty.to_string(), + "Invalid device ID format: Device ID cannot be empty" + ); + + let spaced = FlintRunner::validate_device_id("a b").unwrap_err(); + assert_eq!(err_kind(&spaced), "InvalidDeviceId"); + assert_eq!( + spaced.to_string(), + "Invalid device ID format: Device ID cannot contain spaces" + ); + } + + // build_command joins the path and args with single spaces. Covers a custom + // path, an empty arg slice (trailing space after the path), and a single arg. + #[test] + fn build_command_formats_path_and_args() { + let runner = FlintRunner::with_path("/opt/mellanox/mft/bin/flint"); + check_values( + [ + Check { + scenario: "typical query args", + input: &["-d", "dev0", "q"][..], + expect: "/opt/mellanox/mft/bin/flint -d dev0 q".to_string(), + }, + Check { + scenario: "empty args leaves a trailing space", + input: &[][..], + expect: "/opt/mellanox/mft/bin/flint ".to_string(), + }, + Check { + scenario: "a single arg", + input: &["burn"][..], + expect: "/opt/mellanox/mft/bin/flint burn".to_string(), + }, + ], + |args| runner.build_command(args), + ); + } + + // with_path / with_dry_run are builders; read the private fields back to + // confirm the path is stored verbatim and dry_run defaults off then toggles. + #[test] + fn builders_store_path_and_dry_run() { + let plain = FlintRunner::with_path("flint"); + assert_eq!(plain.flint_path, "flint"); + assert!(!plain.dry_run, "dry_run defaults to false"); + + let dry = FlintRunner::with_path("/usr/bin/flint").with_dry_run(true); + assert_eq!(dry.flint_path, "/usr/bin/flint"); + assert!(dry.dry_run, "with_dry_run(true) enables dry-run"); + + // with_dry_run(false) is the identity for the flag. + let off = FlintRunner::with_path("flint").with_dry_run(false); + assert!(!off.dry_run); + } + + // In dry-run mode the command-issuing methods short-circuit to MlxError::DryRun + // (carrying the would-be command string) before spawning flint, so these are + // deterministic. Key-taking methods validate the key FIRST: an invalid key + // yields InvalidKey even under dry-run; a valid key yields DryRun. + #[test] + fn dry_run_methods_short_circuit() { + let runner = FlintRunner::with_path("flint").with_dry_run(true); + + // query_device has no key validation: always DryRun in dry-run mode. + let q = runner.query_device("dev0").unwrap_err(); + assert_eq!(err_kind(&q), "DryRun"); + assert_eq!( + q.to_string(), + "Dry run - would have executed: flint -d dev0 q" + ); + + // enable_hw_access: valid key -> DryRun with the enable command string. + let en = runner.enable_hw_access("dev0", "0a1b2c3d").unwrap_err(); + assert_eq!(err_kind(&en), "DryRun"); + assert_eq!( + en.to_string(), + "Dry run - would have executed: flint -d dev0 hw_access enable 0a1b2c3d" + ); + + // disable_hw_access: valid key -> DryRun with the disable command string. + let dis = runner.disable_hw_access("dev0", "0a1b2c3d").unwrap_err(); + assert_eq!(err_kind(&dis), "DryRun"); + assert_eq!( + dis.to_string(), + "Dry run - would have executed: flint -d dev0 hw_access disable 0a1b2c3d" + ); + + // set_key: valid key -> DryRun with the set_key command string. + let sk = runner.set_key("dev0", "0a1b2c3d").unwrap_err(); + assert_eq!(err_kind(&sk), "DryRun"); + assert_eq!( + sk.to_string(), + "Dry run - would have executed: flint -d dev0 set_key 0a1b2c3d" + ); + } + + // Key validation precedes the dry-run check in every key-taking method, so an + // invalid key short-circuits to InvalidKey regardless of dry-run state. + #[test] + fn invalid_key_takes_precedence_over_dry_run() { + let runner = FlintRunner::with_path("flint").with_dry_run(true); + + // Each key-taking method, fed a too-short key, must report InvalidKey. + check_values( + [ + Check { + scenario: "enable_hw_access rejects a bad key", + input: "enable", + expect: "InvalidKey", + }, + Check { + scenario: "disable_hw_access rejects a bad key", + input: "disable", + expect: "InvalidKey", + }, + Check { + scenario: "set_key rejects a bad key", + input: "set_key", + expect: "InvalidKey", + }, + ], + |which| { + let bad = "xyz"; // not 8 hex digits + let e = match which { + "enable" => runner.enable_hw_access("dev0", bad).unwrap_err(), + "disable" => runner.disable_hw_access("dev0", bad).unwrap_err(), + "set_key" => runner.set_key("dev0", bad).unwrap_err(), + _ => unreachable!(), + }; + err_kind(&e) + }, + ); + } + + // burn / verify_image check that the image path exists BEFORE any dry-run or + // spawn, returning CommandFailed for a missing image. A nonexistent path is + // deterministic regardless of dry-run, so both methods can be exercised here. + #[test] + fn burn_and_verify_reject_missing_image() { + let runner = FlintRunner::with_path("flint").with_dry_run(true); + let missing = Path::new("/nonexistent/definitely/not/here.bin"); + + let b = runner.burn("dev0", missing).unwrap_err(); + assert_eq!(err_kind(&b), "CommandFailed"); + assert!( + b.to_string().contains("Firmware image does not exist"), + "burn names the missing image: {b}" + ); + + let v = runner.verify_image("dev0", missing).unwrap_err(); + assert_eq!(err_kind(&v), "CommandFailed"); + assert!( + v.to_string().contains("Firmware image does not exist"), + "verify_image names the missing image: {v}" + ); + } + + // When the image exists and dry-run is on, burn/verify short-circuit to DryRun + // with the full command string (including the resolved image path). Creates a + // real temp file so the exists() gate passes deterministically. + #[test] + fn burn_and_verify_dry_run_with_existing_image() { + let runner = FlintRunner::with_path("flint").with_dry_run(true); + + let existing = std::env::temp_dir().join(format!( + "libmlx_runner_cov_{}_{}.bin", + std::process::id(), + line!() + )); + std::fs::write(&existing, b"fw").expect("write temp fixture"); + assert!( + existing.exists(), + "test fixture must exist: {}", + existing.display() + ); + + let img = existing.to_string_lossy().to_string(); + + let b = runner.burn("dev0", &existing).unwrap_err(); + assert_eq!(err_kind(&b), "DryRun"); + assert_eq!( + b.to_string(), + format!("Dry run - would have executed: flint -d dev0 -y -i {img} burn") + ); + + let v = runner.verify_image("dev0", &existing).unwrap_err(); + assert_eq!(err_kind(&v), "DryRun"); + assert_eq!( + v.to_string(), + format!("Dry run - would have executed: flint -d dev0 -i {img} verify") + ); + + let _ = std::fs::remove_file(&existing); + } +} diff --git a/crates/libmlx/src/profile/error.rs b/crates/libmlx/src/profile/error.rs index 8c6c748b3b..f2ac2c0004 100644 --- a/crates/libmlx/src/profile/error.rs +++ b/crates/libmlx/src/profile/error.rs @@ -167,3 +167,267 @@ impl From for MlxProfileError { Self::Io { error } } } + +#[cfg(test)] +mod coverage_tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + + use super::*; + + // discriminant maps an MlxProfileError to a stable &'static str tag so + // tests can assert which variant was produced without needing PartialEq + // (several variants wrap non-PartialEq errors like serde_json::Error). + fn discriminant(error: &MlxProfileError) -> &'static str { + match error { + MlxProfileError::RegistryNotFound { .. } => "RegistryNotFound", + MlxProfileError::VariableNotFound { .. } => "VariableNotFound", + MlxProfileError::ValueValidation { .. } => "ValueValidation", + MlxProfileError::ProfileValidation { .. } => "ProfileValidation", + MlxProfileError::Serialization { .. } => "Serialization", + MlxProfileError::YamlParsing { .. } => "YamlParsing", + MlxProfileError::JsonParsing { .. } => "JsonParsing", + MlxProfileError::TomlParsing { .. } => "TomlParsing", + MlxProfileError::Runner { .. } => "Runner", + MlxProfileError::Io { .. } => "Io", + } + } + + // The constructors each build one specific variant. Project to the + // discriminant so we verify the constructor selects the right arm, + // independent of the (non-PartialEq) payloads. + #[test] + fn constructors_select_the_right_variant() { + check_values( + [ + Check { + scenario: "registry_not_found -> RegistryNotFound", + input: MlxProfileError::registry_not_found("reg"), + expect: "RegistryNotFound", + }, + Check { + scenario: "variable_not_found -> VariableNotFound", + input: MlxProfileError::variable_not_found("var", "reg"), + expect: "VariableNotFound", + }, + Check { + scenario: "value_validation -> ValueValidation", + input: MlxProfileError::value_validation( + "var", + MlxValueError::TypeMismatch { + expected: "Integer".to_string(), + got: "bool".to_string(), + }, + ), + expect: "ValueValidation", + }, + Check { + scenario: "profile_validation -> ProfileValidation", + input: MlxProfileError::profile_validation("bad"), + expect: "ProfileValidation", + }, + Check { + scenario: "serialization -> Serialization", + input: MlxProfileError::serialization("boom"), + expect: "Serialization", + }, + ], + |error| discriminant(&error), + ); + } + + // The Display strings for the hand-built variants are fixed format + // strings, so we can pin them exactly. The ValueValidation row also + // exercises the nested MlxValueError Display ("Type mismatch: ..."). + #[test] + fn constructors_render_their_display_strings() { + check_values( + [ + Check { + scenario: "RegistryNotFound Display", + input: MlxProfileError::registry_not_found("my_registry"), + expect: "Registry 'my_registry' not found in available registries".to_string(), + }, + Check { + scenario: "VariableNotFound Display", + input: MlxProfileError::variable_not_found("my_var", "my_registry"), + expect: "Variable 'my_var' not found in registry 'my_registry'".to_string(), + }, + Check { + scenario: "ValueValidation Display nests the MlxValueError", + input: MlxProfileError::value_validation( + "my_var", + MlxValueError::TypeMismatch { + expected: "Integer".to_string(), + got: "bool".to_string(), + }, + ), + expect: "Value validation failed for variable 'my_var': \ + Type mismatch: expected Integer, got bool" + .to_string(), + }, + Check { + scenario: "ProfileValidation Display", + input: MlxProfileError::profile_validation("something went wrong"), + expect: "Profile validation failed: something went wrong".to_string(), + }, + Check { + scenario: "Serialization Display", + input: MlxProfileError::serialization("could not serialize"), + expect: "Serialization error: could not serialize".to_string(), + }, + ], + |error| error.to_string(), + ); + } + + // The generic Into bounds accept both &str and String; confirm + // both flavors flow through the constructors and land in Display. + #[test] + fn constructors_accept_str_and_string() { + check_values( + [ + Check { + scenario: "registry_not_found from &str", + input: MlxProfileError::registry_not_found("a"), + expect: "Registry 'a' not found in available registries".to_string(), + }, + Check { + scenario: "registry_not_found from String", + input: MlxProfileError::registry_not_found("b".to_string()), + expect: "Registry 'b' not found in available registries".to_string(), + }, + Check { + scenario: "profile_validation from String", + input: MlxProfileError::profile_validation("msg".to_string()), + expect: "Profile validation failed: msg".to_string(), + }, + ], + |error| error.to_string(), + ); + } + + // Each From impl must select its matching variant. We build a real + // wrapped error per source type, convert via Into, and assert the + // resulting discriminant (the wrapped payloads are not PartialEq). + #[test] + fn from_impls_select_the_right_variant() { + check_values( + [ + Check { + scenario: "MlxValueError -> ValueValidation", + input: MlxProfileError::from(MlxValueError::ReadOnlyVariable { + variable_name: "ro".to_string(), + }), + expect: "ValueValidation", + }, + Check { + scenario: "MlxRunnerError -> Runner", + input: MlxProfileError::from(MlxRunnerError::NoDeviceFound), + expect: "Runner", + }, + Check { + scenario: "std::io::Error -> Io", + input: MlxProfileError::from(std::io::Error::other("disk gone")), + expect: "Io", + }, + Check { + scenario: "serde_json::Error -> JsonParsing", + input: MlxProfileError::from( + serde_json::from_str::("not json").unwrap_err(), + ), + expect: "JsonParsing", + }, + Check { + scenario: "serde_yaml::Error -> YamlParsing", + input: MlxProfileError::from( + serde_yaml::from_str::("{ : : invalid").unwrap_err(), + ), + expect: "YamlParsing", + }, + Check { + scenario: "toml::de::Error -> TomlParsing", + input: MlxProfileError::from( + toml::from_str::("= broken").unwrap_err(), + ), + expect: "TomlParsing", + }, + ], + |error| discriminant(&error), + ); + } + + // The From impl tags the variable name as "unknown" and + // nests the inner Display; pin the full rendered string to lock that + // contract in place. + #[test] + fn from_value_error_uses_unknown_variable_name() { + Check { + scenario: "From Display uses 'unknown'", + input: MlxProfileError::from(MlxValueError::PresetOutOfRange { + value: 9, + max_allowed: 5, + }), + expect: "Value validation failed for variable 'unknown': \ + Preset value 9 exceeds maximum 5" + .to_string(), + } + .check(|error| error.to_string()); + } + + // The `?`-style conversion path: a function returning MlxProfileError + // that uses `?` on a fallible toml parse must surface TomlParsing. This + // exercises the From impl through real `?` desugaring. + #[test] + fn question_mark_conversions_propagate_the_right_variant() { + fn parse_toml(raw: &str) -> Result { + let value: toml::Value = toml::from_str(raw)?; + Ok(value) + } + + fn parse_json(raw: &str) -> Result { + let value: serde_json::Value = serde_json::from_str(raw)?; + Ok(value) + } + + check_cases( + [ + Case { + scenario: "valid toml yields", + input: "key = 1", + expect: Yields("TomlParsing-or-ok"), + }, + Case { + scenario: "invalid toml fails as TomlParsing", + input: "= nope", + expect: FailsWith("TomlParsing"), + }, + ], + |raw| { + parse_toml(raw) + .map(|_| "TomlParsing-or-ok") + .map_err(|error| discriminant(&error)) + }, + ); + + check_cases( + [ + Case { + scenario: "valid json yields", + input: "{}", + expect: Yields("JsonParsing-or-ok"), + }, + Case { + scenario: "invalid json fails as JsonParsing", + input: "not json", + expect: FailsWith("JsonParsing"), + }, + ], + |raw| { + parse_json(raw) + .map(|_| "JsonParsing-or-ok") + .map_err(|error| discriminant(&error)) + }, + ); + } +} diff --git a/crates/libmlx/src/profile/profile.rs b/crates/libmlx/src/profile/profile.rs index fcdfe17fb2..e3291beda8 100644 --- a/crates/libmlx/src/profile/profile.rs +++ b/crates/libmlx/src/profile/profile.rs @@ -284,3 +284,444 @@ impl MlxConfigProfile { } } } + +#[cfg(test)] +mod coverage_tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + + use super::*; + use crate::variables::spec::MlxVariableSpec; + use crate::variables::value::MlxValueType; + use crate::variables::variable::MlxConfigVariable; + + // a local registry with a boolean, integer, and enum variable. We avoid the + // global registries on purpose -- those are only needed by the serialization + // round-trips below, which key off the registry *name*. + fn test_registry() -> MlxVariableRegistry { + MlxVariableRegistry::new("test_reg").variables(vec![ + MlxConfigVariable { + name: "BOOL_VAR".to_string(), + description: "a boolean".to_string(), + read_only: false, + spec: MlxVariableSpec::Boolean, + }, + MlxConfigVariable { + name: "INT_VAR".to_string(), + description: "an integer".to_string(), + read_only: false, + spec: MlxVariableSpec::Integer, + }, + MlxConfigVariable { + name: "ENUM_VAR".to_string(), + description: "an enum".to_string(), + read_only: false, + spec: MlxVariableSpec::Enum { + options: vec!["low".to_string(), "high".to_string()], + }, + }, + ]) + } + + // new starts with the given name, no description, and an empty config. This + // pins each field of a freshly-created profile. + #[test] + fn new_initializes_empty_profile() { + let profile = MlxConfigProfile::new("p1", test_registry()); + assert_eq!(profile.name, "p1"); + assert_eq!(profile.description, None); + assert_eq!(profile.variable_count(), 0); + assert!(profile.variable_names().is_empty()); + assert_eq!(profile.registry.name, "test_reg"); + } + + // with_description sets the optional description; absent vs present are the two + // states the rest of the type branches on (notably `summary`). + #[test] + fn with_description_sets_description() { + let profile = MlxConfigProfile::new("p1", test_registry()).with_description("hello"); + assert_eq!(profile.description, Some("hello".to_string())); + } + + // `with` walks the registry-lookup + value-validation paths: a known variable + // with a valid value succeeds; an unknown variable name fails (VariableNotFound); + // a known variable with a value that violates its spec fails (ValueValidation). + // MlxProfileError isn't PartialEq, so failures use `Fails` + map_err(drop). On + // success we project to the configured value name. + #[test] + fn with_validates_name_and_value() { + check_cases( + [ + Case { + scenario: "known boolean variable, valid value", + input: ("BOOL_VAR", "true"), + expect: Yields("BOOL_VAR".to_string()), + }, + Case { + scenario: "unknown variable name is rejected", + input: ("NOPE", "true"), + expect: Fails, + }, + Case { + scenario: "known boolean variable, unparseable value fails validation", + input: ("BOOL_VAR", "maybe"), + expect: Fails, + }, + ], + |(name, value)| { + MlxConfigProfile::new("p", test_registry()) + .with(name, value) + .map(|p| p.get_variable(name).unwrap().name().to_string()) + .map_err(drop) + }, + ); + } + + // `with` on an enum variable: an allowed option succeeds; a disallowed option + // fails. The exact error is ValueValidation wrapping the enum rejection -- we + // just assert failure here, since the inner MlxValueError is exercised in the + // variables tests. + #[test] + fn with_enum_validation_path() { + check_cases( + [ + Case { + scenario: "valid enum option", + input: "high", + expect: Yields(MlxValueType::Enum("high".to_string())), + }, + Case { + scenario: "invalid enum option fails value validation", + input: "middle", + expect: Fails, + }, + ], + |opt| { + MlxConfigProfile::new("p", test_registry()) + .with("ENUM_VAR", opt) + .map(|p| p.get_variable("ENUM_VAR").unwrap().value.clone()) + .map_err(drop) + }, + ); + } + + // with_value takes a pre-built MlxConfigValue. A value whose variable lives in + // the registry is accepted; a value whose variable is foreign to the registry + // is rejected with VariableNotFound. We build the foreign value against its own + // one-variable registry so it is internally valid but absent from `test_reg`. + #[test] + fn with_value_checks_registry_membership() { + let known_value = { + let reg = test_registry(); + reg.get_variable("INT_VAR").unwrap().with(7i64).unwrap() + }; + let foreign_value = { + let other = MlxVariableRegistry::new("other").add_variable(MlxConfigVariable { + name: "FOREIGN".to_string(), + description: "x".to_string(), + read_only: false, + spec: MlxVariableSpec::Integer, + }); + other.get_variable("FOREIGN").unwrap().with(1i64).unwrap() + }; + + check_cases( + [ + Case { + scenario: "value for a known variable is accepted", + input: known_value, + expect: Yields("INT_VAR".to_string()), + }, + Case { + scenario: "value for a foreign variable is rejected", + input: foreign_value, + expect: Fails, + }, + ], + |value| { + let name = value.name().to_string(); + MlxConfigProfile::new("p", test_registry()) + .with_value(value) + .map(|p| p.get_variable(&name).unwrap().name().to_string()) + .map_err(drop) + }, + ); + } + + // add_config_value (driven through `with`) dedups by name: re-adding the same + // variable replaces it in place rather than appending, so the count stays put + // and the stored value reflects the latest write. A second, distinct variable + // does append. Each row reports (variable_count, INT_VAR value). + #[test] + fn adding_same_variable_replaces_in_place() { + check_values( + [ + Check { + scenario: "single write", + input: vec![("INT_VAR", 1i64)], + expect: (1usize, Some(MlxValueType::Integer(1))), + }, + Check { + scenario: "re-write same variable replaces, count stays 1", + input: vec![("INT_VAR", 1i64), ("INT_VAR", 2i64)], + expect: (1usize, Some(MlxValueType::Integer(2))), + }, + Check { + scenario: "distinct second variable appends", + input: vec![("INT_VAR", 5i64), ("INT_VAR", 9i64)], + expect: (1usize, Some(MlxValueType::Integer(9))), + }, + ], + |writes| { + let mut profile = MlxConfigProfile::new("p", test_registry()); + for (name, value) in writes { + profile = profile.with(name, value).unwrap(); + } + ( + profile.variable_count(), + profile.get_variable("INT_VAR").map(|cv| cv.value.clone()), + ) + }, + ); + } + + // a profile with two distinct variables keeps both, in insertion order, and + // variable_count reflects the total. + #[test] + fn distinct_variables_accumulate() { + let profile = MlxConfigProfile::new("p", test_registry()) + .with("BOOL_VAR", true) + .unwrap() + .with("INT_VAR", 42i64) + .unwrap(); + assert_eq!(profile.variable_count(), 2); + assert_eq!(profile.variable_names(), vec!["BOOL_VAR", "INT_VAR"]); + } + + // get_variable returns Some for a configured variable and None otherwise. We + // configure exactly BOOL_VAR, then probe present / unconfigured / nonexistent. + #[test] + fn get_variable_present_and_absent() { + let profile = MlxConfigProfile::new("p", test_registry()) + .with("BOOL_VAR", true) + .unwrap(); + check_values( + [ + Check { + scenario: "configured variable is found", + input: "BOOL_VAR", + expect: true, + }, + Check { + scenario: "known-but-unconfigured variable is absent", + input: "INT_VAR", + expect: false, + }, + Check { + scenario: "nonexistent variable is absent", + input: "MISSING", + expect: false, + }, + ], + |name| profile.get_variable(name).is_some(), + ); + } + + // validate rejects an empty profile (ProfileValidation) and accepts a profile + // whose values were all built through `with` (each validates against its spec). + #[test] + fn validate_empty_vs_populated() { + check_cases( + [ + Case { + scenario: "empty profile is rejected", + input: false, + expect: Fails, + }, + Case { + scenario: "populated profile validates", + input: true, + expect: Yields(()), + }, + ], + |populate| { + let mut profile = MlxConfigProfile::new("p", test_registry()); + if populate { + profile = profile.with("BOOL_VAR", true).unwrap(); + } + profile.validate().map_err(drop) + }, + ); + } + + // compare/sync validate the profile first, so an empty profile fails before any + // runner work. These exercise the early-return validation branch without + // touching a real device. + #[test] + fn compare_and_sync_reject_empty_profile() { + let empty = MlxConfigProfile::new("p", test_registry()); + assert!(empty.compare("dev", None).is_err()); + assert!(empty.sync("dev", None).is_err()); + } + + // summary picks between the with-description and without-description formats. + // Both exact strings are derived from the format! literals in `summary`. + #[test] + fn summary_formats_with_and_without_description() { + check_values( + [ + Check { + scenario: "no description", + input: None, + expect: "Profile 'p': 1 variables for registry 'test_reg'".to_string(), + }, + Check { + scenario: "with description", + input: Some("desc"), + expect: "Profile 'p': desc - 1 variables for registry 'test_reg'".to_string(), + }, + ], + |desc: Option<&str>| { + let mut profile = MlxConfigProfile::new("p", test_registry()); + if let Some(d) = desc { + profile = profile.with_description(d); + } + profile = profile.with("BOOL_VAR", true).unwrap(); + profile.summary() + }, + ); + } + + // from_yaml / from_json resolve the registry by *name* against the global + // registry set. An unknown registry name fails with RegistryNotFound; + // malformed YAML/JSON fails at the parse stage. We only assert that each bad + // input fails, since the wrapped parser error isn't PartialEq. + #[test] + fn from_yaml_and_json_reject_bad_input() { + check_cases( + [ + Case { + scenario: "yaml: unknown registry", + input: "name: p\nregistry_name: does_not_exist\nconfig: {}\n", + expect: Fails, + }, + Case { + scenario: "yaml: malformed document", + input: "name: [unterminated", + expect: Fails, + }, + ], + |yaml| MlxConfigProfile::from_yaml(yaml).map(|_| ()).map_err(drop), + ); + check_cases( + [ + Case { + scenario: "json: unknown registry", + input: r#"{"name":"p","registry_name":"does_not_exist","config":{}}"#, + expect: Fails, + }, + Case { + scenario: "json: malformed document", + input: "{not json", + expect: Fails, + }, + ], + |json| MlxConfigProfile::from_json(json).map(|_| ()).map_err(drop), + ); + } + + // a profile built on the real `mlx_generic` registry round-trips through YAML + // and JSON: serialize, deserialize, and the configured values come back intact. + // We project to (variable_count, SRIOV_EN value, NUM_OF_VFS value) since the + // profile type isn't PartialEq and config ordering isn't guaranteed. + #[test] + fn yaml_json_round_trip_preserves_values() { + let registry = crate::registry::registries::get("mlx_generic") + .expect("mlx_generic registry exists") + .clone(); + let profile = MlxConfigProfile::new("rt", registry) + .with_description("round trip") + .with("SRIOV_EN", true) + .unwrap() + .with("NUM_OF_VFS", 8i64) + .unwrap(); + + let yaml = profile.to_yaml().unwrap(); + let from_yaml = MlxConfigProfile::from_yaml(&yaml).unwrap(); + assert_eq!(from_yaml.variable_count(), 2); + assert_eq!( + from_yaml + .get_variable("SRIOV_EN") + .map(|cv| cv.value.clone()), + Some(MlxValueType::Boolean(true)) + ); + assert_eq!( + from_yaml + .get_variable("NUM_OF_VFS") + .map(|cv| cv.value.clone()), + Some(MlxValueType::Integer(8)) + ); + assert_eq!(from_yaml.description, Some("round trip".to_string())); + + let json = profile.to_json().unwrap(); + let from_json = MlxConfigProfile::from_json(&json).unwrap(); + assert_eq!(from_json.variable_count(), 2); + assert_eq!( + from_json + .get_variable("SRIOV_EN") + .map(|cv| cv.value.clone()), + Some(MlxValueType::Boolean(true)) + ); + assert_eq!( + from_json + .get_variable("NUM_OF_VFS") + .map(|cv| cv.value.clone()), + Some(MlxValueType::Integer(8)) + ); + } + + // from_yaml_file / from_json_file surface I/O errors for a missing path before + // any parsing happens. + #[test] + fn from_file_missing_path_errors() { + assert!(MlxConfigProfile::from_yaml_file("/nonexistent/path/profile.yaml").is_err()); + assert!(MlxConfigProfile::from_json_file("/nonexistent/path/profile.json").is_err()); + } + + // to_yaml_file / to_json_file write a serialized profile that can be read back + // through the file loaders, closing the file round-trip loop. + #[test] + fn file_round_trip_via_temp_dir() { + let registry = crate::registry::registries::get("mlx_generic") + .expect("mlx_generic registry exists") + .clone(); + let profile = MlxConfigProfile::new("file_rt", registry) + .with("SRIOV_EN", false) + .unwrap(); + + let dir = std::env::temp_dir(); + let yaml_path = dir.join(format!("libmlx_profile_{}.yaml", std::process::id())); + let json_path = dir.join(format!("libmlx_profile_{}.json", std::process::id())); + + profile.to_yaml_file(&yaml_path).unwrap(); + let loaded_yaml = MlxConfigProfile::from_yaml_file(&yaml_path).unwrap(); + assert_eq!( + loaded_yaml + .get_variable("SRIOV_EN") + .map(|cv| cv.value.clone()), + Some(MlxValueType::Boolean(false)) + ); + + profile.to_json_file(&json_path).unwrap(); + let loaded_json = MlxConfigProfile::from_json_file(&json_path).unwrap(); + assert_eq!( + loaded_json + .get_variable("SRIOV_EN") + .map(|cv| cv.value.clone()), + Some(MlxValueType::Boolean(false)) + ); + + let _ = std::fs::remove_file(&yaml_path); + let _ = std::fs::remove_file(&json_path); + } +} diff --git a/crates/libmlx/src/runner/json_parser.rs b/crates/libmlx/src/runner/json_parser.rs index 786a845990..ea7b7c6d79 100644 --- a/crates/libmlx/src/runner/json_parser.rs +++ b/crates/libmlx/src/runner/json_parser.rs @@ -589,3 +589,777 @@ impl<'a> JsonResponseParser<'a> { crate::runner::traits::get_array_size_from_spec(spec) } } + +#[cfg(test)] +mod coverage_tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + use serde_json::json; + + use super::*; + use crate::variables::value::MlxValueType; + use crate::variables::variable::MlxConfigVariable; + + // Builds a JsonVariable where every field (current/default/next) holds the + // same JSON value -- enough for the per-field parsing helpers, which only + // read one field at a time. + fn json_var(value: serde_json::Value) -> JsonVariable { + JsonVariable { + current_value: value.clone(), + default_value: value.clone(), + modified: false, + next_value: value, + read_only: false, + } + } + + // Builds a JsonVariable with distinct current/default/next values plus the + // modified/read_only flags, for exercising field selection and flag rollup. + fn json_var_full( + current: serde_json::Value, + default: serde_json::Value, + next: serde_json::Value, + modified: bool, + read_only: bool, + ) -> JsonVariable { + JsonVariable { + current_value: current, + default_value: default, + modified, + next_value: next, + read_only, + } + } + + fn var(name: &str, spec: MlxVariableSpec) -> MlxConfigVariable { + MlxConfigVariable { + name: name.to_string(), + description: format!("desc {name}"), + read_only: false, + spec, + } + } + + fn registry_with(vars: Vec) -> MlxVariableRegistry { + MlxVariableRegistry { + name: "test".to_string(), + variables: vars, + filters: None, + } + } + + // parse_bool_from_json: TRUE(n)/FALSE(n) get their parenthetical stripped and + // are matched case-insensitively; bare true/false work; anything else (or a + // non-string JSON value) is rejected. MlxRunnerError isn't PartialEq, so error + // rows drop the error and assert only that it fails. + #[test] + fn parse_bool_from_json_covers_every_branch() { + let parser = JsonResponseParser { + registry: ®istry_with(vec![]), + options: &ExecOptions::default(), + }; + + check_cases( + [ + Case { + scenario: "TRUE(1) strips the parenthetical", + input: json!("TRUE(1)"), + expect: Yields(true), + }, + Case { + scenario: "FALSE(0) strips the parenthetical", + input: json!("FALSE(0)"), + expect: Yields(false), + }, + Case { + scenario: "bare true", + input: json!("true"), + expect: Yields(true), + }, + Case { + scenario: "bare false", + input: json!("false"), + expect: Yields(false), + }, + Case { + scenario: "mixed case True", + input: json!("True"), + expect: Yields(true), + }, + Case { + scenario: "unrecognized string is rejected", + input: json!("maybe"), + expect: Fails, + }, + Case { + scenario: "a number is not a boolean string", + input: json!(1), + expect: Fails, + }, + Case { + scenario: "a JSON bool is still rejected (must be a string)", + input: json!(true), + expect: Fails, + }, + ], + |value| parser.parse_bool_from_json(&value).map_err(drop), + ); + } + + // parse_int_from_json: only a JSON integer number yields an i64; a float, a + // string, or any non-number fails. + #[test] + fn parse_int_from_json_covers_every_branch() { + let parser = JsonResponseParser { + registry: ®istry_with(vec![]), + options: &ExecOptions::default(), + }; + + check_cases( + [ + Case { + scenario: "positive integer", + input: json!(42), + expect: Yields(42), + }, + Case { + scenario: "negative integer", + input: json!(-7), + expect: Yields(-7), + }, + Case { + scenario: "zero", + input: json!(0), + expect: Yields(0), + }, + Case { + scenario: "a float has no i64 representation", + input: json!(1.5), + expect: Fails, + }, + Case { + scenario: "a numeric string is not a JSON number", + input: json!("42"), + expect: Fails, + }, + ], + |value| parser.parse_int_from_json(&value).map_err(drop), + ); + } + + // parse_string_from_json: keeps everything before the first '(' when a + // parenthetical is present; otherwise returns the string unchanged (no + // trimming here). Non-string JSON is rejected. + #[test] + fn parse_string_from_json_covers_every_branch() { + let parser = JsonResponseParser { + registry: ®istry_with(vec![]), + options: &ExecOptions::default(), + }; + + check_cases( + [ + Case { + scenario: "parenthetical is stripped", + input: json!("ENUM_VALUE(1)"), + expect: Yields("ENUM_VALUE".to_string()), + }, + Case { + scenario: "plain string is unchanged", + input: json!("plain"), + expect: Yields("plain".to_string()), + }, + Case { + scenario: "only the first '(' splits", + input: json!("a(b(c"), + expect: Yields("a".to_string()), + }, + Case { + scenario: "surrounding whitespace is NOT trimmed here", + input: json!(" spaced "), + expect: Yields(" spaced ".to_string()), + }, + Case { + scenario: "a number is rejected", + input: json!(5), + expect: Fails, + }, + ], + |value| parser.parse_string_from_json(&value).map_err(drop), + ); + } + + // parse_hex_from_json: decodes hex with or without an 0x/0X prefix; invalid + // hex (odd length, non-hex chars) and non-string JSON fail. + #[test] + fn parse_hex_from_json_covers_every_branch() { + let parser = JsonResponseParser { + registry: ®istry_with(vec![]), + options: &ExecOptions::default(), + }; + + check_cases( + [ + Case { + scenario: "0x prefix", + input: json!("0x1a2b3c"), + expect: Yields(vec![0x1a, 0x2b, 0x3c]), + }, + Case { + scenario: "0X prefix", + input: json!("0X1A2B"), + expect: Yields(vec![0x1a, 0x2b]), + }, + Case { + scenario: "bare hex", + input: json!("1a2b"), + expect: Yields(vec![0x1a, 0x2b]), + }, + Case { + scenario: "empty string decodes to empty bytes", + input: json!(""), + expect: Yields(vec![]), + }, + Case { + scenario: "non-hex characters are rejected", + input: json!("not_hex"), + expect: Fails, + }, + Case { + scenario: "odd length is rejected", + input: json!("1a2"), + expect: Fails, + }, + Case { + scenario: "a number is rejected", + input: json!(123), + expect: Fails, + }, + ], + |value| parser.parse_hex_from_json(&value).map_err(drop), + ); + } + + // get_json_field_value selects the right field of a JsonVariable per the + // JsonValueField; it never fails. The JsonVariable carries distinct values so + // each arm is distinguishable. + #[test] + fn get_json_field_value_selects_the_right_field() { + let parser = JsonResponseParser { + registry: ®istry_with(vec![]), + options: &ExecOptions::default(), + }; + let jv = json_var_full(json!("cur"), json!("def"), json!("nxt"), false, false); + + check_cases( + [ + Case { + scenario: "Current", + input: JsonValueField::Current, + expect: Yields(json!("cur")), + }, + Case { + scenario: "Default", + input: JsonValueField::Default, + expect: Yields(json!("def")), + }, + Case { + scenario: "Next", + input: JsonValueField::Next, + expect: Yields(json!("nxt")), + }, + ], + |field| { + parser + .get_json_field_value(&jv, field) + .cloned() + .map_err(drop) + }, + ); + } + + // get_array_size delegates to the spec helper: array specs yield their size; + // every scalar/untyped spec is rejected. + #[test] + fn get_array_size_covers_array_and_non_array_specs() { + let parser = JsonResponseParser { + registry: ®istry_with(vec![]), + options: &ExecOptions::default(), + }; + + check_cases( + [ + Case { + scenario: "boolean array", + input: MlxVariableSpec::BooleanArray { size: 4 }, + expect: Yields(4), + }, + Case { + scenario: "integer array", + input: MlxVariableSpec::IntegerArray { size: 2 }, + expect: Yields(2), + }, + Case { + scenario: "binary array", + input: MlxVariableSpec::BinaryArray { size: 7 }, + expect: Yields(7), + }, + Case { + scenario: "enum array", + input: MlxVariableSpec::EnumArray { + options: vec!["a".to_string()], + size: 3, + }, + expect: Yields(3), + }, + Case { + scenario: "scalar boolean has no size", + input: MlxVariableSpec::Boolean, + expect: Fails, + }, + Case { + scenario: "untyped array has no size", + input: MlxVariableSpec::Array, + expect: Fails, + }, + ], + |spec| parser.get_array_size(&spec).map_err(drop), + ); + } + + // json_value_to_config_value: a JSON string routes through the string spec + // conversion (note `with` trims and validates); a JSON number routes through + // integer conversion; bool/null/array are rejected outright as neither string + // nor number. + #[test] + fn json_value_to_config_value_covers_string_number_and_rejects() { + let parser = JsonResponseParser { + registry: ®istry_with(vec![]), + options: &ExecOptions::default(), + }; + let str_var = var("S", MlxVariableSpec::String); + let int_var = var("I", MlxVariableSpec::Integer); + let enum_var = var( + "E", + MlxVariableSpec::Enum { + options: vec!["medium".to_string()], + }, + ); + + // String spec: parse_string keeps the raw text, then `with` trims it. + check_cases( + [ + Case { + scenario: "plain string stored (trimmed by `with`)", + input: json!(" hi "), + expect: Yields(MlxValueType::String("hi".to_string())), + }, + Case { + scenario: "parenthetical stripped before `with`", + input: json!("FOO(2)"), + expect: Yields(MlxValueType::String("FOO".to_string())), + }, + Case { + scenario: "a JSON bool is neither string nor number", + input: json!(true), + expect: Fails, + }, + Case { + scenario: "JSON null is rejected", + input: json!(null), + expect: Fails, + }, + Case { + scenario: "a JSON array is rejected", + input: json!(["a", "b"]), + expect: Fails, + }, + ], + |value| { + parser + .json_value_to_config_value(&str_var, &value) + .map(|v| v.value) + .map_err(drop) + }, + ); + + // Number spec: routes through parse_int + with(i64). + Case { + scenario: "number into an integer variable", + input: json!(99), + expect: Yields(MlxValueType::Integer(99)), + } + .check(|value| { + parser + .json_value_to_config_value(&int_var, &value) + .map(|v| v.value) + .map_err(drop) + }); + + // Enum spec with a parenthetical string: stripped, then validated. + Case { + scenario: "enum string validates after stripping parenthetical", + input: json!("medium(3)"), + expect: Yields(MlxValueType::Enum("medium".to_string())), + } + .check(|value| { + parser + .json_value_to_config_value(&enum_var, &value) + .map(|v| v.value) + .map_err(drop) + }); + + // Enum spec where the stripped value isn't an allowed option -> `with` fails. + Case { + scenario: "enum string not in options is rejected", + input: json!("bogus"), + expect: Fails, + } + .check(|value| { + parser + .json_value_to_config_value(&enum_var, &value) + .map(|v| v.value) + .map_err(drop) + }); + } + + // build_typed_sparse_array fills present indices with Some(parsed) and missing + // indices with None, in ascending order. Uses parse_int_from_json so the value + // type is i64. + #[test] + fn build_typed_sparse_array_fills_present_and_missing() { + let parser = JsonResponseParser { + registry: ®istry_with(vec![]), + options: &ExecOptions::default(), + }; + + // ARR[0] and ARR[2] present, ARR[1] missing, size 3. + let mut json_vars = HashMap::new(); + json_vars.insert("ARR[0]".to_string(), json_var(json!(10))); + json_vars.insert("ARR[2]".to_string(), json_var(json!(30))); + + let result = parser + .build_typed_sparse_array(&json_vars, "ARR", 3, JsonValueField::Current, |jv| { + parser.parse_int_from_json(jv) + }) + .expect("ints parse cleanly"); + assert_eq!(result, vec![Some(10), None, Some(30)]); + + // size 0 yields an empty sparse array regardless of present keys. + let empty = parser + .build_typed_sparse_array(&json_vars, "ARR", 0, JsonValueField::Current, |jv| { + parser.parse_int_from_json(jv) + }) + .expect("size 0 is fine"); + assert_eq!(empty, Vec::>::new()); + + // A present index whose value can't be parsed propagates the error. + let mut bad = HashMap::new(); + bad.insert("ARR[0]".to_string(), json_var(json!("not_an_int"))); + let err = parser.build_typed_sparse_array(&bad, "ARR", 1, JsonValueField::Current, |jv| { + parser.parse_int_from_json(jv) + }); + assert!(err.is_err()); + } + + // build_sparse_array_from_json dispatches on the array spec variant and builds + // a correctly-sized MlxValueType; a non-array spec is rejected. Each row gives + // a fully-populated set of indices so size validation in `with` passes. + #[test] + fn build_sparse_array_from_json_covers_each_array_variant() { + let parser = JsonResponseParser { + registry: ®istry_with(vec![]), + options: &ExecOptions::default(), + }; + + // Boolean array, size 2, both indices present. + let bool_var = var("BA", MlxVariableSpec::BooleanArray { size: 2 }); + let mut bool_vars = HashMap::new(); + bool_vars.insert("BA[0]".to_string(), json_var(json!("TRUE(1)"))); + bool_vars.insert("BA[1]".to_string(), json_var(json!("FALSE(0)"))); + let bool_result = parser + .build_sparse_array_from_json(&bool_var, &bool_vars, "BA", 2, JsonValueField::Current) + .expect("boolean array builds"); + assert_eq!( + bool_result.value, + MlxValueType::BooleanArray(vec![Some(true), Some(false)]) + ); + + // Integer array, size 3, one gap -> None in the middle. + let int_var = var("IA", MlxVariableSpec::IntegerArray { size: 3 }); + let mut int_vars = HashMap::new(); + int_vars.insert("IA[0]".to_string(), json_var(json!(1))); + int_vars.insert("IA[2]".to_string(), json_var(json!(3))); + let int_result = parser + .build_sparse_array_from_json(&int_var, &int_vars, "IA", 3, JsonValueField::Current) + .expect("integer array builds"); + assert_eq!( + int_result.value, + MlxValueType::IntegerArray(vec![Some(1), None, Some(3)]) + ); + + // Enum array, size 2, parenthetical stripped then validated. + let enum_var = var( + "EA", + MlxVariableSpec::EnumArray { + options: vec!["in".to_string(), "out".to_string()], + size: 2, + }, + ); + let mut enum_vars = HashMap::new(); + enum_vars.insert("EA[0]".to_string(), json_var(json!("in(0)"))); + enum_vars.insert("EA[1]".to_string(), json_var(json!("out(1)"))); + let enum_result = parser + .build_sparse_array_from_json(&enum_var, &enum_vars, "EA", 2, JsonValueField::Current) + .expect("enum array builds"); + assert_eq!( + enum_result.value, + MlxValueType::EnumArray(vec![Some("in".to_string()), Some("out".to_string())]) + ); + + // Binary array, size 2, hex decoded. + let bin_var = var("BIN", MlxVariableSpec::BinaryArray { size: 2 }); + let mut bin_vars = HashMap::new(); + bin_vars.insert("BIN[0]".to_string(), json_var(json!("0x1a2b"))); + bin_vars.insert("BIN[1]".to_string(), json_var(json!("3c4d"))); + let bin_result = parser + .build_sparse_array_from_json(&bin_var, &bin_vars, "BIN", 2, JsonValueField::Current) + .expect("binary array builds"); + assert_eq!( + bin_result.value, + MlxValueType::BinaryArray(vec![Some(vec![0x1a, 0x2b]), Some(vec![0x3c, 0x4d])]) + ); + + // A non-array spec hits the catch-all error arm. + let scalar_var = var("SC", MlxVariableSpec::Boolean); + let scalar_err = parser.build_sparse_array_from_json( + &scalar_var, + &HashMap::new(), + "SC", + 1, + JsonValueField::Current, + ); + assert!(scalar_err.is_err()); + + // An enum array element that isn't an allowed option -> `with` rejects. + let mut bad_enum_vars = HashMap::new(); + bad_enum_vars.insert("EA[0]".to_string(), json_var(json!("nope"))); + bad_enum_vars.insert("EA[1]".to_string(), json_var(json!("out"))); + let bad_enum = parser.build_sparse_array_from_json( + &enum_var, + &bad_enum_vars, + "EA", + 2, + JsonValueField::Current, + ); + assert!(bad_enum.is_err()); + } + + // parse_single_variable converts current/default/next and carries the + // modified/read_only flags straight through. + #[test] + fn parse_single_variable_populates_all_fields() { + let parser = JsonResponseParser { + registry: ®istry_with(vec![]), + options: &ExecOptions::default(), + }; + let int_var = var("I", MlxVariableSpec::Integer); + let jv = json_var_full(json!(1), json!(2), json!(3), true, true); + + let result = parser + .parse_single_variable(&int_var, &jv) + .expect("single var parses"); + assert_eq!(result.current_value.value, MlxValueType::Integer(1)); + assert_eq!(result.default_value.value, MlxValueType::Integer(2)); + assert_eq!(result.next_value.value, MlxValueType::Integer(3)); + assert!(result.modified); + assert!(result.read_only); + + // A field that can't convert (bool JSON into an integer spec) fails. + let bad = json_var_full(json!(true), json!(2), json!(3), false, false); + assert!(parser.parse_single_variable(&int_var, &bad).is_err()); + } + + // parse_array_variable reconstructs an array across indices and rolls up the + // modified/read_only flags if ANY index has them set. + #[test] + fn parse_array_variable_reconstructs_and_rolls_up_flags() { + let parser = JsonResponseParser { + registry: ®istry_with(vec![]), + options: &ExecOptions::default(), + }; + let int_array = var("IA", MlxVariableSpec::IntegerArray { size: 3 }); + + let mut json_vars = HashMap::new(); + // index 0: not modified, not read-only + json_vars.insert( + "IA[0]".to_string(), + json_var_full(json!(10), json!(0), json!(10), false, false), + ); + // index 2: modified set here -> should roll up to true; index 1 missing. + json_vars.insert( + "IA[2]".to_string(), + json_var_full(json!(30), json!(0), json!(30), true, false), + ); + + let result = parser + .parse_array_variable(&int_array, &json_vars, "IA") + .expect("array var parses"); + assert_eq!( + result.current_value.value, + MlxValueType::IntegerArray(vec![Some(10), None, Some(30)]) + ); + assert!(result.modified, "modified rolls up from any index"); + assert!(!result.read_only, "no index was read-only"); + + // A non-array variable spec makes get_array_size fail. + let scalar = var("SC", MlxVariableSpec::Boolean); + assert!( + parser + .parse_array_variable(&scalar, &HashMap::new(), "SC") + .is_err() + ); + } + + // parse_variables walks the tlv map: scalar vars resolve by name; array + // indices resolve once per base name (dedup); unknown names are skipped. + #[test] + fn parse_variables_handles_scalars_arrays_and_unknowns() { + let scalar = var("FOO", MlxVariableSpec::Integer); + let array = var("ARR", MlxVariableSpec::IntegerArray { size: 2 }); + let registry = registry_with(vec![scalar, array]); + let parser = JsonResponseParser { + registry: ®istry, + options: &ExecOptions::default(), + }; + + let mut json_vars = HashMap::new(); + json_vars.insert("FOO".to_string(), json_var(json!(7))); + json_vars.insert("ARR[0]".to_string(), json_var(json!(1))); + json_vars.insert("ARR[1]".to_string(), json_var(json!(2))); + // Unknown scalar -> skipped (registry has no UNKNOWN). + json_vars.insert("UNKNOWN".to_string(), json_var(json!(99))); + // Unknown array base -> skipped. + json_vars.insert("MYSTERY[0]".to_string(), json_var(json!(5))); + + let result = parser.parse_variables(&json_vars).expect("parses"); + + // Exactly FOO and ARR are produced (order is map-dependent, so sort names). + let mut names: Vec<&str> = result.iter().map(|v| v.name()).collect(); + names.sort_unstable(); + assert_eq!(names, vec!["ARR", "FOO"]); + + // The ARR entry was reconstructed from both indices, just once. + let arr_entry = result.iter().find(|v| v.name() == "ARR").unwrap(); + assert_eq!( + arr_entry.current_value.value, + MlxValueType::IntegerArray(vec![Some(1), Some(2)]) + ); + + // An invalid array index syntax (non-numeric) propagates an error. + let mut bad = HashMap::new(); + bad.insert("ARR[x]".to_string(), json_var(json!(1))); + assert!(parser.parse_variables(&bad).is_err()); + } + + // parse_variables yields nothing when none of the tlv names are registered. + #[test] + fn parse_variables_skips_when_nothing_registered() { + let registry = registry_with(vec![]); + let parser = JsonResponseParser { + registry: ®istry, + options: &ExecOptions::default(), + }; + let mut json_vars = HashMap::new(); + json_vars.insert("FOO".to_string(), json_var(json!(7))); + + check_values( + [Check { + scenario: "no registered variables -> empty result", + input: json_vars, + expect: 0usize, + }], + |jv| parser.parse_variables(&jv).expect("parses").len(), + ); + } + + // parse_json_response: a well-formed file with a matching device parses; a + // device mismatch, malformed JSON, and a missing file all fail. + #[test] + fn parse_json_response_end_to_end_and_error_paths() { + use std::io::Write; + + let foo = var("FOO", MlxVariableSpec::Integer); + let registry = registry_with(vec![foo]); + let parser = JsonResponseParser { + registry: ®istry, + options: &ExecOptions::default(), + }; + + let good_json = r#"{ + "Device #1": { + "description": "Test Card", + "device": "/dev/mst/mt4129_pciconf0", + "device_type": "ConnectX7", + "name": "MCX755106AS", + "tlv_configuration": { + "FOO": { + "current_value": 7, + "default_value": 0, + "modified": true, + "next_value": 7, + "read_only": false + } + } + } + }"#; + + // Happy path: device matches, FOO is parsed. + let mut good = tempfile::NamedTempFile::new().unwrap(); + good.write_all(good_json.as_bytes()).unwrap(); + let result = parser + .parse_json_response(good.path(), "/dev/mst/mt4129_pciconf0") + .expect("parses"); + assert_eq!( + result.device_info.device_id.as_deref(), + Some("/dev/mst/mt4129_pciconf0") + ); + assert_eq!( + result.device_info.part_number.as_deref(), + Some("MCX755106AS") + ); + assert_eq!(result.variables.len(), 1); + assert_eq!( + result.variables[0].current_value.value, + MlxValueType::Integer(7) + ); + + // Device mismatch path. + let mut mismatch = tempfile::NamedTempFile::new().unwrap(); + mismatch.write_all(good_json.as_bytes()).unwrap(); + assert!( + parser + .parse_json_response(mismatch.path(), "/dev/some/other/device") + .is_err() + ); + + // Malformed JSON path. + let mut bad = tempfile::NamedTempFile::new().unwrap(); + bad.write_all(b"{ not valid json").unwrap(); + assert!( + parser + .parse_json_response(bad.path(), "/dev/mst/mt4129_pciconf0") + .is_err() + ); + + // Missing file path (temp_file_error). + assert!( + parser + .parse_json_response( + std::path::Path::new("/nonexistent/path/to/file.json"), + "/dev/mst/mt4129_pciconf0" + ) + .is_err() + ); + } +} diff --git a/crates/libmlx/src/runner/result_types.rs b/crates/libmlx/src/runner/result_types.rs index ebab0b1f88..a8faffa15b 100644 --- a/crates/libmlx/src/runner/result_types.rs +++ b/crates/libmlx/src/runner/result_types.rs @@ -521,3 +521,723 @@ impl TryFrom for SyncResult { }) } } + +#[cfg(test)] +mod coverage_tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + + use super::*; + use crate::variables::spec::MlxVariableSpec; + use crate::variables::value::MlxValueType; + + // ----- small constructors so each table row reads as data, not setup ----- + + // int_var builds an Integer-spec variable with the given name. read_only + // and description are fixed so rows that exercise them have a known answer. + fn int_var(name: &str) -> MlxConfigVariable { + MlxConfigVariable { + name: name.to_string(), + description: format!("desc of {name}"), + read_only: false, + spec: MlxVariableSpec::Integer, + } + } + + // int_value wraps an Integer value on an Integer-spec variable, validated. + fn int_value(name: &str, n: i64) -> MlxConfigValue { + MlxConfigValue::new(int_var(name), MlxValueType::Integer(n)).unwrap() + } + + // queried builds a QueriedVariable whose current/next values let us drive + // is_pending_change: pending == (current != next). + fn queried(name: &str, current: i64, next: i64) -> QueriedVariable { + QueriedVariable { + variable: int_var(name), + current_value: int_value(name, current), + default_value: int_value(name, 0), + next_value: int_value(name, next), + modified: false, + read_only: false, + } + } + + // ----- QueriedVariable accessors (total) ----- + + // name and description are thin wrappers over the backing variable. + #[test] + fn queried_variable_name_and_description() { + check_values( + [ + Check { + scenario: "name returns the variable name", + input: queried("ALPHA", 1, 1), + expect: "ALPHA".to_string(), + }, + Check { + scenario: "empty name passes through", + input: queried("", 1, 1), + expect: "".to_string(), + }, + ], + |q| q.name().to_string(), + ); + + check_values( + [Check { + scenario: "description mirrors the backing variable", + input: queried("BETA", 1, 1), + expect: "desc of BETA".to_string(), + }], + |q| q.description().to_string(), + ); + } + + // is_pending_change is true exactly when current_value != next_value. + #[test] + fn queried_variable_is_pending_change() { + check_values( + [ + Check { + scenario: "current == next -> no pending change", + input: queried("X", 5, 5), + expect: false, + }, + Check { + scenario: "current != next -> pending change", + input: queried("X", 5, 9), + expect: true, + }, + Check { + scenario: "current == next at zero -> no pending change", + input: queried("X", 0, 0), + expect: false, + }, + ], + |q| q.is_pending_change(), + ); + } + + // ----- QueryResult accessors (total) ----- + + fn query_result(names: &[&str]) -> QueryResult { + QueryResult { + device_info: QueriedDeviceInfo::new(), + variables: names.iter().map(|n| queried(n, 1, 1)).collect(), + } + } + + // variable_count is just the length of the variables vec, including 0. + #[test] + fn query_result_variable_count() { + check_values( + [ + Check { + scenario: "empty", + input: query_result(&[]), + expect: 0usize, + }, + Check { + scenario: "single", + input: query_result(&["a"]), + expect: 1usize, + }, + Check { + scenario: "several", + input: query_result(&["a", "b", "c"]), + expect: 3usize, + }, + ], + |qr| qr.variable_count(), + ); + } + + // get_variable finds by name; returns None when absent. We project to the + // found name (or "") so the row's expectation is a value we are sure of. + #[test] + fn query_result_get_variable() { + check_values( + [ + Check { + scenario: "present in the middle", + input: ("b", query_result(&["a", "b", "c"])), + expect: "b".to_string(), + }, + Check { + scenario: "present at the front", + input: ("a", query_result(&["a", "b", "c"])), + expect: "a".to_string(), + }, + Check { + scenario: "absent yields None", + input: ("missing", query_result(&["a", "b", "c"])), + expect: "".to_string(), + }, + Check { + scenario: "absent in empty result", + input: ("a", query_result(&[])), + expect: "".to_string(), + }, + ], + |(name, qr)| { + qr.get_variable(name) + .map(|v| v.name().to_string()) + .unwrap_or_else(|| "".to_string()) + }, + ); + } + + // variable_names collects every variable's name, preserving order. + #[test] + fn query_result_variable_names() { + check_values( + [ + Check { + scenario: "empty", + input: query_result(&[]), + expect: Vec::::new(), + }, + Check { + scenario: "preserves order", + input: query_result(&["c", "a", "b"]), + expect: vec!["c".to_string(), "a".to_string(), "b".to_string()], + }, + ], + |qr| { + qr.variable_names() + .into_iter() + .map(String::from) + .collect::>() + }, + ); + } + + // ----- summary / description formatters (total) ----- + + // SyncResult::summary embeds changed/checked counts and the execution time. + // The Duration is fixed at zero (Default) so the "{:?}" tail is "0ns". + #[test] + fn sync_result_summary() { + fn sync(checked: usize, changed: usize) -> SyncResult { + SyncResult { + variables_checked: checked, + variables_changed: changed, + changes_applied: vec![], + execution_time: Duration::default(), + query_result: query_result(&[]), + } + } + + check_values( + [ + Check { + scenario: "none changed", + input: sync(3, 0), + expect: "Sync complete: 0/3 variables changed in 0ns".to_string(), + }, + Check { + scenario: "all changed", + input: sync(2, 2), + expect: "Sync complete: 2/2 variables changed in 0ns".to_string(), + }, + ], + |s| s.summary(), + ); + } + + // ComparisonResult::summary embeds needing-change/checked counts. + #[test] + fn comparison_result_summary() { + fn cmp(checked: usize, needing: usize) -> ComparisonResult { + ComparisonResult { + variables_checked: checked, + variables_needing_change: needing, + planned_changes: vec![], + query_result: query_result(&[]), + } + } + + check_values( + [ + Check { + scenario: "none needing change", + input: cmp(5, 0), + expect: "Comparison complete: 0/5 variables would change".to_string(), + }, + Check { + scenario: "some needing change", + input: cmp(5, 2), + expect: "Comparison complete: 2/5 variables would change".to_string(), + }, + ], + |c| c.summary(), + ); + } + + // PlannedChange::description formats "name: current → desired". Integer + // values render bare via to_display_string, so the arrow tail is exact. + #[test] + fn planned_change_description() { + check_values( + [Check { + scenario: "integer current and desired", + input: PlannedChange { + variable_name: "VAR".to_string(), + current_value: int_value("VAR", 1), + desired_value: int_value("VAR", 2), + }, + expect: "VAR: 1 → 2".to_string(), + }], + |c| c.description(), + ); + } + + // VariableChange::description formats "name: old → new". + #[test] + fn variable_change_description() { + check_values( + [Check { + scenario: "integer old and new", + input: VariableChange { + variable_name: "VAR".to_string(), + old_value: int_value("VAR", 7), + new_value: int_value("VAR", 8), + }, + expect: "VAR: 7 → 8".to_string(), + }], + |c| c.description(), + ); + } + + // ----- QueriedDeviceInfo builders (total) ----- + + // new starts every field None; each with_* setter fills exactly its field. + // We project the four fields into a tuple so one table walks every setter. + #[test] + fn queried_device_info_builders() { + type Fields = ( + Option, + Option, + Option, + Option, + ); + fn fields(i: &QueriedDeviceInfo) -> Fields { + ( + i.device_id.clone(), + i.device_type.clone(), + i.part_number.clone(), + i.description.clone(), + ) + } + + check_values( + [ + Check { + scenario: "new is all None", + input: QueriedDeviceInfo::new(), + expect: (None, None, None, None), + }, + Check { + scenario: "with_device_id fills only device_id", + input: QueriedDeviceInfo::new().with_device_id("dev-1"), + expect: (Some("dev-1".to_string()), None, None, None), + }, + Check { + scenario: "with_device_type fills only device_type", + input: QueriedDeviceInfo::new().with_device_type("ConnectX"), + expect: (None, Some("ConnectX".to_string()), None, None), + }, + Check { + scenario: "with_part_number fills only part_number", + input: QueriedDeviceInfo::new().with_part_number("MCX-1"), + expect: (None, None, Some("MCX-1".to_string()), None), + }, + Check { + scenario: "with_description fills only description", + input: QueriedDeviceInfo::new().with_description("a nic"), + expect: (None, None, None, Some("a nic".to_string())), + }, + Check { + scenario: "all setters chain", + input: QueriedDeviceInfo::new() + .with_device_id("dev-1") + .with_device_type("ConnectX") + .with_part_number("MCX-1") + .with_description("a nic"), + expect: ( + Some("dev-1".to_string()), + Some("ConnectX".to_string()), + Some("MCX-1".to_string()), + Some("a nic".to_string()), + ), + }, + ], + |i| fields(&i), + ); + } + + // ----- QueriedDeviceInfo proto round-trips (infallible From both ways) ----- + + // QueriedDeviceInfo <-> Pb is a plain field copy in both directions, so a + // round-trip preserves every Option field, including the all-None case. + #[test] + fn queried_device_info_proto_round_trip() { + type Fields = ( + Option, + Option, + Option, + Option, + ); + fn fields(i: &QueriedDeviceInfo) -> Fields { + ( + i.device_id.clone(), + i.device_type.clone(), + i.part_number.clone(), + i.description.clone(), + ) + } + + check_values( + [ + Check { + scenario: "all None survives the round-trip", + input: QueriedDeviceInfo::new(), + expect: (None, None, None, None), + }, + Check { + scenario: "all Some survives the round-trip", + input: QueriedDeviceInfo::new() + .with_device_id("d") + .with_device_type("t") + .with_part_number("p") + .with_description("x"), + expect: ( + Some("d".to_string()), + Some("t".to_string()), + Some("p".to_string()), + Some("x".to_string()), + ), + }, + ], + |info| { + let pb: QueriedDeviceInfoPb = info.into(); + let back: QueriedDeviceInfo = pb.into(); + fields(&back) + }, + ); + } + + // ----- QueriedVariable proto conversions (fallible) ----- + + // A complete QueriedVariable round-trips through its Pb form; we project the + // name to confirm success. Missing each required sub-message fails, so we + // build the Pb by hand with one field set to None per rejection row. + #[test] + fn queried_variable_proto_conversions() { + // Round-trip the happy path, projecting the variable name out. + Case { + scenario: "complete round-trip preserves the name", + input: queried("RT", 3, 4), + expect: Yields("RT".to_string()), + } + .check(|q| -> Result { + let pb: QueriedVariablePb = q.try_into().map_err(drop)?; + let back: QueriedVariable = pb.try_into().map_err(drop)?; + Ok(back.name().to_string()) + }); + + // Each None field is a distinct MissingArgument rejection path. + fn full_pb() -> QueriedVariablePb { + let q = queried("M", 1, 1); + q.try_into().expect("complete QueriedVariable converts") + } + + check_cases( + [ + Case { + scenario: "missing variable", + input: QueriedVariablePb { + variable: None, + ..full_pb() + }, + expect: Fails, + }, + Case { + scenario: "missing current_value", + input: QueriedVariablePb { + current_value: None, + ..full_pb() + }, + expect: Fails, + }, + Case { + scenario: "missing default_value", + input: QueriedVariablePb { + default_value: None, + ..full_pb() + }, + expect: Fails, + }, + Case { + scenario: "missing next_value", + input: QueriedVariablePb { + next_value: None, + ..full_pb() + }, + expect: Fails, + }, + ], + |pb| { + QueriedVariable::try_from(pb) + .map(|q| q.name().to_string()) + .map_err(drop) + }, + ); + } + + // ----- QueryResult proto conversions (fallible) ----- + + // QueryResult round-trips (projecting variable_count); a missing device_info + // fails the inbound conversion. + #[test] + fn query_result_proto_conversions() { + Case { + scenario: "round-trip preserves variable count", + input: query_result(&["a", "b"]), + expect: Yields(2usize), + } + .check(|qr| -> Result { + let pb: QueryResultPb = qr.try_into().map_err(drop)?; + let back: QueryResult = pb.try_into().map_err(drop)?; + Ok(back.variable_count()) + }); + + Case { + scenario: "missing device_info fails", + input: QueryResultPb { + device_info: None, + variables: vec![], + }, + expect: Fails, + } + .check(|pb| { + QueryResult::try_from(pb) + .map(|q| q.variable_count()) + .map_err(drop) + }); + } + + // ----- PlannedChange proto conversions (fallible) ----- + + #[test] + fn planned_change_proto_conversions() { + fn planned() -> PlannedChange { + PlannedChange { + variable_name: "PC".to_string(), + current_value: int_value("PC", 1), + desired_value: int_value("PC", 2), + } + } + + Case { + scenario: "round-trip preserves the variable name", + input: planned(), + expect: Yields("PC".to_string()), + } + .check(|c| -> Result { + let pb: PlannedChangePb = c.try_into().map_err(drop)?; + let back: PlannedChange = pb.try_into().map_err(drop)?; + Ok(back.variable_name) + }); + + fn full_pb() -> PlannedChangePb { + planned() + .try_into() + .expect("complete PlannedChange converts") + } + + check_cases( + [ + Case { + scenario: "missing current_value", + input: PlannedChangePb { + current_value: None, + ..full_pb() + }, + expect: Fails, + }, + Case { + scenario: "missing desired_value", + input: PlannedChangePb { + desired_value: None, + ..full_pb() + }, + expect: Fails, + }, + ], + |pb| { + PlannedChange::try_from(pb) + .map(|c| c.variable_name) + .map_err(drop) + }, + ); + } + + // ----- VariableChange proto conversions (fallible) ----- + + #[test] + fn variable_change_proto_conversions() { + fn change() -> VariableChange { + VariableChange { + variable_name: "VC".to_string(), + old_value: int_value("VC", 7), + new_value: int_value("VC", 8), + } + } + + Case { + scenario: "round-trip preserves the variable name", + input: change(), + expect: Yields("VC".to_string()), + } + .check(|c| -> Result { + let pb: VariableChangePb = c.try_into().map_err(drop)?; + let back: VariableChange = pb.try_into().map_err(drop)?; + Ok(back.variable_name) + }); + + fn full_pb() -> VariableChangePb { + change() + .try_into() + .expect("complete VariableChange converts") + } + + check_cases( + [ + Case { + scenario: "missing old_value", + input: VariableChangePb { + old_value: None, + ..full_pb() + }, + expect: Fails, + }, + Case { + scenario: "missing new_value", + input: VariableChangePb { + new_value: None, + ..full_pb() + }, + expect: Fails, + }, + ], + |pb| { + VariableChange::try_from(pb) + .map(|c| c.variable_name) + .map_err(drop) + }, + ); + } + + // ----- ComparisonResult proto conversions (fallible) ----- + + // ComparisonResult round-trips (projecting the checked/needing counts as a + // tuple); a missing query_result fails the inbound conversion. The counts + // also exercise the usize<->u64 casts in both directions. + #[test] + fn comparison_result_proto_conversions() { + fn cmp() -> ComparisonResult { + ComparisonResult { + variables_checked: 4, + variables_needing_change: 1, + planned_changes: vec![PlannedChange { + variable_name: "PC".to_string(), + current_value: int_value("PC", 1), + desired_value: int_value("PC", 2), + }], + query_result: query_result(&["a"]), + } + } + + Case { + scenario: "round-trip preserves the counts", + input: cmp(), + expect: Yields((4usize, 1usize)), + } + .check(|c| -> Result<(usize, usize), ()> { + let pb: ComparisonResultPb = c.try_into().map_err(drop)?; + let back: ComparisonResult = pb.try_into().map_err(drop)?; + Ok((back.variables_checked, back.variables_needing_change)) + }); + + Case { + scenario: "missing query_result fails", + input: ComparisonResultPb { + variables_checked: 0, + variables_needing_change: 0, + planned_changes: vec![], + query_result: None, + }, + expect: Fails, + } + .check(|pb| { + ComparisonResult::try_from(pb) + .map(|c| c.variables_checked) + .map_err(drop) + }); + } + + // ----- SyncResult proto conversions (fallible) ----- + + // SyncResult round-trips (projecting checked/changed counts). execution_time + // is serde(skip)/not in the proto, so the inbound side always defaults it to + // zero -- we assert that explicitly. A missing query_result fails. + #[test] + fn sync_result_proto_conversions() { + fn sync() -> SyncResult { + SyncResult { + variables_checked: 6, + variables_changed: 2, + changes_applied: vec![VariableChange { + variable_name: "VC".to_string(), + old_value: int_value("VC", 7), + new_value: int_value("VC", 8), + }], + // A non-zero time that should NOT survive the proto hop. + execution_time: Duration::from_secs(42), + query_result: query_result(&["a"]), + } + } + + Case { + scenario: "round-trip preserves counts and zeroes execution_time", + input: sync(), + expect: Yields((6usize, 2usize, Duration::from_secs(0))), + } + .check(|s| -> Result<(usize, usize, Duration), ()> { + let pb: SyncResultPb = s.try_into().map_err(drop)?; + let back: SyncResult = pb.try_into().map_err(drop)?; + Ok(( + back.variables_checked, + back.variables_changed, + back.execution_time, + )) + }); + + Case { + scenario: "missing query_result fails", + input: SyncResultPb { + variables_checked: 0, + variables_changed: 0, + changes_applied: vec![], + query_result: None, + }, + expect: Fails, + } + .check(|pb| { + SyncResult::try_from(pb) + .map(|s| s.variables_checked) + .map_err(drop) + }); + } +} diff --git a/crates/libmlx/src/variables/registry.rs b/crates/libmlx/src/variables/registry.rs index 3319e9f2af..71448d2e79 100644 --- a/crates/libmlx/src/variables/registry.rs +++ b/crates/libmlx/src/variables/registry.rs @@ -160,3 +160,483 @@ impl TryFrom for MlxVariableRegistry { }) } } + +#[cfg(test)] +mod coverage_tests { + use ::rpc::protos::mlx_device::{ + DeviceFilter as DeviceFilterPb, DeviceFilterSet as DeviceFilterSetPb, + MlxConfigVariable as MlxConfigVariablePb, MlxVariableSpec as MlxVariableSpecPb, + mlx_variable_spec as mlx_variable_spec_pb, + }; + use carbide_libmlx_model::device::info::MlxDeviceInfo; + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + + use super::*; + use crate::device::filters::{DeviceField, MatchMode}; + use crate::variables::spec::MlxVariableSpec; + + // var builds a minimal MlxConfigVariable with the given name and a Boolean + // spec; the spec is irrelevant to registry behavior, only the name is read. + fn var(name: &str) -> MlxConfigVariable { + MlxConfigVariable { + name: name.to_string(), + description: format!("desc for {name}"), + read_only: false, + spec: MlxVariableSpec::Boolean, + } + } + + // exact_filter builds a DeviceFilter on the given field that matches `value` + // exactly (case-insensitive), used for the matches_device cases. + fn exact_filter(field: DeviceField, value: &str) -> DeviceFilter { + DeviceFilter { + field, + values: vec![value.to_string()], + match_mode: MatchMode::Exact, + } + } + + // ----- new --------------------------------------------------------------- + + // new starts empty: the provided name, no variables, and no filters. + #[test] + fn new_starts_empty() { + let reg = MlxVariableRegistry::new("reg-a"); + assert_eq!(reg.name, "reg-a"); + assert!(reg.variables.is_empty()); + assert!(reg.filters.is_none()); + assert!(!reg.has_filters()); + } + + // ----- builder name / variables / add_variable --------------------------- + + // The builder setters overwrite/append as documented. Each row projects the + // single field the setter touches so the non-PartialEq registry stays usable. + #[test] + fn builder_name_overwrites() { + check_values( + [ + Check { + scenario: "name() replaces the constructor name", + input: "renamed", + expect: "renamed".to_string(), + }, + Check { + scenario: "name() accepts an empty string", + input: "", + expect: String::new(), + }, + ], + |new_name| MlxVariableRegistry::new("orig").name(new_name).name, + ); + } + + // variables() replaces the whole list; the projected length proves the swap. + #[test] + fn builder_variables_replaces_list() { + check_values( + [ + Check { + scenario: "empty replacement", + input: vec![], + expect: 0usize, + }, + Check { + scenario: "two-element replacement", + input: vec![var("a"), var("b")], + expect: 2usize, + }, + ], + |vars| { + MlxVariableRegistry::new("r") + .add_variable(var("preexisting")) + .variables(vars) + .variables + .len() + }, + ); + } + + // add_variable appends; chaining three adds yields the three names in order. + #[test] + fn add_variable_appends_in_order() { + let reg = MlxVariableRegistry::new("r") + .add_variable(var("first")) + .add_variable(var("second")) + .add_variable(var("third")); + assert_eq!(reg.variable_names(), vec!["first", "second", "third"]); + } + + // ----- get_variable ------------------------------------------------------ + + // get_variable finds by exact name and returns None for a miss. The closure + // projects the found variable's name (or "") so the row stays robust. + #[test] + fn get_variable_finds_by_name() { + let reg = MlxVariableRegistry::new("r") + .add_variable(var("alpha")) + .add_variable(var("beta")); + + check_values( + [ + Check { + scenario: "first variable", + input: "alpha", + expect: "alpha".to_string(), + }, + Check { + scenario: "later variable", + input: "beta", + expect: "beta".to_string(), + }, + Check { + scenario: "missing variable yields None", + input: "gamma", + expect: "".to_string(), + }, + Check { + scenario: "name match is case-sensitive", + input: "ALPHA", + expect: "".to_string(), + }, + ], + |name| { + reg.get_variable(name) + .map(|v| v.name.clone()) + .unwrap_or_else(|| "".to_string()) + }, + ); + } + + // get_variable on an empty registry is always None. + #[test] + fn get_variable_empty_registry_is_none() { + let reg = MlxVariableRegistry::new("r"); + assert!(reg.get_variable("anything").is_none()); + } + + // ----- variable_names ---------------------------------------------------- + + // variable_names lists names in insertion order; empty for an empty registry. + #[test] + fn variable_names_empty_is_empty() { + let reg = MlxVariableRegistry::new("r"); + assert!(reg.variable_names().is_empty()); + } + + // ----- has_filters ------------------------------------------------------- + + // has_filters is false with no filter set, false with an *empty* set, and + // true only once a set holds at least one filter. + #[test] + fn has_filters_distinguishes_none_empty_and_nonempty() { + check_values( + [ + Check { + scenario: "no filter set", + input: MlxVariableRegistry::new("r"), + expect: false, + }, + Check { + scenario: "present but empty filter set", + input: MlxVariableRegistry::new("r").with_filters(DeviceFilterSet::new()), + expect: false, + }, + Check { + scenario: "non-empty filter set", + input: MlxVariableRegistry::new("r") + .with_filter(exact_filter(DeviceField::DeviceType, "ConnectX-6 Dx")), + expect: true, + }, + ], + |reg| reg.has_filters(), + ); + } + + // ----- with_filter (both match arms) ------------------------------------- + + // with_filter on a fresh registry takes the None arm and creates a one-filter + // set; a second with_filter takes the Some arm and appends. The projected + // filter count distinguishes the two arms. + #[test] + fn with_filter_creates_then_extends() { + check_values( + [ + Check { + scenario: "first filter creates the set (None arm)", + input: 1usize, + expect: 1usize, + }, + Check { + scenario: "second filter extends the set (Some arm)", + input: 2usize, + expect: 2usize, + }, + ], + |count| { + let mut reg = MlxVariableRegistry::new("r"); + for i in 0..count { + reg = reg.with_filter(exact_filter(DeviceField::DeviceType, &format!("v{i}"))); + } + reg.filters.map(|f| f.filters.len()).unwrap_or(0) + }, + ); + } + + // with_filters replaces any prior set wholesale. + #[test] + fn with_filters_sets_the_set() { + let set = DeviceFilterSet::new() + .with_filter(exact_filter(DeviceField::PartNumber, "MCX623106AN-CDAT")); + let reg = MlxVariableRegistry::new("r").with_filters(set); + assert!(reg.has_filters()); + } + + // ----- filter_summary ---------------------------------------------------- + + // filter_summary reports "No filters" with no set, and otherwise delegates to + // the DeviceFilterSet Display. An empty set also Displays as "No filters". + #[test] + fn filter_summary_reports_none_and_delegates() { + check_values( + [ + Check { + scenario: "no filter set", + input: MlxVariableRegistry::new("r"), + expect: "No filters".to_string(), + }, + Check { + scenario: "empty set Displays as No filters", + input: MlxVariableRegistry::new("r").with_filters(DeviceFilterSet::new()), + expect: "No filters".to_string(), + }, + Check { + scenario: "one exact filter renders field:value:mode", + input: MlxVariableRegistry::new("r") + .with_filter(exact_filter(DeviceField::DeviceType, "ConnectX")), + expect: "device_type:ConnectX:exact".to_string(), + }, + ], + |reg| reg.filter_summary(), + ); + } + + // ----- matches_device ---------------------------------------------------- + + // matches_device allows every device when no filters are configured, allows + // when the set is empty (vacuous all()), matches on a satisfied exact filter, + // and rejects on an unsatisfied one. The device's device_type is + // "ConnectX-6 Dx" and part_number "MCX623106AN-CDAT" (create_test_device). + #[test] + fn matches_device_respects_filters() { + let device = MlxDeviceInfo::create_test_device(); + + check_values( + [ + Check { + scenario: "no filters allows all", + input: MlxVariableRegistry::new("r"), + expect: true, + }, + Check { + scenario: "empty filter set allows all", + input: MlxVariableRegistry::new("r").with_filters(DeviceFilterSet::new()), + expect: true, + }, + Check { + scenario: "matching exact device_type filter", + input: MlxVariableRegistry::new("r") + .with_filter(exact_filter(DeviceField::DeviceType, "ConnectX-6 Dx")), + expect: true, + }, + Check { + scenario: "exact filter is case-insensitive", + input: MlxVariableRegistry::new("r") + .with_filter(exact_filter(DeviceField::DeviceType, "connectx-6 dx")), + expect: true, + }, + Check { + scenario: "non-matching device_type filter", + input: MlxVariableRegistry::new("r") + .with_filter(exact_filter(DeviceField::DeviceType, "BlueField3")), + expect: false, + }, + Check { + scenario: "all filters must match (one fails)", + input: MlxVariableRegistry::new("r") + .with_filter(exact_filter(DeviceField::DeviceType, "ConnectX-6 Dx")) + .with_filter(exact_filter(DeviceField::PartNumber, "WRONG-PART")), + expect: false, + }, + ], + |reg| reg.matches_device(&device), + ); + } + + // ----- pb round-trip and conversion error paths -------------------------- + + // From for the pb preserves name, variable count, and + // whether a filter set is present. + #[test] + fn into_pb_preserves_shape() { + let reg = MlxVariableRegistry::new("reg-name") + .add_variable(var("v1")) + .add_variable(var("v2")) + .with_filter(exact_filter(DeviceField::DeviceType, "ConnectX-6 Dx")); + + let pb: MlxVariableRegistryPb = reg.into(); + assert_eq!(pb.name, "reg-name"); + assert_eq!(pb.variables.len(), 2); + assert!(pb.filters.is_some()); + } + + // A registry with no filters maps to a pb with filters None. + #[test] + fn into_pb_without_filters_has_none() { + let pb: MlxVariableRegistryPb = MlxVariableRegistry::new("r").into(); + assert!(pb.filters.is_none()); + } + + // Round-trip Rust -> pb -> Rust preserves name, the variable names, and + // whether filters are present. The registry is not PartialEq, so we compare + // the projected pieces instead of the whole value. + #[test] + fn registry_round_trips_through_pb() { + check_cases( + [ + Case { + scenario: "no filters, no variables", + input: MlxVariableRegistry::new("empty"), + expect: Yields(("empty".to_string(), Vec::::new(), false)), + }, + Case { + scenario: "variables only", + input: MlxVariableRegistry::new("vars") + .add_variable(var("a")) + .add_variable(var("b")), + expect: Yields(( + "vars".to_string(), + vec!["a".to_string(), "b".to_string()], + false, + )), + }, + Case { + scenario: "variables and a filter", + input: MlxVariableRegistry::new("full") + .add_variable(var("only")) + .with_filter(exact_filter(DeviceField::PartNumber, "MCX")), + expect: Yields(("full".to_string(), vec!["only".to_string()], true)), + }, + ], + |reg: MlxVariableRegistry| { + let pb: MlxVariableRegistryPb = reg.into(); + let back: MlxVariableRegistry = pb.try_into().map_err(drop)?; + let names: Vec = back + .variable_names() + .iter() + .map(|s| s.to_string()) + .collect(); + let has_filters = back.has_filters(); + Ok::<_, ()>((back.name, names, has_filters)) + }, + ); + } + + // TryFrom fails when a contained filter is invalid (an unspecified device + // field, i32 = 0), surfacing an InvalidArgument from the registry conversion. + // A variable missing its spec also fails the conversion. The happy path with a + // valid filter succeeds. Project to Ok-vs-Err since the error isn't PartialEq. + #[test] + fn try_from_pb_rejects_bad_filters_and_variables() { + fn bool_spec_pb() -> MlxVariableSpecPb { + MlxVariableSpecPb { + spec_type: Some(mlx_variable_spec_pb::SpecType::Boolean( + mlx_variable_spec_pb::BooleanSpec {}, + )), + } + } + + check_cases( + [ + Case { + scenario: "valid filter converts", + input: MlxVariableRegistryPb { + name: "ok".to_string(), + variables: vec![], + filters: Some(DeviceFilterSetPb { + filters: vec![DeviceFilterPb { + // 1 == DEVICE_FIELD_DEVICE_TYPE + field: 1, + values: vec!["x".to_string()], + // 2 == MATCH_MODE_EXACT + match_mode: 2, + }], + }), + }, + expect: Yields(()), + }, + Case { + scenario: "unspecified device field rejected", + input: MlxVariableRegistryPb { + name: "bad-field".to_string(), + variables: vec![], + filters: Some(DeviceFilterSetPb { + filters: vec![DeviceFilterPb { + // 0 == DEVICE_FIELD_UNSPECIFIED -> conversion error + field: 0, + values: vec!["x".to_string()], + match_mode: 2, + }], + }), + }, + expect: Fails, + }, + Case { + scenario: "out-of-range device field rejected", + input: MlxVariableRegistryPb { + name: "bad-field-range".to_string(), + variables: vec![], + filters: Some(DeviceFilterSetPb { + filters: vec![DeviceFilterPb { + field: 999, + values: vec!["x".to_string()], + match_mode: 2, + }], + }), + }, + expect: Fails, + }, + Case { + scenario: "variable missing its spec rejected", + input: MlxVariableRegistryPb { + name: "bad-var".to_string(), + variables: vec![MlxConfigVariablePb { + name: "v".to_string(), + description: "d".to_string(), + read_only: false, + spec: None, + }], + filters: None, + }, + expect: Fails, + }, + Case { + scenario: "no filters, valid variable converts", + input: MlxVariableRegistryPb { + name: "plain".to_string(), + variables: vec![MlxConfigVariablePb { + name: "v".to_string(), + description: "d".to_string(), + read_only: true, + spec: Some(bool_spec_pb()), + }], + filters: None, + }, + expect: Yields(()), + }, + ], + |pb: MlxVariableRegistryPb| MlxVariableRegistry::try_from(pb).map(drop).map_err(drop), + ); + } +} diff --git a/crates/libmlx/src/variables/spec.rs b/crates/libmlx/src/variables/spec.rs index f5f66e9b46..4f577dab4b 100644 --- a/crates/libmlx/src/variables/spec.rs +++ b/crates/libmlx/src/variables/spec.rs @@ -397,3 +397,357 @@ impl TryFrom for MlxVariableSpec { } } } + +#[cfg(test)] +mod coverage_tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + + use super::*; + + // Display covers every enum arm: the six plain scalars, the untyped Array, the + // configured-scalar Enum/Preset, the sized arrays, the sized+optioned EnumArray, + // and Opaque. Each row pins the exact rendered string, including the join/format + // details (comma-space joins, bracketed sizes, "max:" / "[size]" layouts). + #[test] + fn display_renders_each_variant() { + check_values( + [ + Check { + scenario: "boolean", + input: MlxVariableSpec::Boolean, + expect: "Boolean", + }, + Check { + scenario: "integer", + input: MlxVariableSpec::Integer, + expect: "Integer", + }, + Check { + scenario: "string", + input: MlxVariableSpec::String, + expect: "String", + }, + Check { + scenario: "binary", + input: MlxVariableSpec::Binary, + expect: "Binary", + }, + Check { + scenario: "bytes", + input: MlxVariableSpec::Bytes, + expect: "Bytes", + }, + Check { + scenario: "array", + input: MlxVariableSpec::Array, + expect: "Array", + }, + Check { + scenario: "opaque", + input: MlxVariableSpec::Opaque, + expect: "Opaque", + }, + Check { + scenario: "enum with options", + input: MlxVariableSpec::Enum { + options: vec!["low".to_string(), "high".to_string()], + }, + expect: "Enum [low, high]", + }, + Check { + scenario: "enum with no options", + input: MlxVariableSpec::Enum { options: vec![] }, + expect: "Enum []", + }, + Check { + scenario: "preset", + input: MlxVariableSpec::Preset { max_preset: 7 }, + expect: "Preset (max: 7)", + }, + Check { + scenario: "boolean array", + input: MlxVariableSpec::BooleanArray { size: 3 }, + expect: "BooleanArray[3]", + }, + Check { + scenario: "integer array", + input: MlxVariableSpec::IntegerArray { size: 5 }, + expect: "IntegerArray[5]", + }, + Check { + scenario: "enum array", + input: MlxVariableSpec::EnumArray { + options: vec!["a".to_string(), "b".to_string()], + size: 2, + }, + expect: "EnumArray[2] [a, b]", + }, + Check { + scenario: "enum array with no options", + input: MlxVariableSpec::EnumArray { + options: vec![], + size: 4, + }, + expect: "EnumArray[4] []", + }, + Check { + scenario: "binary array", + input: MlxVariableSpec::BinaryArray { size: 8 }, + expect: "BinaryArray[8]", + }, + ], + |spec| &*spec.to_string().leak(), + ); + } + + // The builder entry points for the configuration-free variants each return a + // SimpleBuilder whose build() yields exactly the matching variant. Driving them + // through one table exercises every simple builder method. + #[test] + fn simple_builders_yield_their_variant() { + check_values( + [ + Check { + scenario: "boolean", + input: MlxVariableSpec::builder().boolean().build(), + expect: MlxVariableSpec::Boolean, + }, + Check { + scenario: "integer", + input: MlxVariableSpec::builder().integer().build(), + expect: MlxVariableSpec::Integer, + }, + Check { + scenario: "string", + input: MlxVariableSpec::builder().string().build(), + expect: MlxVariableSpec::String, + }, + Check { + scenario: "binary", + input: MlxVariableSpec::builder().binary().build(), + expect: MlxVariableSpec::Binary, + }, + Check { + scenario: "bytes", + input: MlxVariableSpec::builder().bytes().build(), + expect: MlxVariableSpec::Bytes, + }, + Check { + scenario: "array", + input: MlxVariableSpec::builder().array().build(), + expect: MlxVariableSpec::Array, + }, + Check { + scenario: "opaque", + input: MlxVariableSpec::builder().opaque().build(), + expect: MlxVariableSpec::Opaque, + }, + ], + |built| built, + ); + } + + // The configured builders each have a with_* setter path and an unset-default + // path. This pins both: enum defaults to empty options, preset defaults to 0, + // and the sized arrays default to size 1; the configured rows pin the set value. + #[test] + fn configured_builders_apply_values_and_defaults() { + check_values( + [ + Check { + scenario: "enum with options set", + input: MlxVariableSpec::builder() + .enum_type() + .with_options(vec!["x".to_string(), "y".to_string()]) + .build(), + expect: MlxVariableSpec::Enum { + options: vec!["x".to_string(), "y".to_string()], + }, + }, + Check { + scenario: "enum default options is empty", + input: MlxVariableSpec::builder().enum_type().build(), + expect: MlxVariableSpec::Enum { options: vec![] }, + }, + Check { + scenario: "preset with max set", + input: MlxVariableSpec::builder() + .preset() + .with_max_preset(9) + .build(), + expect: MlxVariableSpec::Preset { max_preset: 9 }, + }, + Check { + scenario: "preset default max is 0", + input: MlxVariableSpec::builder().preset().build(), + expect: MlxVariableSpec::Preset { max_preset: 0 }, + }, + Check { + scenario: "boolean array with size set", + input: MlxVariableSpec::builder() + .boolean_array() + .with_size(6) + .build(), + expect: MlxVariableSpec::BooleanArray { size: 6 }, + }, + Check { + scenario: "boolean array default size is 1", + input: MlxVariableSpec::builder().boolean_array().build(), + expect: MlxVariableSpec::BooleanArray { size: 1 }, + }, + Check { + scenario: "integer array with size set", + input: MlxVariableSpec::builder() + .integer_array() + .with_size(4) + .build(), + expect: MlxVariableSpec::IntegerArray { size: 4 }, + }, + Check { + scenario: "integer array default size is 1", + input: MlxVariableSpec::builder().integer_array().build(), + expect: MlxVariableSpec::IntegerArray { size: 1 }, + }, + Check { + scenario: "binary array with size set", + input: MlxVariableSpec::builder() + .binary_array() + .with_size(2) + .build(), + expect: MlxVariableSpec::BinaryArray { size: 2 }, + }, + Check { + scenario: "binary array default size is 1", + input: MlxVariableSpec::builder().binary_array().build(), + expect: MlxVariableSpec::BinaryArray { size: 1 }, + }, + Check { + scenario: "enum array with options and size set", + input: MlxVariableSpec::builder() + .enum_array() + .with_options(vec!["a".to_string()]) + .with_size(3) + .build(), + expect: MlxVariableSpec::EnumArray { + options: vec!["a".to_string()], + size: 3, + }, + }, + Check { + scenario: "enum array defaults: empty options, size 1", + input: MlxVariableSpec::builder().enum_array().build(), + expect: MlxVariableSpec::EnumArray { + options: vec![], + size: 1, + }, + }, + ], + |built| built, + ); + } + + // Every variant survives a From -> TryFrom round trip through the protobuf + // representation. This drives both the From conversion (all 13 arms) and the + // TryFrom success path (all 13 arms), pinning the recovered spec to the input. + // Sizes are chosen small so the usize/u64 and u8/u32 casts are lossless. + #[test] + fn pb_round_trip_preserves_each_variant() { + let specs = [ + ("boolean", MlxVariableSpec::Boolean), + ("integer", MlxVariableSpec::Integer), + ("string", MlxVariableSpec::String), + ("binary", MlxVariableSpec::Binary), + ("bytes", MlxVariableSpec::Bytes), + ("array", MlxVariableSpec::Array), + ("opaque", MlxVariableSpec::Opaque), + ( + "enum", + MlxVariableSpec::Enum { + options: vec!["one".to_string(), "two".to_string()], + }, + ), + ( + "enum empty options", + MlxVariableSpec::Enum { options: vec![] }, + ), + ("preset zero", MlxVariableSpec::Preset { max_preset: 0 }), + ("preset max u8", MlxVariableSpec::Preset { max_preset: 255 }), + ("boolean array", MlxVariableSpec::BooleanArray { size: 0 }), + ("integer array", MlxVariableSpec::IntegerArray { size: 7 }), + ( + "enum array", + MlxVariableSpec::EnumArray { + options: vec!["in".to_string(), "out".to_string()], + size: 4, + }, + ), + ("binary array", MlxVariableSpec::BinaryArray { size: 1 }), + ]; + + check_cases( + specs.into_iter().map(|(scenario, spec)| Case { + scenario, + input: spec.clone(), + expect: Yields(spec), + }), + |spec| { + let pb: MlxVariableSpecPb = spec.into(); + MlxVariableSpec::try_from(pb).map_err(drop) + }, + ); + } + + // TryFrom rejects a protobuf with no spec_type set (the MissingArgument arm). + // The error type is not asserted for equality here, so map_err(drop) + Fails + // keeps the row to an Ok-vs-Err contract. + #[test] + fn try_from_rejects_missing_spec_type() { + Case { + scenario: "absent spec_type fails", + input: MlxVariableSpecPb { spec_type: None }, + expect: Fails, + } + .check(|pb| MlxVariableSpec::try_from(pb).map_err(drop)); + } + + // The serde tag/content/snake_case attributes mean a spec serializes to JSON and + // deserializes back to itself. Round-tripping every variant exercises the derived + // Serialize/Deserialize over the full enum without pinning the exact JSON text. + #[test] + fn json_round_trip_preserves_each_variant() { + let specs = [ + ("boolean", MlxVariableSpec::Boolean), + ("opaque", MlxVariableSpec::Opaque), + ( + "enum", + MlxVariableSpec::Enum { + options: vec!["a".to_string(), "b".to_string()], + }, + ), + ("preset", MlxVariableSpec::Preset { max_preset: 12 }), + ("boolean array", MlxVariableSpec::BooleanArray { size: 3 }), + ("integer array", MlxVariableSpec::IntegerArray { size: 9 }), + ( + "enum array", + MlxVariableSpec::EnumArray { + options: vec!["x".to_string()], + size: 2, + }, + ), + ("binary array", MlxVariableSpec::BinaryArray { size: 4 }), + ]; + + check_cases( + specs.into_iter().map(|(scenario, spec)| Case { + scenario, + input: spec.clone(), + expect: Yields(spec), + }), + |spec| { + let json = serde_json::to_string(&spec).map_err(drop)?; + serde_json::from_str::(&json).map_err(drop) + }, + ); + } +} diff --git a/crates/libmlx/src/variables/value.rs b/crates/libmlx/src/variables/value.rs index 28367312b3..93c3e35b91 100644 --- a/crates/libmlx/src/variables/value.rs +++ b/crates/libmlx/src/variables/value.rs @@ -1349,3 +1349,1012 @@ impl TryFrom for MlxValueType { } } } + +#[cfg(test)] +mod coverage_tests { + use carbide_test_support::Outcome::*; + use carbide_test_support::{Case, Check, check_cases, check_values}; + use serde_yaml::Value as Yaml; + + use super::*; + + // Small helper to build a variable around a spec for `with`/validate tests. + fn var(name: &str, spec: MlxVariableSpec) -> MlxConfigVariable { + MlxConfigVariable { + name: name.to_string(), + description: format!("desc: {name}"), + read_only: false, + spec, + } + } + + // ----- MlxValueType::to_display_string for scalar (non-array) variants ----- + // The array variants are already covered by the sibling tests/ file; here we + // pin every scalar arm of `to_display_string`. + #[test] + fn display_string_for_scalar_value_types() { + // We build the value directly, wrapping in an MlxConfigValue via a matching + // spec, then read back to_display_string. + check_values( + [ + Check { + scenario: "boolean true", + input: MlxValueType::Boolean(true), + expect: "true".to_string(), + }, + Check { + scenario: "boolean false", + input: MlxValueType::Boolean(false), + expect: "false".to_string(), + }, + Check { + scenario: "integer negative", + input: MlxValueType::Integer(-5), + expect: "-5".to_string(), + }, + Check { + scenario: "string verbatim", + input: MlxValueType::String("hi there".to_string()), + expect: "hi there".to_string(), + }, + Check { + scenario: "binary hex-encoded", + input: MlxValueType::Binary(vec![0x1a, 0x2b]), + expect: "0x1a2b".to_string(), + }, + Check { + scenario: "bytes reports length", + input: MlxValueType::Bytes(vec![0x00, 0x01, 0x02]), + expect: "3 bytes".to_string(), + }, + Check { + scenario: "untyped array joins elements", + input: MlxValueType::Array(vec!["a".to_string(), "b".to_string()]), + expect: "[a, b]".to_string(), + }, + Check { + scenario: "enum verbatim", + input: MlxValueType::Enum("selected".to_string()), + expect: "selected".to_string(), + }, + Check { + scenario: "preset prefixed", + input: MlxValueType::Preset(4), + expect: "preset_4".to_string(), + }, + Check { + scenario: "opaque reports byte count", + input: MlxValueType::Opaque(vec![0x01, 0x02, 0x03]), + expect: "opaque(3 bytes)".to_string(), + }, + // Binary array with all-None still reports total/set counts. + Check { + scenario: "binary array all unset", + input: MlxValueType::BinaryArray(vec![None, None]), + expect: "[2 binary values, 0 set]".to_string(), + }, + ], + |value| { + MlxConfigValue { + variable: var("v", MlxVariableSpec::Boolean), + value, + } + .to_display_string() + }, + ); + } + + // The Display impl just forwards to to_display_string; confirm parity once. + #[test] + fn display_impl_matches_to_display_string() { + let value = MlxConfigValue { + variable: var("v", MlxVariableSpec::Integer), + value: MlxValueType::Integer(99), + }; + assert_eq!(format!("{value}"), value.to_display_string()); + assert_eq!(format!("{value}"), "99"); + } + + // ----- MlxConfigValue accessors ----- + #[test] + fn accessors_project_the_backing_variable() { + let variable = MlxConfigVariable { + name: "the_name".to_string(), + description: "the_desc".to_string(), + read_only: true, + spec: MlxVariableSpec::Boolean, + }; + let value = MlxConfigValue { + variable, + value: MlxValueType::Boolean(true), + }; + assert_eq!(value.name(), "the_name"); + assert_eq!(value.description(), "the_desc"); + assert!(value.is_read_only()); + assert_eq!(value.spec(), &MlxVariableSpec::Boolean); + } + + // ----- MlxValueError Display strings (the contract for each variant) ----- + #[test] + fn error_display_strings() { + check_values( + [ + Check { + scenario: "type mismatch", + input: MlxValueError::TypeMismatch { + expected: "A".to_string(), + got: "B".to_string(), + }, + expect: "Type mismatch: expected A, got B".to_string(), + }, + Check { + scenario: "invalid enum option", + input: MlxValueError::InvalidEnumOption { + value: "x".to_string(), + allowed: vec!["a".to_string(), "b".to_string()], + }, + expect: "Invalid enum option 'x', allowed: [a, b]".to_string(), + }, + Check { + scenario: "preset out of range", + input: MlxValueError::PresetOutOfRange { + value: 9, + max_allowed: 5, + }, + expect: "Preset value 9 exceeds maximum 5".to_string(), + }, + Check { + scenario: "array size mismatch", + input: MlxValueError::ArraySizeMismatch { + expected: 4, + got: 2, + }, + expect: "Array size mismatch: expected 4, got 2".to_string(), + }, + Check { + scenario: "invalid enum array option", + input: MlxValueError::InvalidEnumArrayOption { + position: 1, + value: "z".to_string(), + allowed: vec!["a".to_string()], + }, + expect: "Invalid enum option 'z' at position 1, allowed: [a]".to_string(), + }, + Check { + scenario: "read only variable", + input: MlxValueError::ReadOnlyVariable { + variable_name: "RO".to_string(), + }, + expect: "Variable 'RO' is read-only".to_string(), + }, + ], + |err| err.to_string(), + ); + } + + // ----- MlxConfigValue::new / validate ----- + // new() runs validate_internal; cover both the happy path and the validation + // failures that map spec<->value. The exact errors are pinned where derivable. + #[test] + fn new_validates_spec_against_value() { + check_cases( + [ + Case { + scenario: "matching boolean", + input: (MlxVariableSpec::Boolean, MlxValueType::Boolean(true)), + expect: Yields(()), + }, + Case { + scenario: "matching opaque", + input: (MlxVariableSpec::Opaque, MlxValueType::Opaque(vec![1, 2])), + expect: Yields(()), + }, + Case { + scenario: "matching untyped array", + input: ( + MlxVariableSpec::Array, + MlxValueType::Array(vec!["a".to_string()]), + ), + expect: Yields(()), + }, + Case { + scenario: "spec/value type mismatch", + input: (MlxVariableSpec::Boolean, MlxValueType::Integer(1)), + expect: Fails, + }, + Case { + scenario: "enum value not in options", + input: ( + MlxVariableSpec::Enum { + options: vec!["a".to_string()], + }, + MlxValueType::Enum("nope".to_string()), + ), + expect: FailsWith(MlxValueError::InvalidEnumOption { + value: "nope".to_string(), + allowed: vec!["a".to_string()], + }), + }, + Case { + scenario: "enum value in options", + input: ( + MlxVariableSpec::Enum { + options: vec!["a".to_string()], + }, + MlxValueType::Enum("a".to_string()), + ), + expect: Yields(()), + }, + Case { + scenario: "preset over max", + input: ( + MlxVariableSpec::Preset { max_preset: 3 }, + MlxValueType::Preset(4), + ), + expect: FailsWith(MlxValueError::PresetOutOfRange { + value: 4, + max_allowed: 3, + }), + }, + Case { + scenario: "preset at max", + input: ( + MlxVariableSpec::Preset { max_preset: 3 }, + MlxValueType::Preset(3), + ), + expect: Yields(()), + }, + Case { + scenario: "boolean array wrong size", + input: ( + MlxVariableSpec::BooleanArray { size: 2 }, + MlxValueType::BooleanArray(vec![Some(true)]), + ), + expect: FailsWith(MlxValueError::ArraySizeMismatch { + expected: 2, + got: 1, + }), + }, + Case { + scenario: "integer array right size", + input: ( + MlxVariableSpec::IntegerArray { size: 2 }, + MlxValueType::IntegerArray(vec![Some(1), None]), + ), + expect: Yields(()), + }, + Case { + scenario: "binary array wrong size", + input: ( + MlxVariableSpec::BinaryArray { size: 1 }, + MlxValueType::BinaryArray(vec![None, None]), + ), + expect: FailsWith(MlxValueError::ArraySizeMismatch { + expected: 1, + got: 2, + }), + }, + Case { + scenario: "enum array wrong size", + input: ( + MlxVariableSpec::EnumArray { + options: vec!["a".to_string()], + size: 2, + }, + MlxValueType::EnumArray(vec![Some("a".to_string())]), + ), + expect: FailsWith(MlxValueError::ArraySizeMismatch { + expected: 2, + got: 1, + }), + }, + Case { + scenario: "enum array invalid element", + input: ( + MlxVariableSpec::EnumArray { + options: vec!["a".to_string()], + size: 2, + }, + MlxValueType::EnumArray(vec![Some("a".to_string()), Some("b".to_string())]), + ), + expect: FailsWith(MlxValueError::InvalidEnumArrayOption { + position: 1, + value: "b".to_string(), + allowed: vec!["a".to_string()], + }), + }, + Case { + scenario: "enum array None elements skipped in validation", + input: ( + MlxVariableSpec::EnumArray { + options: vec!["a".to_string()], + size: 2, + }, + MlxValueType::EnumArray(vec![Some("a".to_string()), None]), + ), + expect: Yields(()), + }, + ], + |(spec, value)| MlxConfigValue::new(var("v", spec), value).map(|_| ()), + ); + } + + // ----- IntoMlxValue for i64: preset conversion paths ----- + #[test] + fn i64_into_value_for_spec() { + let preset = MlxVariableSpec::Preset { max_preset: 5 }; + check_cases( + [ + Case { + scenario: "integer spec", + input: (MlxVariableSpec::Integer, 42i64), + expect: Yields(MlxValueType::Integer(42)), + }, + Case { + scenario: "preset in range", + input: (preset.clone(), 3i64), + expect: Yields(MlxValueType::Preset(3)), + }, + Case { + scenario: "preset negative rejected", + input: (preset.clone(), -1i64), + expect: Fails, + }, + Case { + scenario: "preset above u8::MAX rejected", + input: (preset.clone(), 300i64), + expect: Fails, + }, + Case { + scenario: "preset over max rejected", + input: (preset.clone(), 10i64), + expect: FailsWith(MlxValueError::PresetOutOfRange { + value: 10, + max_allowed: 5, + }), + }, + Case { + scenario: "mismatched spec", + input: (MlxVariableSpec::Boolean, 1i64), + expect: Fails, + }, + ], + |(spec, n)| n.into_mlx_value_for_spec(&spec), + ); + } + + // i32 delegates to i64. + #[test] + fn i32_delegates_to_i64() { + Case { + scenario: "i32 integer", + input: 7i32, + expect: Yields(MlxValueType::Integer(7)), + } + .check(|n| n.into_mlx_value_for_spec(&MlxVariableSpec::Integer)); + } + + // ----- IntoMlxValue for u8: Preset vs Integer vs mismatch ----- + #[test] + fn u8_into_value_for_spec() { + let preset = MlxVariableSpec::Preset { max_preset: 5 }; + check_cases( + [ + Case { + scenario: "preset in range", + input: (preset.clone(), 5u8), + expect: Yields(MlxValueType::Preset(5)), + }, + Case { + scenario: "preset over max", + input: (preset.clone(), 6u8), + expect: FailsWith(MlxValueError::PresetOutOfRange { + value: 6, + max_allowed: 5, + }), + }, + Case { + scenario: "integer spec widens to i64", + input: (MlxVariableSpec::Integer, 200u8), + expect: Yields(MlxValueType::Integer(200)), + }, + Case { + scenario: "mismatched spec", + input: (MlxVariableSpec::String, 1u8), + expect: Fails, + }, + ], + |(spec, n)| n.into_mlx_value_for_spec(&spec), + ); + } + + // ----- IntoMlxValue for bool: only Boolean spec, else mismatch ----- + #[test] + fn bool_into_value_for_spec() { + check_cases( + [ + Case { + scenario: "boolean spec", + input: (MlxVariableSpec::Boolean, true), + expect: Yields(MlxValueType::Boolean(true)), + }, + Case { + scenario: "mismatched spec", + input: (MlxVariableSpec::Integer, false), + expect: Fails, + }, + ], + |(spec, b)| b.into_mlx_value_for_spec(&spec), + ); + } + + // ----- IntoMlxValue for Vec: Binary / Bytes / Opaque / mismatch ----- + #[test] + fn vec_u8_into_value_for_spec() { + check_cases( + [ + Case { + scenario: "binary spec", + input: (MlxVariableSpec::Binary, vec![1u8, 2, 3]), + expect: Yields(MlxValueType::Binary(vec![1, 2, 3])), + }, + Case { + scenario: "bytes spec", + input: (MlxVariableSpec::Bytes, vec![1u8, 2, 3]), + expect: Yields(MlxValueType::Bytes(vec![1, 2, 3])), + }, + Case { + scenario: "opaque spec", + input: (MlxVariableSpec::Opaque, vec![1u8, 2, 3]), + expect: Yields(MlxValueType::Opaque(vec![1, 2, 3])), + }, + Case { + scenario: "mismatched spec", + input: (MlxVariableSpec::Boolean, vec![1u8]), + expect: Fails, + }, + ], + |(spec, bytes)| bytes.into_mlx_value_for_spec(&spec), + ); + } + + // ----- String IntoMlxValue: the spec branches not in the sibling file ----- + // Binary/Bytes/Opaque hex parsing (bare + 0x prefix), and the catch-all array + // rejection. + #[test] + fn string_into_value_hex_and_array_rejection() { + check_cases( + [ + Case { + scenario: "binary bare hex", + input: (MlxVariableSpec::Binary, "1a2b".to_string()), + expect: Yields(MlxValueType::Binary(vec![0x1a, 0x2b])), + }, + Case { + scenario: "bytes 0X prefix", + input: (MlxVariableSpec::Bytes, "0XFF00".to_string()), + expect: Yields(MlxValueType::Bytes(vec![0xff, 0x00])), + }, + Case { + scenario: "opaque bad hex rejected", + input: (MlxVariableSpec::Opaque, "zz".to_string()), + expect: Fails, + }, + Case { + scenario: "integer-array spec rejects single string", + input: (MlxVariableSpec::IntegerArray { size: 2 }, "5".to_string()), + expect: Fails, + }, + ], + |(spec, s)| s.into_mlx_value_for_spec(&spec), + ); + } + + // &str and &String both wrap the String impl. + #[test] + fn str_and_string_ref_delegate() { + let s = "hello".to_string(); + Case { + scenario: "&str", + input: "hello", + expect: Yields(MlxValueType::String("hello".to_string())), + } + .check(|raw: &str| raw.into_mlx_value_for_spec(&MlxVariableSpec::String)); + Case { + scenario: "&String", + input: &s, + expect: Yields(MlxValueType::String("hello".to_string())), + } + .check(|raw: &String| raw.into_mlx_value_for_spec(&MlxVariableSpec::String)); + } + + // ----- Direct sparse-input IntoMlxValue impls (size + mismatch arms) ----- + #[test] + fn sparse_vec_inputs_validate_size_and_spec() { + // Vec> + check_cases( + [ + Case { + scenario: "bool sparse right size", + input: ( + MlxVariableSpec::BooleanArray { size: 2 }, + vec![Some(true), None], + ), + expect: Yields(MlxValueType::BooleanArray(vec![Some(true), None])), + }, + Case { + scenario: "bool sparse wrong size", + input: (MlxVariableSpec::BooleanArray { size: 3 }, vec![Some(true)]), + expect: Fails, + }, + Case { + scenario: "bool sparse wrong spec", + input: (MlxVariableSpec::Boolean, vec![Some(true)]), + expect: Fails, + }, + ], + |(spec, v)| v.into_mlx_value_for_spec(&spec), + ); + + // Vec> + check_cases( + [ + Case { + scenario: "int sparse right size", + input: ( + MlxVariableSpec::IntegerArray { size: 2 }, + vec![Some(1i64), None], + ), + expect: Yields(MlxValueType::IntegerArray(vec![Some(1), None])), + }, + Case { + scenario: "int sparse wrong size", + input: (MlxVariableSpec::IntegerArray { size: 1 }, vec![None, None]), + expect: Fails, + }, + Case { + scenario: "int sparse wrong spec", + input: (MlxVariableSpec::Integer, vec![Some(1i64)]), + expect: Fails, + }, + ], + |(spec, v)| v.into_mlx_value_for_spec(&spec), + ); + + // Vec>> + check_cases( + [ + Case { + scenario: "binary sparse right size", + input: ( + MlxVariableSpec::BinaryArray { size: 2 }, + vec![Some(vec![1u8]), None], + ), + expect: Yields(MlxValueType::BinaryArray(vec![Some(vec![1]), None])), + }, + Case { + scenario: "binary sparse wrong size", + input: (MlxVariableSpec::BinaryArray { size: 3 }, vec![None]), + expect: Fails, + }, + Case { + scenario: "binary sparse wrong spec", + input: (MlxVariableSpec::Bytes, vec![Some(vec![1u8])]), + expect: Fails, + }, + ], + |(spec, v)| v.into_mlx_value_for_spec(&spec), + ); + + // Vec> for enum arrays: size, invalid option, wrong spec. + let enum_spec = MlxVariableSpec::EnumArray { + options: vec!["a".to_string(), "b".to_string()], + size: 2, + }; + check_cases( + [ + Case { + scenario: "enum sparse valid", + input: (enum_spec.clone(), vec![Some("a".to_string()), None]), + expect: Yields(MlxValueType::EnumArray(vec![Some("a".to_string()), None])), + }, + Case { + scenario: "enum sparse wrong size", + input: (enum_spec.clone(), vec![Some("a".to_string())]), + expect: Fails, + }, + Case { + scenario: "enum sparse invalid option pins position", + input: ( + enum_spec.clone(), + vec![Some("a".to_string()), Some("zzz".to_string())], + ), + expect: FailsWith(MlxValueError::InvalidEnumArrayOption { + position: 1, + value: "zzz".to_string(), + allowed: vec!["a".to_string(), "b".to_string()], + }), + }, + Case { + scenario: "enum sparse wrong spec", + input: (MlxVariableSpec::String, vec![Some("a".to_string())]), + expect: Fails, + }, + ], + |(spec, v)| v.into_mlx_value_for_spec(&spec), + ); + } + + // ----- Dense Vec inputs: the mismatched-spec arms (size arms are covered) ----- + #[test] + fn dense_vec_inputs_reject_mismatched_specs() { + // Vec against non-BooleanArray. + Case { + scenario: "Vec wrong spec", + input: vec![true, false], + expect: Fails, + } + .check(|v| v.into_mlx_value_for_spec(&MlxVariableSpec::Boolean)); + + // Vec against non-IntegerArray. + Case { + scenario: "Vec wrong spec", + input: vec![1i64, 2], + expect: Fails, + } + .check(|v| v.into_mlx_value_for_spec(&MlxVariableSpec::Integer)); + + // Vec delegates to Vec; right size succeeds. + Case { + scenario: "Vec integer array", + input: vec![1i32, 2], + expect: Yields(MlxValueType::IntegerArray(vec![Some(1), Some(2)])), + } + .check(|v| v.into_mlx_value_for_spec(&MlxVariableSpec::IntegerArray { size: 2 })); + + // Vec> against non-BinaryArray. + Case { + scenario: "Vec> wrong spec", + input: vec![vec![1u8]], + expect: Fails, + } + .check(|v| v.into_mlx_value_for_spec(&MlxVariableSpec::Binary)); + + // Vec<&str> delegates to Vec; trims into an untyped Array. + Case { + scenario: "Vec<&str> into untyped array", + input: vec![" a ", "b"], + expect: Yields(MlxValueType::Array(vec!["a".to_string(), "b".to_string()])), + } + .check(|v| v.into_mlx_value_for_spec(&MlxVariableSpec::Array)); + } + + // ----- IntoMlxValue for serde_yaml::Value ----- + #[test] + fn yaml_value_into_value_for_spec() { + check_cases( + [ + Case { + scenario: "bool", + input: (MlxVariableSpec::Boolean, Yaml::Bool(true)), + expect: Yields(MlxValueType::Boolean(true)), + }, + Case { + scenario: "number to integer", + input: (MlxVariableSpec::Integer, Yaml::Number(42.into())), + expect: Yields(MlxValueType::Integer(42)), + }, + Case { + scenario: "number to preset in range", + input: ( + MlxVariableSpec::Preset { max_preset: 5 }, + Yaml::Number(3.into()), + ), + expect: Yields(MlxValueType::Preset(3)), + }, + Case { + scenario: "negative number for preset rejected", + input: ( + MlxVariableSpec::Preset { max_preset: 5 }, + Yaml::Number((-1).into()), + ), + expect: Fails, + }, + Case { + scenario: "string to enum", + input: ( + MlxVariableSpec::Enum { + options: vec!["a".to_string()], + }, + Yaml::String("a".to_string()), + ), + expect: Yields(MlxValueType::Enum("a".to_string())), + }, + Case { + scenario: "sequence to untyped array", + input: ( + MlxVariableSpec::Array, + Yaml::Sequence(vec![ + Yaml::String("x".to_string()), + Yaml::Number(2.into()), + Yaml::Bool(true), + ]), + ), + expect: Yields(MlxValueType::Array(vec![ + "x".to_string(), + "2".to_string(), + "true".to_string(), + ])), + }, + Case { + scenario: "unsupported yaml type rejected", + input: (MlxVariableSpec::String, Yaml::Null), + expect: Fails, + }, + Case { + scenario: "sequence with complex element rejected", + input: ( + MlxVariableSpec::Array, + Yaml::Sequence(vec![Yaml::Sequence(vec![])]), + ), + expect: Fails, + }, + ], + |(spec, y)| y.into_mlx_value_for_spec(&spec), + ); + } + + // yaml_value_to_string helper directly: simple values stringify, complex fails. + #[test] + fn yaml_value_to_string_helper() { + assert_eq!(yaml_value_to_string(Yaml::Bool(true)).unwrap(), "true"); + assert_eq!( + yaml_value_to_string(Yaml::Number(7.into())).unwrap(), + "7".to_string() + ); + assert_eq!( + yaml_value_to_string(Yaml::String("s".to_string())).unwrap(), + "s".to_string() + ); + assert!(yaml_value_to_string(Yaml::Null).is_err()); + } + + // ----- MlxValueType::to_yaml_value: every arm ----- + #[test] + fn to_yaml_value_for_each_variant() { + check_values( + [ + Check { + scenario: "boolean", + input: MlxValueType::Boolean(true), + expect: Yaml::Bool(true), + }, + Check { + scenario: "integer", + input: MlxValueType::Integer(7), + expect: Yaml::Number(7.into()), + }, + Check { + scenario: "string", + input: MlxValueType::String("s".to_string()), + expect: Yaml::String("s".to_string()), + }, + Check { + scenario: "enum", + input: MlxValueType::Enum("e".to_string()), + expect: Yaml::String("e".to_string()), + }, + Check { + scenario: "preset as number", + input: MlxValueType::Preset(3), + expect: Yaml::Number(3.into()), + }, + Check { + scenario: "binary as hex string", + input: MlxValueType::Binary(vec![0x1a, 0x2b]), + expect: Yaml::String("0x1a2b".to_string()), + }, + Check { + scenario: "bytes as hex string", + input: MlxValueType::Bytes(vec![0xff]), + expect: Yaml::String("0xff".to_string()), + }, + Check { + scenario: "opaque as hex string", + input: MlxValueType::Opaque(vec![0x00]), + expect: Yaml::String("0x00".to_string()), + }, + Check { + scenario: "untyped array as sequence", + input: MlxValueType::Array(vec!["a".to_string(), "b".to_string()]), + expect: Yaml::Sequence(vec![ + Yaml::String("a".to_string()), + Yaml::String("b".to_string()), + ]), + }, + Check { + scenario: "boolean array with None as dash", + input: MlxValueType::BooleanArray(vec![Some(true), None]), + expect: Yaml::Sequence(vec![Yaml::Bool(true), Yaml::String("-".to_string())]), + }, + Check { + scenario: "integer array with None as dash", + input: MlxValueType::IntegerArray(vec![Some(5), None]), + expect: Yaml::Sequence(vec![ + Yaml::Number(5.into()), + Yaml::String("-".to_string()), + ]), + }, + Check { + scenario: "enum array with None as dash", + input: MlxValueType::EnumArray(vec![Some("x".to_string()), None]), + expect: Yaml::Sequence(vec![ + Yaml::String("x".to_string()), + Yaml::String("-".to_string()), + ]), + }, + Check { + scenario: "binary array with None as dash", + input: MlxValueType::BinaryArray(vec![Some(vec![0x1a]), None]), + expect: Yaml::Sequence(vec![ + Yaml::String("0x1a".to_string()), + Yaml::String("-".to_string()), + ]), + }, + ], + |value| value.to_yaml_value(), + ); + } + + // ----- Proto round-trips for MlxValueType: From then TryFrom returns input ----- + // Each variant that round-trips cleanly (no Some("") enum-array element, which + // the proto bridge collapses to None) should survive the trip unchanged. + #[test] + fn value_type_proto_round_trips() { + check_cases( + [ + Case { + scenario: "boolean", + input: MlxValueType::Boolean(true), + expect: Yields(MlxValueType::Boolean(true)), + }, + Case { + scenario: "integer", + input: MlxValueType::Integer(-9), + expect: Yields(MlxValueType::Integer(-9)), + }, + Case { + scenario: "string", + input: MlxValueType::String("s".to_string()), + expect: Yields(MlxValueType::String("s".to_string())), + }, + Case { + scenario: "binary", + input: MlxValueType::Binary(vec![1, 2]), + expect: Yields(MlxValueType::Binary(vec![1, 2])), + }, + Case { + scenario: "bytes", + input: MlxValueType::Bytes(vec![3, 4]), + expect: Yields(MlxValueType::Bytes(vec![3, 4])), + }, + Case { + scenario: "untyped array", + input: MlxValueType::Array(vec!["a".to_string()]), + expect: Yields(MlxValueType::Array(vec!["a".to_string()])), + }, + Case { + scenario: "enum", + input: MlxValueType::Enum("e".to_string()), + expect: Yields(MlxValueType::Enum("e".to_string())), + }, + Case { + scenario: "preset", + input: MlxValueType::Preset(7), + expect: Yields(MlxValueType::Preset(7)), + }, + Case { + scenario: "boolean array sparse", + input: MlxValueType::BooleanArray(vec![Some(true), None, Some(false)]), + expect: Yields(MlxValueType::BooleanArray(vec![ + Some(true), + None, + Some(false), + ])), + }, + Case { + scenario: "integer array sparse", + input: MlxValueType::IntegerArray(vec![Some(1), None]), + expect: Yields(MlxValueType::IntegerArray(vec![Some(1), None])), + }, + Case { + scenario: "enum array sparse (None survives empty-string bridge)", + input: MlxValueType::EnumArray(vec![Some("x".to_string()), None]), + expect: Yields(MlxValueType::EnumArray(vec![Some("x".to_string()), None])), + }, + Case { + scenario: "binary array sparse", + input: MlxValueType::BinaryArray(vec![Some(vec![9]), None]), + expect: Yields(MlxValueType::BinaryArray(vec![Some(vec![9]), None])), + }, + Case { + scenario: "opaque", + input: MlxValueType::Opaque(vec![5, 6]), + expect: Yields(MlxValueType::Opaque(vec![5, 6])), + }, + ], + |value| { + let pb: MlxValueTypePb = value.into(); + MlxValueType::try_from(pb).map_err(drop) + }, + ); + } + + // TryFrom with an empty value_type is the missing-argument path. + #[test] + fn value_type_try_from_missing_value_type_fails() { + Case { + scenario: "no value_type", + input: MlxValueTypePb { value_type: None }, + expect: Fails, + } + .check(|pb| MlxValueType::try_from(pb).map_err(drop)); + } + + // ----- MlxConfigValue proto round-trip (From then TryFrom) ----- + #[test] + fn config_value_proto_round_trip() { + let original = MlxConfigValue { + variable: var("vrr", MlxVariableSpec::Integer), + value: MlxValueType::Integer(11), + }; + Case { + scenario: "config value survives proto round-trip", + input: original.clone(), + expect: Yields(original), + } + .check(|cv| { + let pb: MlxConfigValuePb = cv.into(); + MlxConfigValue::try_from(pb).map_err(drop) + }); + } + + // TryFrom missing-argument and invalid-argument paths. + #[test] + fn config_value_try_from_failure_paths() { + // Missing variable. + Case { + scenario: "missing variable", + input: MlxConfigValuePb { + variable: None, + value: Some(MlxValueType::Integer(1).into()), + }, + expect: Fails, + } + .check(|pb| MlxConfigValue::try_from(pb).map_err(drop)); + + // Missing value. + let variable_pb: ::rpc::protos::mlx_device::MlxConfigVariable = + var("v", MlxVariableSpec::Integer).into(); + Case { + scenario: "missing value", + input: MlxConfigValuePb { + variable: Some(variable_pb.clone()), + value: None, + }, + expect: Fails, + } + .check(|pb| MlxConfigValue::try_from(pb).map_err(drop)); + + // Present but spec/value-incompatible -> new() validation fails -> InvalidArgument. + Case { + scenario: "spec/value mismatch surfaces as invalid argument", + input: MlxConfigValuePb { + variable: Some(variable_pb), + value: Some(MlxValueType::Boolean(true).into()), + }, + expect: Fails, + } + .check(|pb| MlxConfigValue::try_from(pb).map_err(drop)); + } +}