From b602de8e7b98e0d4909ef74236014573ef5df0b8 Mon Sep 17 00:00:00 2001 From: Viorel Munteanu Date: Fri, 4 Sep 2026 09:58:35 +0000 Subject: [PATCH] Document the new Catch2 test suite README.md: new "Testing" section -- the 4 category/tag-expression table, what meson test covers vs. what stays manual, and tests/setup-tests.py. CLAUDE.md: rewrote the self_test.{h,cpp} entry to describe its new role (pure Catch2 Session::run() plumbing, ENABLE_TESTS-guarded) instead of the hand-rolled tests it used to contain directly, and added a full per-file breakdown of the new tests/unit, tests/integration, and tests/support infrastructure -- including the real bugs found building it (the two parse_args()/getopt_long state-reset bugs, the ScratchXdgDirs mixed-iterator UB, the Catch2-inherited-SIGTERM-handler artifact, the missing /sys mount and spdlog-writes-to-stdout findings), all in the same narrative depth this file already uses throughout. Also updated the "Build & test commands" flag list and meson test description. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz --- CLAUDE.md | 211 ++++++++++++++++++++++++++++++++++++++---------------- README.md | 33 +++++++++ 2 files changed, 184 insertions(+), 60 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ceab5d6..40f7f9f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -301,64 +301,151 @@ Source layout (all under `src/`): right after the `stop_tap_relay()` loop) for `--clean-processes`'s own crash-orphan sweep (`network_tap_relay.h`'s `clean_stale_tap_relays()`, see below). -- `self_test.{h,cpp}` — `run_self_tests()` implements `-t/--test`, this - project's own built-in self-test mode (distinct from the Meson-driven - fixture smoke test under `tests/`, described in "Build & test commands" - below; previously reported `detect_bwrap_unshare_args()`'s output — - `bwrap.{h,cpp}` — unplugged since that's kernel-capability diagnostics, not - a test). Currently exercises `persistent_netns.{h,cpp}`'s (see below) - create/verify/remove cycle: skipped with a message (not a failure) when not - root, since `create_persistent_netns()` requires it for the bind mount. - Confirms the namespace is missing before creation, exists right after - (checked from this process, *after* the forked child that actually did the - `unshare()`/bind-mount has already exited — the actual claim being tested: - the namespace outlives its creating process), then gone again after - removal. Also exercises `network_tap_relay.{h,cpp}`'s (see below) - create/attach/teardown cycle, same root-only skip: a throwaway bridge - (`ip link add ... type bridge` inside a throwaway persistent namespace - of its own — `create_tap_relay()` now always enters a network's - persistent namespace first, both kinds, see `network_bridge.{h,cpp}`'s - own "Resolved: `extern` had no connectivity" entry, so this test needs - one too even though `network_bridge.h`'s own bridge *provisioning* logic - still isn't otherwise exercised here — a tap device only cares that - *some* bridge exists to attach to) stands in for a real network's bridge, - and a throwaway network namespace (a forked - child that `unshare(CLONE_NEWNET)`s then blocks in `pause()` until - signaled) stands in for a real `-r/--run` session's isolated one. **Real - race caught by testing, not assumed**: `fork()` returning to the parent - doesn't mean the child has actually reached its own `unshare(CLONE_NEWNET)` - call yet — the same race `network_join.cpp`'s own - `wait_for_isolated_net_namespace()` already guards against for a real - session; an earlier version of this test used the child's pid immediately - and the container-side tap device silently ended up created in the *host's* - namespace instead (confirmed: it wasn't visible via `nsenter` into the - child's namespace at all). Fixed the same way — polling (bounded, 1s, - 20ms interval) `namespace_isolated()` (`sandbox_process.h`) until the - child's own net namespace actually differs from this process's before - trusting its pid. Confirms the host-side tap gets created and attached to - the bridge (`ip link show` output contains `master `), the - container-side tap gets created with the requested name inside the target - namespace (checked via a plain, single-shot - `nsenter --net=/proc//ns/net -- ip link show`; an earlier version - wrapped this in a bounded retry, added when tap devices were still created - via bare `ioctl(TUNSETIFF)` and intermittently weren't immediately - visible — see `network_tap_relay.{h,cpp}`'s own entry below for why that - retry, and the whole class of symptom it was compensating for, is gone - now that devices are created persistently instead), and — **updated once - the original "both devices disappear on their own once `stop_tap_relay()` - stops the relay, no explicit `ip link del` needed" assumption turned out - to be wrong on the real target device** (`network_tap_relay.{h,cpp}`'s own - entry below has the full story) — now confirms the *host*-side device is - explicitly gone after `stop_tap_relay()` (the new `ip link del` step) - while the *container*-side device deliberately still exists (correctly - persistent — only the relay stopped, not the container's own network - namespace); the container-side device's actual disappearance, once that - namespace itself is torn down, isn't separately re-checked (`nsenter` has - nothing left to target once the namespace's only holding process has - already exited) — it's destroyed moments later anyway, at the very end of - this test, when its throwaway namespace-holder process is killed. - Deliberately its own small file since more real tests are expected here as - more of the networking feature lands. +- `self_test.{h,cpp}` — `run_self_tests(args)` implements `-t/--test` + (`args` is `ParsedArgs::test_args`, `cli_args.h` — everything on the + command line after `-t`, captured the same trailing-argv way `-r`/`-x` + capture their own command). Purely plumbing: builds a synthetic argv + (`{"slocker-lite -t"} + args`) and hands it straight to Catch2's + `Catch::Session().run(argc, argv)` — no test logic of its own lives here + at all, that's all under `tests/` (see below); this file is `#if + ENABLE_TESTS`-guarded (`config.h`, from Meson's `enable_tests` option, + default on — the same macro that already gated whether `catch2_dep` gets + linked at all) so a `-Denable_tests=false` build prints a clear "not + compiled into this build" message and returns nonzero instead of + failing to link. `-t`'s own leftover-args capture requires a literal + `--` before any Catch2 option that looks like one of slocker-lite's own + (`-r/--reporter` collides with `-r/--run`, `-c/--section` with + `-c/--cleanup`) — a bare tag expression like `-t -- "[unit]"` needs it + too by convention, though `getopt_long`'s own permutation happens to let + a `-t "[unit]"` without `--` work anyway, since `"[unit]"` doesn't start + with `-`. Distinct from the Meson-driven fixture smoke test under + `tests/` (`tests/gen_fixture.py`/`tests/run_test.py`, described in + "Build & test commands" below), which stays a separate, always-on, + Python-driven mount/unmount/cleanup check. + + **Test organization under `tests/`** (all in-process — every category + calls this project's own already-header-exposed functions directly, no + subprocess-spawning, no refactoring of `commands.cpp`'s file-local + functions needed; see `README.md`'s own "Testing" section for the + user-facing category table/tag-expression cheat sheet): + - `tests/unit/*.cpp` (`[unit]`) — one file per source area + (`test_port_forward.cpp`, `test_env_spec.cpp`, `test_network_subnet.cpp`, + `test_cli_args.cpp`) exercising pure/isolated functions with no side + effects: `parse_port_forward_spec()`, `resolve_env_specs()`, + `network_subnet.h`'s CIDR validation/overlap/allocation/address + arithmetic, and `parse_args()` itself against synthetic argv's. **Two + real bugs found running `parse_args()` repeatedly in one process** (never + possible before this suite existed — a real invocation only ever calls + it once, from `main()`): `getopt_long`'s own scanning position + (`optind`) is process-global and was never reset between calls, so a + second `parse_args()` call would silently resume wherever the first + left off; fixing that alone (`optind = 1`) wasn't enough either, since + `-h`/`-V` return out of the `getopt_long` loop early (their own + `return 0` case), before a call ever completes its scan and lets + `getopt_long` null out its own private `nextchar` pointer — the *next* + call then resumed scanning through that stale pointer into the + *previous* call's already-destroyed argv strings. Fixed with + `optind = 0` (not `1`) at the top of `parse_args()` itself — glibc + documents that value specifically as "fully reinitialize private state + before rescanning a new argv"; confirmed clean across repeated runs in + both random and deterministic (`--order lex`) Catch2 ordering. + - `tests/integration/test_config_bwrap_chain.cpp` (`[integration]`, no + net/root) — chains `config_file.h`'s read/write with `bwrap.h`'s argv + assembly: write a config file, load it back, resolve a + `NamespaceConfig` the same way `run_container()` does, confirm + `build_bwrap_args()`'s resulting argv actually reflects it (disabled + `unshare-net`/`unshare-uts` never requested; an all-default config + matches the live host's own `detect_bwrap_unshare_args()` probe + exactly). `build_bwrap_args()` is pure argv assembly given a `root` + that's just a string here, never accessed — no mounting, no privilege. + - `tests/integration/test_rootless_run.cpp` (`[integration][net]`, + rootless) — runs a real busybox image through `dispatch_command()` + (`commands.h`) itself, the exact real `-r/--run` path, in-process. + Confirms bwrap's *default* sandboxing (no `-n`/`-p` at all) is + genuinely isolating: a fresh net namespace with nothing but loopback, + and pid/uts/ipc namespaces differing from the test process's own. + **Real findings, not assumed**: bwrap's sandbox mounts `--proc /proc` + and `--dev /dev` but *not* `/sys` at all (`ls /sys/class/net` inside + the sandbox: "No such file or directory", reproduced via the real CLI + too, not just this test) — the loopback-only check instead reads + `/proc/net/dev` (two header lines + one `: ...` line per + interface). Also: spdlog's default sink writes to stdout, not stderr, + same as the plain `"mounted image at: ..."` success line — so a naive + stdout capture (`tests/support/fixtures.h`'s `CapturedStdout`, below) + mixes slocker-lite's own status/log output in with the sandboxed + command's real output; fixed by having the sandboxed command bracket + its own output between two unique markers and extracting only what's + strictly between them. + - `tests/integration/test_root_networking.cpp` (`[integration][root][net]`) + — the persistent-netns/tap-relay/dns-resolver tests that originally + lived directly in this file, ported to tagged `TEST_CASE`s (every + assertion uses `CHECK`, not `REQUIRE`, so a failure partway through + still reaches the same unconditional cleanup at the end — these + manage real host-side namespaces/bridges/tap devices that must not + leak just because an earlier assertion failed; simple `if`/pid guards + skip meaningless dependent steps instead). All of the real-bug + narrative originally written here — the `fork()`-vs-`unshare()` race + caught by `wait_for_isolated_net_namespace()`-style polling, the + persistent-tap-device redesign, the `ip link del` teardown fix — is + unchanged in substance, just now describing that file instead of this + one. **One further real bug found porting this to Catch2**: the + tap-relay test's own `create_tap_relay()` call forks a relay child + that relies on `SIGTERM`'s *default* disposition to terminate cleanly + once `stop_tap_relay()` signals it (`network_tap_relay.cpp`'s own + relay loop deliberately installs no handler) — but Catch2 installs its + own fatal-signal handler around a running `TEST_CASE`, which that + forked child inherits, so its ordinary shutdown signal got caught by + the *inherited* handler *in the child* instead, producing a spurious + "FAILED ... due to a fatal error condition: SIGTERM" report + interleaved into the real output (confirmed cosmetic only — exit code + and assertion count were correct either way). Fixed by resetting + `SIGTERM` to `SIG_DFL` just around the `create_tap_relay()` call and + restoring it right after — only the disposition *at fork time* is + inherited, so nothing about how long the relay then keeps running + matters. No production code changed for this; it's purely an artifact + of forking network primitives from within a Catch2-instrumented + process. + - `tests/support/fixtures.{h,cpp}` — `find_busybox_fixture()` (searches + `images/busybox.tar` relative to cwd, this project's own established + manual-testing convention; `nullopt` if absent, so `[net]` tests + `SKIP()` rather than fail — see `tests/setup-tests.py`, below), + `ScratchXdgDirs` (RAII: points `XDG_CONFIG_HOME`/`XDG_STATE_HOME` at a + fresh `mkdtemp()` directory for its lifetime, restoring the previous + environment and removing the directory on destruction, so integration + tests never touch the real developer's own config/state), and + `CapturedStdout` (RAII: redirects this process's own fd 1 — and + anything a forked/exec'd child inherits from it — to a throwaway temp + file for its lifetime). **Real bug found via ~10-30 repeated combined + `[unit]`+`[integration]` runs, not assumed**: `ScratchXdgDirs`'s + constructor originally built its `mkdtemp()` template vector from two + *separate* temporary `std::string` objects (`.begin()` off one, + `.end()` off the other) — mixing iterators from different containers + is undefined behavior, here manifesting as an intermittent, + heap-address-dependent `std::length_error: cannot create std::vector + larger than max_size()` inside whichever test happened to run adjacent + to it. Fixed by using a single named string instance for both ends of + the range. + - `tests/setup-tests.py` — idempotent fixture fetcher for + `images/busybox.tar`: does nothing if it already exists, otherwise + tries `skopeo` → `podman` → `docker` in that order (`skopeo`/`podman` + both reliably produce a genuine OCI Image Layout tar; a plain `docker + save` only does if the containerd image store happens to be enabled, + so the result is verified — `oci-layout`/`index.json` actually present + at the tar root — regardless of which tool produced it, falling + through to the next option otherwise), clear instructions + nonzero + exit if none are available and no fixture already exists. + + `meson.build` only compiles any of `tests/unit/`/`tests/integration/`/ + `tests/support/` into the `slocker-lite` binary at all when + `enable_tests` is on (mirroring `config.h`'s own `ENABLE_TESTS` guard — + `TEST_CASE`s need `catch2_dep` actually linked, which itself is + conditional on the same option), and registers two more `test()` entries + (`unit-tests`: `-t -- "[unit]"`; `integration-tests`: + `-t -- "[integration]~[net]"`) alongside the original fixture smoke test + — only the categories safe to run unprivileged with no network setup; + `[net]`/`[root]` stay manual-only, run by a developer on a real machine, + matching how this project's self-tests were never part of `meson test` + either. - `env_spec.{h,cpp}` — `resolve_env_specs()` turns an ordered list of `EnvSpec {is_file, value}` (see `cli_args.{h,cpp}` above) into a flat, ordered list of `(key, value)` pairs. A literal (`--env`) is split at its *first* `=` (the @@ -2083,8 +2170,12 @@ Build directory is `buildDir/` (already configured). `--user`, `--group`, `--hostname`, `--env`, `--env-file`, `-v/--volume`, `--list-volumes`, `--delete-volume`, `--delete-volume-full`, `-n/--network`, `--extern`, `--intern`, `--subnet`, `--no-ipv6`, `--subnet6`, `--no-veth`, `--list-networks`, `--delete-network`, `-p/--port-forward`, `--no-dns`, `--list-processes`, `--clean-processes`, - `-w/--write-config`, `-t/--test`, `--log-level`, `-h/--help`, `-V/--version`) -- Run tests: `meson test -C buildDir` + `-w/--write-config`, `-t/--test [-- ]`, `--log-level`, + `-h/--help`, `-V/--version`) +- Run tests: `meson test -C buildDir` (the `[unit]` + safe `[integration]` categories + only — see `self_test.{h,cpp}`'s own entry above and `README.md`'s "Testing" section + for the full `-t/--test` category/tag breakdown, including the `[net]`/`[root]` + categories that stay manual-only) ## Code style diff --git a/README.md b/README.md index 0494e8c..091b537 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,39 @@ meson test -C buildDir This also builds `buildDir/slocker-lite-priv-drop`, a small statically-linked helper that `-r --user`/`--group` needs at runtime (see "How it works"). +## Testing + +`meson test -C buildDir` above runs three checks: the original fixture mount/unmount +smoke test, plus two of the four test categories `-t/--test` provides (see below) -- +`meson test` only ever runs what's safe unprivileged with no network setup; the other +two categories are for a developer to run by hand on a real machine. + +`-t`/`--test` runs this project's own built-in [Catch2](https://github.com/catchorg/Catch2) +test suite directly: `slocker-lite -t [-- ]`. A bare `-t` +runs everything Catch2 finds; select a category with a tag expression (note the `--` +before it, needed since Catch2's own `-r/--reporter` and `-c/--section` would otherwise +collide with slocker-lite's own `-r/--run` and `-c/--cleanup`): + +| Category | What it needs | Invocation | +|---|---|---| +| 1. Unit | Nothing | `slocker-lite -t -- "[unit]"` | +| 2. Integration | Nothing (no network, no root) | `slocker-lite -t -- "[integration]~[net]"` | +| 3. Integration, networked | A real image (see below), no root | `slocker-lite -t -- "[integration][net]~[root]"` | +| 4. Integration, root | Root | `slocker-lite -t -- "[integration][root]"` | + +Append `~[slow]` to any of these to skip notably slow cases. A `[root]`-tagged test +`SKIP()`s cleanly (not a failure) when not actually run as root, and a +`[net]`-tagged one does the same when it needs a real container image it can't find +(see next) -- so a bare `-t` degrades gracefully rather than failing outright on a +plain rootless dev machine with no fixture set up. + +Categories 3 and 4 need a real, runnable OCI image (something with an actual +`/bin/sh`, unlike the minimal single-file fixture the plain `meson test` smoke test +uses) at `images/busybox.tar`. Run `tests/setup-tests.py` to fetch one automatically +(via `skopeo`, `podman`, or `docker`, whichever is available -- does nothing if +`images/busybox.tar` already exists, e.g. your own build); or just drop your own +busybox-like image there directly. + ## Usage ```