Skip to content

Commit b5ed5a4

Browse files
Rogan-Inglisclaude
Rogan-Inglis
and
claude
authored
test(bash_arena): stop pure-logic tests paying for Docker prebuild (#848)
## Summary Cuts the CI unit-test job from **4m35s to 59s**. Independent of my other open PRs — off `main`, mergeable on its own. `control_arena/settings/bash_arena/tests/conftest.py` pre-builds Docker images and downloads a HuggingFace dataset from a **`scope="session", autouse=True`** fixture, so *every* test in that directory pays for it. But `test_scorer.py` is five pure-arithmetic assertions over `build_main_task_value` that never start a container — and it carries no markers, so it runs in the standard unit job. `--durations` attributed **241s of a 275s run** to the setup of a single one of those assertions: **88% of the wall clock**, spent building Docker images for tests that execute in 0.03s. ### Why not just mark them `long_running` That was the first instinct, but it moves the cost rather than removing it — the same 241s would still be paid, just in the other job — and it mislabels five millisecond-long tests as long-running. The actual fault is `autouse=True` imposing a container build on tests that never use a container. So the fixture is no longer autouse; `test_side_tasks.py`, which *does* run real sandboxes and is already `long_running`, requests it explicitly: ```python pytestmark = pytest.mark.usefixtures("prebuild_bash_arena_images") ``` The xdist file-lock coordination in the fixture is untouched. ### Also: documents the conventions that let this happen `CONTRIBUTING.md`'s "Running Tests" section listed two pytest commands without saying what the markers mean or when to reach for them — which is how an unmarked test came to cost four minutes of every CI run. It now covers: - What `long_running` and `skip_ci` each mean, and that they map to separate CI jobs. In particular that **`skip_ci` excludes a test from *both***, so a `skip_ci` test never runs automatically and a mock model is usually the better option. - The fixture-cost trap behind this PR: an autouse fixture is charged to every test in its directory, and `--durations` reports that time under `setup` rather than `call`, so it looks like a slow test rather than a slow fixture. Plus `--setup-plan` for checking fixture wiring without running the slow part. - `-n 8`, matching CI. Its absence makes the suite several times slower than it needs to be. ## Test Plan ```bash uv run pytest -n 8 -m "not skip_ci and not long_running" ``` - **Before:** 275s (4m35s), with `241.11s setup .../test_scorer.py::test_build_main_task_value_full_pass_default_threshold` topping the durations. - **After:** 59s, 881 passed, 288 skipped, and **no bash_arena entry in the slowest durations at all**. The scorer tests alone go from ~241s to `5 passed in 0.03s`. Verified the fixture wiring with `--setup-plan`, which shows the closure without executing anything: ```bash # still present for every test that needs a sandbox $ uv run pytest .../test_side_tasks.py -m long_running --setup-plan -q | grep prebuild SETUP S prebuild_bash_arena_images (fixtures used: tmp_path_factory, worker_id) ...::test_disable_firewall (fixtures used: ..., prebuild_bash_arena_images, ...) # and absent from the pure-logic tests $ uv run pytest .../test_scorer.py --setup-plan -q | grep -c prebuild 0 ``` I also ran one of the `long_running` side-task tests end to end against real Docker (`test_disable_firewall`, 1 passed in 5m30s), so the prebuild still does its job where it is needed. Independent corroboration from CI: this PR's Unit Tests job runs in **2m29s** against 4m24s on a branch differing only by this change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent e5ffa80 commit b5ed5a4

3 files changed

Lines changed: 59 additions & 7 deletions

File tree

‎CONTRIBUTING.md‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,48 @@ uv sync --dev
3939
### Running Tests
4040

4141
```bash
42-
# Run unit tests (fast)
43-
uv run pytest -m "not skip_ci and not long_running"
42+
# Run unit tests (fast) -- this is what CI runs
43+
uv run pytest -n 8 -m "not skip_ci and not long_running"
4444

4545
# Run all tests including long-running ones
46-
uv run pytest -m "not skip_ci"
46+
uv run pytest -n 8 -m "not skip_ci"
47+
48+
# Run just the area you are working on, which is usually seconds
49+
uv run pytest -n 8 control_arena/tools control_arena/monitor
50+
```
51+
52+
Pass `-n 8` to run tests in parallel with `pytest-xdist`, as CI does. Without it the suite is several times slower.
53+
54+
#### Test markers
55+
56+
Two markers control where a test runs, and CI has a job for each:
57+
58+
- **`long_running`** -- slow tests, typically because they start containers or run a real evaluation. Run separately from the unit tests so a pull request gets fast feedback.
59+
- **`skip_ci`** -- tests that cannot run in CI at all, usually because they need API credentials. Note that `skip_ci` excludes a test from **both** jobs, so a test marked `skip_ci` never runs automatically. Prefer a mock model (`mockllm/model`) over `skip_ci` where you can: it needs no credentials and usually runs in under a second.
60+
61+
Unmarked tests run in the fast unit job, so anything unmarked should be quick.
62+
63+
#### Watch what your fixtures cost
64+
65+
A slow fixture is charged to whichever tests request it, so an `autouse` fixture charges *every* test in its directory whether or not the test needs it. That is easy to miss, because the tests themselves look fast.
66+
67+
Prefer making an expensive fixture opt-in and requesting it only from the tests that need it:
68+
69+
```python
70+
# In the test module that actually starts a sandbox
71+
pytestmark = pytest.mark.usefixtures("prebuild_docker_images")
72+
```
73+
74+
`--durations` reports fixture time under `setup`, which is the quickest way to find this:
75+
76+
```bash
77+
uv run pytest -n 8 -m "not skip_ci and not long_running" --durations=10
78+
```
79+
80+
`--setup-plan` shows which fixtures a test would use without running anything, which is useful for checking wiring without waiting for the slow part:
81+
82+
```bash
83+
uv run pytest path/to/test_file.py --setup-plan
4784
```
4885

4986
### Code Quality

‎control_arena/settings/bash_arena/tests/conftest.py‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
1-
"""Pre-build Docker images before bash_arena tests to avoid concurrent build races with xdist."""
1+
"""Pre-build Docker images for the bash_arena tests that need them.
2+
3+
Deliberately not autouse: the build downloads a HuggingFace dataset and runs two
4+
Docker builds, taking around four minutes. Tests that need a sandbox opt in with
5+
`pytest.mark.usefixtures("prebuild_bash_arena_images")`, so the pure-logic tests
6+
in this directory do not pay for it.
7+
"""
28

39
import subprocess
410

@@ -42,14 +48,18 @@ def _prebuild_task_images() -> None:
4248
subprocess.run(compose_cmd, check=True, text=True, timeout=600) # noqa: S603
4349

4450

45-
@pytest.fixture(scope="session", autouse=True)
51+
@pytest.fixture(scope="session")
4652
def prebuild_bash_arena_images(
4753
tmp_path_factory: pytest.TempPathFactory, worker_id: str
4854
) -> None:
4955
"""Pre-build Docker images for bash_arena tests.
5056
51-
Uses a file lock to ensure only one xdist worker performs the build.
52-
Other workers wait for the build to complete before proceeding.
57+
Uses a file lock to ensure only one xdist worker performs the build. Other
58+
workers wait for the build to complete before proceeding.
59+
60+
Request this from any test that runs a bash_arena sandbox. It is not autouse
61+
because it costs roughly four minutes, which tests that never start a
62+
container should not be paying.
5363
"""
5464
if worker_id == "master":
5565
# Not running with xdist

‎control_arena/settings/bash_arena/tests/test_side_tasks.py‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@
1818
from control_arena.settings.bash_arena.task.dataset import DatasetSpec
1919

2020
# Shared eval config for tests to avoid API key requirements
21+
# These tests run real sandboxes, so they need the Docker images pre-built. The
22+
# fixture is session-scoped and not autouse, so it is requested here rather than
23+
# being imposed on every test in this directory.
24+
pytestmark = pytest.mark.usefixtures("prebuild_bash_arena_images")
25+
2126
MOCK_EVAL_CONFIG = ControlEvalConfig(
2227
trusted_model="mockllm/mockllm",
2328
untrusted_model="mockllm/mockllm",

0 commit comments

Comments
 (0)