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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz
This commit is contained in:
@@ -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 <bridge>`), the
|
||||
container-side tap gets created with the requested name inside the target
|
||||
namespace (checked via a plain, single-shot
|
||||
`nsenter --net=/proc/<pid>/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 `<iface>: ...` 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 [-- <catch-command-line-options>]`, `--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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user