Skip to content

test(core): stop output-format global leaking across tests - #93

Open
GregHolmes wants to merge 3 commits into
mainfrom
fix/output-format-test-isolation
Open

test(core): stop output-format global leaking across tests#93
GregHolmes wants to merge 3 commits into
mainfrom
fix/output-format-test-isolation

Conversation

@GregHolmes

@GregHolmes GregHolmes commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Problem

BaseCommand.output_result reads the output format from the process-global deepctl_core.output._output_config (via get_output_format()), not from config.get(). But five TestBaseCommand.test_output_result_* JSON tests only stubbed config.get.return_value = "json" — a value the code never reads.

As a result those tests depended on an earlier CLI-invoking test having left the global on "json" (the CLI entrypoint src/deepctl/main.py calls setup_output(), which mutates the global and nothing resets it). They pass in the full suite by collection-order luck and fail in isolation:

# on main
$ pytest packages/deepctl-core/tests/unit/test_base.py
5 failed, 83 passed

In isolation the format is its default "default", so output_result returns early and _output_json / print_json is never called → the assertions fail.

Fix (both halves)

  1. Tests set what the code reads. The 5 JSON tests now @patch("deepctl_core.output._output_config", {...}) — the same pattern the yaml/table/csv tests already use — instead of the no-op config.get stub. All 11 format patch sites now spread a shared _OUTPUT_CONFIG_DEFAULTS constant ({**_OUTPUT_CONFIG_DEFAULTS, "format": "json"}) that mirrors the real global's full key set, so the stand-in can't drift from _output_config (previously the dicts omitted agentic).
  2. Stop the leak. New autouse fixture in packages/deepctl-core/tests/conftest.py restores the pristine _output_config around every core test, so a format set by one test can no longer bleed into the next.

Half 1 makes the tests correct on their own; half 2 keeps the latent ordering bug from silently returning.

Verification

# in isolation (was 5 failed)
$ pytest packages/deepctl-core/tests/unit/test_base.py   → 88 passed
# whole core unit dir (was 5 failed)
$ pytest packages/deepctl-core/tests/unit                → 354 passed
# full suite
$ pytest                                                 → 976 passed

ruff / format clean (only test files touched). No src/ changes.

Note on the diff

Includes ~90 lines of ruff format reflow in TestConfirmPromptGating / TestIsGuided (the with patch(A), patch(B): blocks became parenthesized with ( … ): form, plus one line-wrap). All behavior-neutral formatter output, no logic change — called out here so it doesn't read as unexplained scope.

Scope

Pre-existing core test hygiene — independent of #92. Reset fixture is intentionally core-scoped to limit blast radius; a root-level version (covering cross-package leakage) can follow if wanted.

🤖 Generated with Claude Code

output_result reads the format from the process-global
deepctl_core.output._output_config (via get_output_format), not from
config.get(). But the five TestBaseCommand.test_output_result_* JSON tests
only stubbed config.get.return_value = "json" — a no-op the code never
reads — so they depended on an earlier CLI-invoking test having left the
global on "json". They passed in the full suite by collection-order luck and
failed in isolation (default format "default" → output_result returns early →
_output_json / print_json never called).

Fix both halves:
- Patch deepctl_core.output._output_config in each JSON test (matching the
  yaml/table/csv tests) so they set the format the code actually reads.
- Add an autouse fixture in packages/deepctl-core/tests/conftest.py that
  restores the pristine _output_config around every core test, so a format
  set by one test can no longer leak into the next.

test_base.py in isolation: 5 failed -> 88 passed. Full core suite: 354 passed.
…ests

The 11 format-test patch dicts stood in for the process-global
deepctl_core.output._output_config but carried only 4 of its 5 keys
(agentic was missing). Safe on today's path — output_result ->
get_output_format reads only ["format"] and _output_json prints directly —
but other output.py helpers read _output_config["agentic"], so a future
refactor routing output through one of them would KeyError every 4-key-dict
test at once.

Add a shared _OUTPUT_CONFIG_DEFAULTS constant mirroring the real global's
full key set and spread it at each site ({**_OUTPUT_CONFIG_DEFAULTS,
"format": "json"}), removing both the drift risk and the duplication.

No behavior change: test_base.py 88 passed in isolation, full suite 976.

@dg-coreylweathers dg-coreylweathers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved — verified end-to-end, both rounds.

Re-ran the whole story locally rather than trusting the PR body:

  • Reproduced the bug on main: test_base.py in isolation → 5 failed / 83 passed, exactly the five output_result JSON tests, failing on the "default" early-return. Confirmed in source that output_result reads get_output_format()_output_config["format"], so the old config.get stubs were no-ops as described.
  • On this branch: isolation 88 passed, core unit dir 354 passed, ruff format --check clean, no new ruff check findings vs main.
  • The autouse reset fixture is correct: pristine snapshot taken at collection time before any test runs, in-place clear()/update() composes cleanly with the tests' @patch swaps, no overlap with the root conftest.
  • f1469c1's _OUTPUT_CONFIG_DEFAULTS spread resolves the missing-agentic-key shape drift across all 12 format tests, and the PR body now accounts for the ruff-format reflow.

Two non-gating notes for the record:

  1. Full suite here is 975 passed + 1 pre-existing unrelated failure: test_file_info_modified_timestamp (deepctl-shared-utils) uses local-time fromtimestamp() and fails on any machine west of UTC — that's the 975-vs-976 gap between our machines. Deserves its own issue (fromtimestamp(ts, tz=timezone.utc)).
  2. Core-scoped reset fixture accepted as-is; root-level version can follow if cross-package leakage ever shows up.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants