test(host-runtime): render fixture paths through TOML at six more sites - #1888
Conversation
Six tests build a `MeshConfig` by interpolating a temp path straight into a
TOML basic string. On Windows that path is `C:\Users\...`, and TOML reads the
`\U` as the start of a unicode escape:
Error { message: "too few unicode value digits, expected unicode
hexadecimal value" }
Same defect #1834 fixed in mesh-llm-commands, in the same shape:
resolver/exact_head_tests.rs:50 2 tests
resolver/native_mtp_tests.rs:389 and :1030 2 tests
runtime/local_split/test_support.rs:693 and :704 1 test, one template
runtime/local_split/tests/content_attestation.rs:85 1 test
The `local_split` template carries two `[[models]]` entries that share one
`{model_path}` argument, so both placeholders lose their quotes together.
Each site now renders the path through `toml::Value::String(..).to_string()`,
which quotes and escapes for TOML, and the fixture drops its own quotes around
the placeholder. Nothing new is introduced: `resolver/test_support.rs` already
has `toml_path` doing exactly that, used eleven times by the speculative
tests, and `local_split/test_support.rs` already writes the same expression
three times a few lines below one of the sites that bypassed it. The one
addition is an explicit import in `exact_head_tests.rs`, since `test_support`
is a private module with no glob re-export.
On Linux and macOS the rendered value is byte-identical to what the fixtures
produced before: a path without backslashes or quotes renders as the same
basic string.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe test fixtures now serialize filesystem paths as TOML values. This replaces manual quoting and display formatting across resolver and runtime tests. ChangesTOML path serialization
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The test fixtures preserve filesystem paths through TOML serialization, with no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
i386
left a comment
i386
left a comment
There was a problem hiding this comment.
Reviewed all seven render sites (the six tests plus the shared {model_path} template used by two [[models]] entries). Every path now goes through toml::Value::String(...) (or the existing toml_path helper), which quotes and escapes for TOML — the same fix as #1834. Purely test code, byte-identical output on Unix since a plain path renders as the same basic string. CI green (the one red Rust Clippy (${{ matrix.batch.id }}) entry is a stale unrendered matrix job with no logs, the real clippy batches 0-2 all pass). Approving.
2 participants
Six host-runtime tests build a
MeshConfigby interpolating a temp path straight into a TOML basic string. On Windows that path isC:\Users\..., and TOML reads\Uas the start of a unicode escape:Same defect #1834 fixed in
mesh-llm-commands, in the same shape.The six sites
resolver::exact_head_tests::canonical_selector_wins_over_another_models_derived_profileresolver/exact_head_tests.rs:50resolver::exact_head_tests::served_alias_and_resolved_path_do_not_select_model_specific_configresolver::native_mtp_tests::speculative_strategy_auto_uses_hardware_model_path_for_direct_gguf_detectionresolver/native_mtp_tests.rs:389resolver::native_mtp_tests::speculative_strategy_native_mtp_accepts_external_mtp_sidecarresolver/native_mtp_tests.rs:1030local_split::test_support::runtime_resolver_uses_config_identity_and_honors_device_overridelocal_split/test_support.rs:693and:704, one templatelocal_split::tests::content_attestation::strict_multimodal_stage_builder_keeps_local_paths_off_downstream_loadslocal_split/tests/content_attestation.rs:85The
local_splittemplate carries two[[models]]entries that share a single{model_path}argument, so both placeholders lose their quotes together. My first pass fixed only the second one and the test kept failing with the rendered literal string wrapped in a second pair of quotes, which is how the seventh site turned up.What this changes
Each site renders the path through
toml::Value::String(...).to_string(), which quotes and escapes for TOML, and the fixture drops its own quotes around the placeholder. Nothing new is introduced:resolver/test_support.rsalready hastoml_pathdoing exactly that, used eleven times by the speculative tests, andlocal_split/test_support.rsalready writes the same expression three times a few lines below one of the sites that bypassed it. The one addition is an explicit import inexact_head_tests.rs, sincetest_supportis a private module with no glob re-export.On Linux and macOS the rendered value is byte-identical to what the fixtures produced before, since a path without backslashes or quotes renders as the same basic string.
Measured effect
Windows 11. On
mainat 950e905 all six fail with the unicode-escape error above. On this branch, the six plus the seven other tests ofruntime::local_split::test_support, run on the tree thatmainat 8ab3586 now has (85b142d plus #1885, which the run carried as a cherry-pick becausemaindid not build in between):Gates
cargo test -p mesh-llm-host-runtime(the six tests andruntime::local_split::test_support): 13 passed, 0 failed.cargo fmt --all --check: clean.cargo clippy -p mesh-llm-host-runtime --all-targets: 8 locations, complete sorted set identical tomainat 60b7f5f.cargo run -p xtask -- repo-consistency no-console-print: passed.scripts/check-env-mutation-contract.pyandpython -m unittest scripts.tests.test_env_mutation_contract: passed, 234 sites.scripts/check-conventional-commit.py --range upstream/main..HEAD: passed.Not verified
Nothing run on Linux or macOS. The rendered TOML is the same there, but I have not executed the tests on either.
Summary by CodeRabbit