Skip to content

fix(test): stop the load_configs test reading the developer's real config#456

Merged
consideRatio merged 3 commits into
sensmetry:mainfrom
consideRatio:fix/load-configs-test-isolation
Jul 21, 2026
Merged

fix(test): stop the load_configs test reading the developer's real config#456
consideRatio merged 3 commits into
sensmetry:mainfrom
consideRatio:fix/load-configs-test-isolation

Conversation

@consideRatio

Copy link
Copy Markdown
Collaborator

config::local_fs::tests::load_configs calls load_configs, which merges the user configuration from dirs::config_dir() before asserting equality against a fixture. The assertion therefore depends on whatever ~/.config/sysand/sysand.toml happens to contain: green on CI runners (no user config), red on any developer machine where sysand is actually used, with the developer's own index entries showing up in the assertion diff.

Fix by injection rather than environment mutation (mutating HOME/XDG_CONFIG_HOME in tests is racy under the multithreaded test runner): the loader is split into load_configs_from(user_config: Option<&Utf8Path>, working_dir), and load_configs delegates to it with the dirs lookup, so public behavior is unchanged. The existing test passes None; a new test exercises the user-then-working-dir merge through the injected path, which was previously untestable in isolation.

Verified on a machine with a real user config (which reproduced the failure before the fix): the config tests and the full core suite pass.


🤖 Generated with Claude Code

https://claude.ai/code/session_01EfyrkJkeo3mRngVoppJjUN

@consideRatio
consideRatio marked this pull request as draft July 19, 2026 18:28
@consideRatio

This comment was marked as outdated.

@andrius-puksta-sensmetry

andrius-puksta-sensmetry commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Isn't every integration test that touches indexes or custom sources affected by this? Do they all pass --no-config?

The test called load_configs, which merges the user configuration from
dirs::config_dir(), so it asserted against whatever
~/.config/sysand/sysand.toml happens to contain: green on CI runners,
red on any developer machine with a real config.

Split the loader into load_configs_from with the user-config location
injected; load_configs delegates with the dirs lookup, public behavior
unchanged. The test passes None, and a new test covers the
user-then-working-dir merge through the injected path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EfyrkJkeo3mRngVoppJjUN
Signed-off-by: Erik Sundell <erik.sundell+2025@sensmetry.com>
@consideRatio

Copy link
Copy Markdown
Collaborator Author

🤖 Drafted by Claude Code, reviewed and approved by Erik before posting.

Isn't every integration test that touches indexes or custom sources affected by this? Do they all pass --no-config?

Good instinct to check, but they're already covered — the exposure was narrower than it looks.

The real ~/.config/sysand/sysand.toml is only reachable through load_configs at sysand/src/lib.rs:177, which is gated by --no-config (it substitutes Config::default() and never reads the user config dir). And every CLI integration test spawns the binary through a single helper, sysand_cmd_in_with in sysand/tests/common/mod.rs, which unconditionally injects --no-config:

let args = args
    .into_iter()
    .chain(["--no-config"])
    .chain(cfg.iter().flat_map(|cfg| ["--config-file", cfg]));

cargo_bin!("sysand") appears only in that helper — no test constructs the command directly (the only other Command::new calls spawn git). So all the index/source suites (cli_add_remove, cli_source, cli_publish, cli_lock, cli_sync, cli_info, cli_build, …) already get --no-config, and tests that need config pass it explicitly via --config-file. The core/tests/* integration tests call the library functions directly and never go through load_configs at all.

That's exactly why the unit test was the one exposure: it called the library load_configs directly, where there's no --no-config escape hatch. load_configs_from(None, …) is the library-level equivalent of what the CLI suite already relies on.

One fair follow-up your question surfaces: isolation for the whole CLI suite rests entirely on that single --no-config injection, with no HOME/XDG_CONFIG_HOME sandbox as a backstop — so a future test that bypassed the helper would silently read the real user config. Happy to add temp-dir env sandboxing to the helper as defense-in-depth, but I'd keep it to a separate PR rather than widen this one.

@consideRatio
consideRatio force-pushed the fix/load-configs-test-isolation branch from b013e8c to 0986dc6 Compare July 20, 2026 21:53
Comment thread core/src/config/local_fs.rs Outdated
Comment thread core/src/config/local_fs_tests.rs Outdated
Comment thread core/src/config/local_fs_tests.rs Outdated
consideRatio and others added 2 commits July 21, 2026 07:45
Co-authored-by: Andrius Pukšta <andrius.puksta@sensmetry.com>
Signed-off-by: Erik Sundell <erik.i.sundell@gmail.com>
The `wrapfs::write` line from the GitHub-applied review suggestion
exceeded the line width; `cargo fmt` wraps it. No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPfB9S7WwNyqx6xsfTwKmF
Signed-off-by: Erik Sundell <erik.sundell+2025@sensmetry.com>
andrius-puksta-sensmetry
andrius-puksta-sensmetry approved these changes Jul 21, 2026
@consideRatio
consideRatio marked this pull request as ready for review July 21, 2026 07:35
@consideRatio
consideRatio merged commit c7a2a53 into sensmetry:main Jul 21, 2026
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants