Skip to content

Remove mock-awareness from shipped code (production branches on isinstance(..., Mock)) #116

Description

@jeremymanning

Part of #108 · Phase 2 · This is the highest-value structural fix in the plan.

Problem

Shipped code in clustrix/ knows whether it is being tested and behaves differently.

1. The SLURM status checker branches on Mock

clustrix/executor_scheduler_status.py:89:

from unittest.mock import Mock
is_mock = isinstance(self.connection_manager.ssh_client, Mock) ...
if ... and not is_mock:
    return self._check_slurm_job_status_robust(job_id, active_jobs)
else:
    # Fallback to original logic for unit tests

The comment reads "Use robust checking only if we have a real SSH connection (not unit tests)".

Consequence: _check_slurm_job_status_robust — the code that actually ships to users — is structurally unreachable from any unit test. Every SLURM status test exercises the dead fallback branch instead. The tests are green; the shipped path has never been executed by them.

2. Fake widgets ship in the package

clustrix/notebook_magic_mocks.py defines _MockDropdown, _MockButton, observe(): pass, display(): pass as the ImportError fallback for ipywidgets/IPython. It is imported by six production modules: notebook_magic.py:53, notebook_magic_core.py:15, notebook_magic_widget.py:28, notebook_magic_aws.py:17, notebook_magic_azure.py:17, notebook_magic_gcp.py:18.

So widget code "works" against fake widgets instead of failing loudly when the real dependency is absent — which is precisely the "fallback system" the project's rules prohibit: "Never use mock objects or tests, even as fallback systems — if real functionality doesn't work... they should raise an exception or fail."

Acceptance criteria

  • grep -rn "unittest.mock\|MagicMock" clustrix/ returns zero hits
  • _check_slurm_job_status_robust is the only SLURM status path; the "original logic for unit tests" fallback is deleted
  • A test exercises the robust path against a real (or genuinely simulated-at-the-socket) SSH endpoint, not a Mock
  • notebook_magic_mocks.py is deleted. Missing ipywidgets/IPython raises a clear, actionable ImportError naming the extra to install (pip install clustrix[widget]) rather than silently degrading
  • A CI check fails the build if unittest.mock is ever imported from clustrix/

Why this is first in Phase 2

Every other de-mocking effort is undermined while production code retains a mock-detecting branch: you can delete a thousand mock assertions and still be testing the wrong code path. Fix the code's awareness of tests before fixing the tests.

Verification

grep -rn "unittest.mock\|MagicMock\|isinstance(.*Mock" clustrix/    # must be empty
python -c "import clustrix.notebook_magic_mocks"                     # must ModuleNotFoundError

Activity

  1. added
    P1-highRequired for production readiness
    testingTest suite, CI, coverage
    tech-debtDead code, duplication, refactoring
    on Aug 17, 2026
  2. jeremymanning commented on Aug 20, 2026

    @jeremymanning
    MemberAuthor

    Fixed — the criterion this issue set is met

    The issue's own test is that this grep must be empty:

    $ grep -rn "unittest.mock\|MagicMock\|isinstance(.*Mock" clustrix/
    (no output)
    

    It is. Production code no longer branches on whether it is being tested.

    The guard, and an honest note about counting it

    tests/unit/test_no_mocks_in_shipped_code.py enforces it, and the guard names the forbidden patterns as data:

    FORBIDDEN_MODULES = frozenset({"mock", "unittest.mock", "pytest", "_pytest"})

    That has a consequence worth recording, because it will confuse the next person who counts: the guard file itself matches the repository's mock-usage grep, while importing no mock at all (grep -nE "^\s*(import|from)\s+.*mock" finds nothing in it).

    So CLAUDE.md's mock-using module count reads 21 of 166 rather than its recorded 20 of 152 — one of which is this guard, and the denominator grew as tests were added. Neither number indicates a regression. CLAUDE.md already says to recount before quoting the figure, which is the right instinct; the guard is simply uncountable by the metric it enforces.

    What this issue does not close

    #117 remains open. Replacing assertion-free mock tests is a separate and much larger job — 21 of 166 test modules still use unittest.mock, and that is its criterion, not this one's.

  3. jeremymanning commented on Aug 22, 2026

    @jeremymanning
    MemberAuthor

    Verified on the merged v0.2.0 tree (f2a7205, all six work branches merged):

    $ grep -rn "unittest.mock\|MagicMock\|isinstance(.*Mock" clustrix/ --include="*.py" | wc -l
    0
    

    Production code no longer branches on whether it is being tested. Guarded by tests/unit/test_no_mocks_in_shipped_code.py. Full non-billable suite on the merged tree: 2760 passed / 0 failed.

  4. added 2 commits that reference this issue on Aug 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P1-highRequired for production readinesstech-debtDead code, duplication, refactoringtestingTest suite, CI, coverage

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions