Skip to content

fix(tests): preserve config module identity in backup tests - #73272

Closed
frizikk wants to merge 1 commit into
NousResearch:mainfrom
frizikk:fix/test-config-state-leak
Closed

fix(tests): preserve config module identity in backup tests#73272
frizikk wants to merge 1 commit into
NousResearch:mainfrom
frizikk:fix/test-config-state-leak

Conversation

@frizikk

@frizikk frizikk commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes an order-dependent test-isolation failure in the CLI suite. The pre-update-backup fixture removed hermes_cli.config and hermes_constants directly from sys.modules. Other collected tests retained imports from the old config module, while string-based patches resolved the newly imported module, so the patched sanitizer was not called.

The fixture now relies on Hermes' path/mtime-keyed config caches instead of replacing shared module objects.

Related Issue

Fixes #73144

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • Removed direct sys.modules eviction from TestRunPreUpdateBackup setup and mode configuration in tests/hermes_cli/test_backup.py.
  • Kept the backup tests' HERMES_HOME and Path.home() isolation intact.
  • Restored compatibility with TestSanitizeEnvLines.test_migrate_reports_normalized_line_formatting when it runs after the backup test.

How to Test

  1. On unmodified current main, run:
    pytest \
      tests/hermes_cli/test_backup.py::TestRunPreUpdateBackup::test_default_creates_quick_snapshot_only \
      tests/hermes_cli/test_config.py::TestSanitizeEnvLines::test_migrate_reports_normalized_line_formatting \
      -q -o 'addopts='
    This produces 1 passed, 1 failed: the config test captures no normalization message.
  2. Apply this change and rerun the same command: 2 passed.
  3. Run the focused suites:
    scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/hermes_cli/test_config.py -q
    Result: 350 passed.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run pytest tests/ -q and all tests pass (the wider CLI suite has unrelated existing failures; targeted canonical suites pass)
  • I've added/retained regression coverage through the exact ordering repro
  • I've tested on Linux

Documentation & Housekeeping

  • Documentation N/A — test-isolation-only change
  • cli-config.yaml.example N/A
  • CONTRIBUTING.md / AGENTS.md N/A
  • Cross-platform impact N/A — Python test-process isolation only
  • Tool descriptions/schemas N/A

Screenshots / Logs

Focused validation:

163 passed
350 passed
ruff check: All checks passed

@teknium1

Copy link
Copy Markdown
Contributor

Merged via the round-3 stability PR #74576 (commit 57cd922, your authorship preserved). Root fix confirmed: backup+config in one process now 114 green — this was the correct diagnosis of issue #73144 where the earlier attempt (#73304) treated the symptom. Thanks @frizikk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have type/test Test coverage or test infrastructure

Projects

None yet

3 participants