ContextLab / ContextLab/clustrix

Replace assertion-free mock tests with tests that actually execute (2,513 mock occurrences)

Open
#117 2 comments 0 reactions 0 assignees View on GitHub
P1-high testing
Dominant language
Python
Stars
10
Forks
4
Avg merge
6h 27m
Merged PRs (30d)
9

Description

Part of #108 · **Phase 2** · Depends on the production-mock-removal issue

## Problem

**60 of 240 test files use mocks; 2,513 total occurrences** of `unittest.mock|MagicMock|AsyncMock|@patch|patch(|monkeypatch|Mock(`. Three of them are inside `tests/real_world/`, defeating the purpose of that directory entirely.

The project's own rules state: *"Do not use mock services for anything ever"* and *"All tests need to use 'real' function calls."*

## Representative offenders — tests that mock exactly the thing they claim to test

1. **`tests/test_executor_schedulers.py:73-131` `test_submit_slurm_job_basic`** — patches `tempfile.NamedTemporaryFile`, `pickle.dump`, `os.unlink`, `setup_remote_environment`, `create_job_script`, *and* stubs `connection_manager.execute_remote_command` to return `"Submitted batch job 12345"`. Then asserts `job_id == "12345"`. **It tests a regex against a string the test itself supplied.** No SLURM is involved. Same pattern at `:139`, `:212`, `:262`, `:321`, `:409`.
2. **`tests/test_executor.py:79 test_execute_command`** — `@patch("paramiko.SSHClient")`, sets `mock_stdout.read.return_value = b"command output"`, asserts `stdout == "command output"`. Asserts a mock returns what it was told to return.
3. **`tests/test_ssh_automation.py:123 test_validate_ssh_key_success`** — mocks `paramiko.SSHClient`, asserts `validate_ssh_key(...) is True`. **No key is ever validated.**
4. **`tests/test_filesystem.py:205 test_remote_ls`** — mocks SSH, feeds `b"file1.txt\nfile2.py\nsubdir/\n"`, asserts `ls()` returns those three names. Tests `str.split` and `rstrip`.
5. **`tests/test_cloud_providers_aws.py:42-44 test_authenticate_success`** — patches `BOTO3_AVAILABLE = True` *and* `boto3`. **Passes on a machine with no boto3 installed at all.**
6. **`tests/test_kubernetes_integration.py:31`** — fixture patches `kubernetes.client` wholesale; `create_namespaced_job` returns a `Mock` whose `.metadata.name` is `"test-job-123"`. No cluster, no API.
7. **`tests/test_executor_connections.py:72,103,144,190,215,234,253,279,295`** — every connection test patches `paramiko.SSHClient`. The connection manager is never tested against a socket.

## The standard to replace them with

For each subsystem, in preference order:

1. **Real execution against a real target.** SSH/SLURM/PBS/SGE -> a container or the HF Jobs substrate. Filesystem -> a real temp dir and a real SFTP session.
2. **Real local server.** Spin an actual sshd in a container; connect a real socket. This is what `test_executor_connections.py` should do.
3. **Recorded real interactions** (e.g. `vcrpy` for pricing APIs) — recorded once from a genuine call, per the project's "verify with real calls, then record" rule for paid APIs.
4. **Delete.** A test that only asserts a mock's configured return value has negative value: it costs maintenance and creates false confidence. Deleting it *raises* the quality of the suite.

## Acceptance criteria

- [ ] `grep -rn "unittest.mock" tests/real_world/` returns zero — that directory is real-only by definition
- [ ] Every test named in the list above is either rewritten against a real target or deleted with a stated reason
- [ ] Mock occurrences in `tests/` reduced by at least an order of magnitude from 2,513
- [ ] Each supported backend has at least one test that submits a real job and asserts on a real returned value
- [ ] No test asserts equality against a string the same test supplied to a stub

## Note

Expect the pass count to **drop** as this lands, and expect real bugs to surface. That is the point — the cloud-path `KeyError` in Phase 3 survived precisely because tests mocked past it.

Contributor guide

Open the contributing guide

Research direction

Start with the listed offenders in tests/test_executor_schedulers.py, tests/test_executor.py, tests/test_ssh_automation.py, tests/test_filesystem.py, tests/test_cloud_providers_aws.py, tests/test_kubernetes_integration.py, and tests/test_executor_connections.py; inspect the existing fixtures and real-world test setup. Use the acceptance criteria to choose a narrowly scoped subsystem, then verify that tests execute against real targets or are deleted with a stated reason and that tests/real_world/ contains no mocks.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
distributed-systems, testing-qa
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.