Refactor tests #6

2026-05-11T16:02:18-04:00

didericis commented

2026-05-11 16:02:18 -04:00

Summary

The current test suite has a bunch of low-value tests and a structure that gets in the way. Things that motivated this:

Several tests assert on the shape of rendered string output (YAML greps in test_pipelock_yaml.py, stdout regex in test_dry_run_plan.py) instead of on the structured data behind that output. Brittle in both directions — passes when it shouldn't, breaks on unrelated formatting changes.
A few tests pin the wording of error messages (test_manifest_runtime.py), which catches copy edits rather than real regressions.
test_pipelock_sidecar_smoke.py hand-rolls the docker create → cp → start sequence in parallel with what cli.py start already does, so divergences in production code don't fail it.
test_pipelock_image.py is really an upstream-packaging canary and shouldn't run on every dev push.
tests/run_tests.py exists mostly to maintain a hand-curated INTEGRATION_NAMES set — that classification should fall out of the directory layout, not a list someone has to remember to update.

Goal: tighten the structure (split tests/unit/ vs tests/integration/, drop the bespoke runner), and replace the low-value tests with ones that actually catch regressions — driving production code paths and asserting on structured data.

## Summary The current test suite has a bunch of low-value tests and a structure that gets in the way. Things that motivated this: - Several tests assert on the *shape of rendered string output* (YAML greps in `test_pipelock_yaml.py`, stdout regex in `test_dry_run_plan.py`) instead of on the structured data behind that output. Brittle in both directions — passes when it shouldn't, breaks on unrelated formatting changes. - A few tests pin the wording of error messages (`test_manifest_runtime.py`), which catches copy edits rather than real regressions. - `test_pipelock_sidecar_smoke.py` hand-rolls the `docker create` → `cp` → `start` sequence in parallel with what `cli.py start` already does, so divergences in production code don't fail it. - `test_pipelock_image.py` is really an upstream-packaging canary and shouldn't run on every dev push. - `tests/run_tests.py` exists mostly to maintain a hand-curated `INTEGRATION_NAMES` set — that classification should fall out of the directory layout, not a list someone has to remember to update. Goal: tighten the structure (split `tests/unit/` vs `tests/integration/`, drop the bespoke runner), and replace the low-value tests with ones that actually catch regressions — driving production code paths and asserting on structured data.

didericis added 1 commit 2026-05-11 16:02:18 -04:00

chore(test): open refactor-tests branch

test / run tests/run_tests.py (pull_request) Successful in 14s

Details

83fe5741f5

Empty commit to seed the branch so a PR can be opened against main.
Actual test refactor work will follow.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

didericis added 4 commits 2026-05-11 16:25:46 -04:00

test: reorganize suite into unit/integration/canaries directories 4462863d56

Replace the hand-maintained INTEGRATION_NAMES classifier (and the
bespoke run_tests.py around it) with a directory-driven split:

  tests/unit/         unit tests, always run
  tests/integration/  Docker-dependent, skip cleanly without Docker
  tests/canaries/     upstream-regression checks, opt-in via
                      CLAUDE_BOTTLE_RUN_CANARIES=1

The pinned-pipelock-image check moves to the canary suite — it tests
upstream packaging, not our code, so it shouldn't gate every dev push.
A scheduled canaries.yml workflow runs it weekly.

The manifest-runtime tests collapse the four assertRaises cases for
distinct 'runtime' values into one subTest loop and drop the
error-message-wording assertions; the contract is "any value is
rejected", not "the error literally contains 'auto-detect'".

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

refactor(pipelock): expose structured config; assert on dict in tests 30b4f12288

Split pipelock config building from YAML rendering: pipelock_build_config
returns a dict, pipelock_render_yaml serializes it, and _build_pipelock_yaml
chains the two onto disk. Unchanged behavior — pipelock loads the same YAML.

The yaml test now asserts on the structured config dict, which is
robust to cosmetic YAML changes (key order, quoting). The two checks
that only make sense on the rendered output — file mode 0600 and
no-secret-leakage — stay against the on-disk content.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

feat(cli): add --format=json to start --dry-run for machine-readable plan beb0c9d58f

BottlePlan gains a to_dict method (abstract on the base, implemented
on DockerBottlePlan) returning a JSON-serializable view of the resolved
plan. `cli.py start --dry-run --format=json` prints it to stdout and
exits zero. --format=json without --dry-run is rejected — emitting JSON
during a real launch would race the y/N prompt.

The dry-run integration test now parses the JSON and asserts on
structured fields (agent, bottle, runtime, hosts sorted+deduped, etc.)
instead of regex-matching the human-readable preflight stdout. That
kills the magic-"8 hosts allowed" coupling — adding a new baked
default doesn't break the test.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

test(pipelock): drive sidecar smoke through production prepare/start

test / unit (pull_request) Successful in 14s

Details

test / integration (pull_request) Successful in 23s

Details

8f5e07af7f

The old smoke test hand-rolled the docker create/cp/start sequence in
parallel with what DockerPipelockProxy.start already does, so any
divergence in production code wouldn't trip it. Rewritten to call
.prepare and .start directly and probe /health from a sibling curl
container on the same internal network — same access topology the
agent container uses in production.

In-network probing means the test no longer depends on a published
port, so it can run under act_runner (where host-loopback port
publishing isn't reachable from the job container).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

didericis added 2 commits 2026-05-11 16:36:09 -04:00

test(cli): tighten and relocate --format=json validation test 757e76add7

Move the --format=json-requires-dry-run check out of the integration
suite (it doesn't need Docker — argparse fails before any backend
runs) and tighten the assertion: previously asserted only that exit
code was nonzero, so any unrelated breakage (manifest resolution
failure, bad agent name, etc.) silently passed. Now asserts stderr
contains the actual flag-conflict message.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

test(pipelock): collapse over-decomposed allowlist helper tests

test / unit (pull_request) Successful in 11s

Details

test / integration (pull_request) Successful in 21s

Details

479adc625a

The four lower-level helpers (pipelock_bottle_allowlist,
pipelock_bottle_ssh_hostnames, pipelock_bottle_ssh_ip_cidrs,
pipelock_bottle_ssh_trusted_domains) are one-line filters; testing
each in isolation duplicates coverage that pipelock_effective_allowlist
already provides end-to-end. The /32 CIDR suffix is the only behavior
beyond filtering, so it keeps a tiny dedicated test.

Drops the misplaced test_rejects_non_string_entry — that's manifest
validation, not allowlist resolution. Belongs in a manifest-validation
test file (which doesn't exist yet); leaving for a separate PR rather
than adding a one-branch sample here.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

didericis added 1 commit 2026-05-11 16:50:26 -04:00

refactor(pipelock): take stage_dir, derive yaml_path internally

test / unit (pull_request) Successful in 11s

Details

test / integration (pull_request) Failing after 12s

Details

f943e14891

PipelockProxy.prepare now accepts (bottle, slug, stage_dir) and derives
the yaml_path itself, so callers don't need to know the filename.
DockerBottleBackend.prepare_proxy becomes a one-line wrapper whose only
caller already has bottle and slug in scope, so it's inlined and
deleted.

didericis added 1 commit 2026-05-11 19:24:37 -04:00

test(pipelock): skip sidecar smoke under act_runner

test / unit (pull_request) Successful in 13s

Details

test / integration (pull_request) Successful in 14s

Details

7fb0b8488b

The smoke test now drives the production prepare/start path, which
calls network_create_internal. Under Gitea act_runner the docker
socket mount topology makes `docker network create --internal` fail
(or be invisible across the host/job-container boundary) — the same
limitation that test_orphan_cleanup.test_create_and_remove already
skips for. Match that skip here so CI goes green; the test still
runs in environments with a direct docker daemon.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

didericis merged commit b1001ba1b8 into main

2026-05-11 19:26:28 -04:00

didericis referenced this issue from a commit

2026-05-24 21:46:26 -04:00

docs(prd-0011): drop the migration command requirement

~~didericis-claude referenced this pull request 2026-06-02 00:12:12 -04:00~~

Complexity hotspots in launch, egress, and auth paths #117

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: didericis/bot-bottle#6