feature-unit-testing

# Feature-Centric Unit Testing ## Overview One test = one transport feature. Tests live at the internal API boundary, not at the system level (`ncclAllReduce`). This localizes failures immediately: broken test → broken feature, no stack archaeology. The suite is the acceptance criterion for every PR. **Central discipline:** measure coverage first, plan second. Verify assumptions against real data before writing a single test. ## Modules - [multithread-validation.md](multithread-validation.md) — covering concurrent use of a net plugin: which production threading to reproduce, which concurrency is not worth testing, worker-option parsing, and how to make overlap an assertion. --- ## Feature Testing Lifecycle ``` Feature spec / integration plan │ ▼ DOMAIN 1 — Requirements & Planning • Write test plan at spec time (before any code) • Hardware scope: which cluster, NIC model, GDR, AINIC, QP count • Agree on coverage tier as merge acceptance criterion │ ▼ DOMAIN 2 — Basic Scenarios • Happy path, functional correctness, data integrity • Whitebox: direct internal API calls, scheduler inspection • Typically reaches ~50% line coverage │ ▼ DOMAIN 3 — Bottleneck & Edge Case Discovery • Fault injection, stress, boundary sizes, concurrent connections • Parametric sweeps, async data mutation, multidirectional patterns • Pushes coverage from 50% toward 70–90%+ │ ▼ ACCEPTANCE — Coverage Measurement • Measure → classify gap → add tests → re-measure • below target ──► identify gap ──► add tests ──► re-measure • at target ──► merge ``` **TDD analogy:** test plan = "red" phase written before code exists; Domain 2+3 = "green"; coverage measurement = objective acceptance signal. --- ## Coverage Acceptance Tiers Prefer **branch coverage** as the primary merge criterion. Line coverage is a secondary signal; function coverage is informational only. See the *Why Branch Coverage* section. | Tier | Line coverage | Branch coverage | When to target | |------|:------------:|:---------------:|----------------| | **Basic** | ≥ 50% | ≥ 35% | New feature, first PR — core path exercised | | **Standard** | ≥ 70% | ≥ 50% | Feature complete — error paths and main branches covered | | **Thorough** | ≥ 90% | ≥ 65% | Stable, high-impact — fault injection required | | **Critical** | ≥ 95% | ≥ 80% | Safety-critical: fatal-error handling, data integrity | **Cost of moving between tiers:** Standard→Thorough (branch) requires fault injection and parametric sweeps. Thorough→Critical is expensive — hardware-specific paths and rare races; calculate the realistic branch ceiling for your cluster before committing to this tier. --- ## Domain 1: Requirements & Planning Before writing any test code, create a test plan that defines: 1. **Feature scope** — which internal API surface is being tested, what is explicitly out of scope 2. **Scenario inventory** — happy path, error paths, edge cases, concurrency patterns 3. **Hardware scope** — which NIC model, how many ports, GDR/DMA-buf availability, single-node vs cross-host. Discovering that a test requires unavailable hardware after writing it wastes effort. 4. **Coverage tier** — agree on a branch coverage target (Basic/Standard/Thorough/Critical) as the merge acceptance criterion. This is the "red" phase in TDD: the bar is set before code exists. The plan becomes the PR description's Test Plan section — reviewable alongside code. It is a planning artifact — do not commit it as a file; keep it in the PR body. --- ## Commit Convention **One commit per test.** Each test gets its own atomic commit so reviewers can evaluate test intent and scope individually, and bisect targets a single test on regression. **Title format:** `<subsystem>: add <test name> test` (lowercase, no brackets) ``` net-ib: add test infrastructure (StressTests harness) ← CMakeLists + base fixture net-ib: add InvalidRecvCount test ← repeated ×N (test name verbatim) net-ib: update feature-unit-testing skill ← skill update last ``` **Body (required):** one short paragraph — what the test does, what path it covers, BRDA ref: ``` Call ncclIbIrecv with n=9 (> NCCL_NET_IB_MAX_RECVS=8). Verifies the early-return branch returns ncclInternalError without crashing (net_ib.cc:2731, BRDA:2731,0,1). ``` **What to NOT commit:** - Test plans (`TEST_PLAN.md`) — planning artifact, not part of the test suite - Coverage shell scripts (`run_*.sh`, `merge_coverage.sh`) — operational, not source - Profraw / profdata files — build artifacts --- ## Domain 2: Basic Scenario Patterns ### Whitebox testing — bypass the public API Call internal APIs directly. No `ncclCommInit` overhead. Example: ```cpp // Direct internal call — not through ncclAllReduce IbCastIsend(sendComm, buf, size, tag, mh, nullptr, &req); // Read scheduler state directly to verify internal invariants ncclIbCastGetSchedState(sendComm, &state); EXPECT_GT(state.activeQpTokens[0], 0); ``` Whitebox lets a test assert that WRR correctly redistributed tokens when one link is slow — something a blackbox allreduce test cannot verify. ### Structural patterns (MPI transport tests) **Double-barrier around every send/recv iteration:** ```cpp // rank 0: post recv | rank 1: post send MPI_Barrier(MPI_COMM_WORLD); // after both sides post // both: wait for completion MPI_Barrier(MPI_COMM_WORLD); // after both sides complete ``` Without the second barrier, one rank reuses request slots before the other finishes. **Retry loop for NULL send request (FIFO backpressure is normal):** ```cpp do { ASSERT_EQ(net_->isend(sendComm, data, size, tag, mh, nullptr, &req), ncclSuccess); if (req) break; usleep(10000); } while (true); // Never ASSERT_NE(req, nullptr) immediately after isend. ``` **RAII guard declaration order:** ```cpp NetConnectionGuard connGuard(net_); // 1st — connection auto bufGuard = makeHostBufferAutoGuard(…); // 2nd — buffers NetMHandleGuard mhGuard(mh, …); // 3rd — memory registration // Destruction runs in reverse: MR deregistered before connection closed. ``` **Non-fatal checks in multi-rank helpers** (fan-in, fan-out, all-to-all): ```cpp // Use EXPECT_ (non-fatal), not ASSERT_ (fatal), before any MPI_Barrier. // A fatal assert in rank 0 leaves all other ranks hanging at the barrier. EXPECT_EQ(PostRecv(…), ncclSuccess); // … MPI_Barrier(MPI_COMM_WORLD); // unconditional — always reached ``` **Timeout in poll loops (never spin forever):** ```cpp // Poll loops that wait for async completion MUST have a timeout. // An infinite loop masks the real bug (e.g. unroutable NIC) as "test hung". auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(30); while (!comm && std::chrono::steady_clock::now() < deadline) { ncclResult_t r = AcceptConnection(listenComm, &comm); if (r != ncclSuccess) break; } if (!comm) { /* handle timeout — GTEST_SKIP or fail with diagnostic */ } ``` Track the return value of each poll iteration separately from the NULL-comm check. A loop that only checks `!comm` cannot distinguish "still in progress" from "API returned an error 10 iterations ago and will never succeed". **GTEST_SKIP() synchronization across ranks:** ```cpp // When one rank detects a condition requiring SKIP (e.g. QP connect failed), // it MUST broadcast this to all ranks BEFORE calling GTEST_SKIP(). // GTEST_SKIP() does a return — if rank 0 returns, rank 1 hangs on MPI_Recv. int skipFlag = (connectFailed ? 1 : 0); MPI_Allreduce(MPI_IN_PLACE, &skipFlag, 1, MPI_INT, MPI_MAX, MPI_COMM_WORLD); if (skipFlag) { GTEST_SKIP() << "QP connect failed on at least one rank"; } ``` Without the Allreduce, a unilateral `GTEST_SKIP()` in one rank leaves the other rank blocked forever at its next MPI call or poll loop. --- ## Domain 3: Fault Injection **Deliberately break one component — verify the system response.** Compile-guarded (`-DENABLE_FAULT_INJECTION=ON`), zero cost in production: ```cpp ncclIbCastFaultSetQpDelay(comm, qpIdx, delayUs); // artificial latency on one QP ncclIbCastFaultSetQpError(comm, qpIdx, true); // force ncclSystemError ncclIbCastFaultClear(comm); // reset for recovery test ``` **Test categories fault injection enables:** - Fatal event propagation — all QPs faulted simultaneously - Single link failure — one QP faulted, WRR steered away deterministically - Scheduler rebalancing — artificial delay → WRR redistributes tokens - Data integrity under delay — sends complete correctly despite latency - Recovery — fault → clear → fresh connection completes cleanly **When fault injection is the only option:** `wc.status != IBV_WC_SUCCESS` paths in `ncclIbTest`, `fatalErrorCount` threshold branches, async error processing — all show zero hits in coverage until a fault injection hook exists. **Without a compile-time hook:** use LD_PRELOAD to intercept at the dynamic linker level: ```c // libibverbs_mock.so — intercepts ibv_poll_cq, injects bad WC after N calls int ibv_poll_cq(struct ibv_cq *cq, int num, struct ibv_wc *wc) { static int calls = 0; if (++calls == INJECT_AT) { wc->status = IBV_WC_REM_ACCESS_ERR; return 1; } return real_ibv_poll_cq(cq, num, wc); } ``` LD_PRELOAD only works if the target calls the symbol through the PLT. It does **not** work when the code uses `dlopen`/`dlsym` to resolve symbols into private function pointers, or dispatches through a driver ops-struct (e.g. `cq->context->ops.poll_cq`) — both bypass interposition. Check the binary for `dlopen`/`dlsym` usage before planning a shim; if it's there, fall back to a compile-time fault hook instead. --- ## Domain 3: Stress Testing **Resource leak detection:** ``` 100 × (listen → connect → transfer → close) Assert QP / PD / CQ / MR counts return to baseline after every cycle. ``` **Transfer size coverage:** - Minimum: 1-byte messages — exposes framing bugs invisible at larger sizes - Non-powers-of-two: 3, 7, 1023, 4097, 65537 — alignment and boundary conditions - Maximum: up to 64 MB — exercises large MR registration and buffer management **Multidirectional patterns:** allgather, alltoall, hypercube topologies; bidirectional simultaneous send/recv on the same connection; many concurrent connections open simultaneously (stress QP allocation limits). **Async data mutation:** mutate the send buffer *after* posting the send. Verifies the NIC captured data before mutation — detects use-after-post bugs in buffer registration. --- ## Domain 3: Parametric Configuration Matrix One test body swept over a grid of env-var combinations: ``` WRR on/off × QPS count × split threshold × adaptive routing threshold ``` Surfaces interactions between features that per-feature tests miss. Implement with `GTEST_SKIP()` when a required env var is absent: ```cpp const char* v = getenv("NCCL_IB_QPS_PER_CONNECTION"); if (!v) GTEST_SKIP() << "Set NCCL_IB_QPS_PER_CONNECTION to run this test"; ``` --- ## Why Branch Coverage, Not Lines or Functions **Line coverage lies. Branch coverage tells the truth.** A line is "covered" if it executed at least once — regardless of which path through it was taken. A function is "covered" if it was called — regardless of what happened inside. A branch is covered only when **both sides** of a conditional have been exercised. ``` // Line coverage: 100% (line executes in every test) // Branch coverage: 50% (error path never taken) if (result != ncclSuccess) goto fail; // ← "fail" branch: count = 0 ``` **Typical pattern in transport code:** adding a batch of stress tests can leave line coverage essentially flat while moving branch coverage only marginally — because the new tests re-exercise already-covered happy-path lines, while the uncovered side of each decision (the error or hardware-specific path) stays untaken. A healthy-looking line-coverage figure can hide that a large share of decision points are only half-tested. For transport code that gap maps directly to bugs tests cannot catch. **Functions coverage is the least informative metric for systems code.** A function called once with the happy-path arguments appears fully covered even if it contains 20 conditional branches that were never exercised. ### Branch coverage in practice **Use `--branch-coverage` in genhtml:** ```bash genhtml --branch-coverage merged.lcov.info -o html/ # Without this flag, the HTML report does not show per-branch hit counts. ``` **lcov vs llvm-cov branch counts differ:** the two tools report different branch denominators for the same file (llvm-cov includes C++ exception pseudo-branches that lcov's BRDA parsing excludes). Use one tool consistently within a comparison — mixing denominators makes deltas meaningless. lcov/genhtml is preferred for branch-level HTML drill-down. **Read BRDA lines, not DA lines, when hunting gaps:** ``` DA:1234,5 ← line 1234 executed 5 times (line coverage) BRDA:1234,0,0,5 ← block 0, branch 0: taken 5 times (covered) BRDA:1234,0,1,0 ← block 0, branch 1: taken 0 times ← THIS IS THE GAP ``` A line with `DA` count > 0 but a `BRDA` count of 0 on one arm is the exact pattern of "line covered, branch not covered" — the most common source of false confidence. Agree on a branch coverage tier at planning time (see *Coverage Acceptance Tiers* above). Branch coverage ceilings are hardware-dependent — establish a **realistic ceiling** (branches reachable on your hardware) before setting a target. --- ## Coverage Analysis Workflow > **STOP — mandatory before writing any test to cover a gap:** > 1. Export BRDA data and find the exact branch at the target line. > 2. Confirm the count field is `0`. If it is `> 0`, the branch is already covered — do not write a test. > 3. Read ±10 lines of source context to classify the branch (Trivial / Structural / Hardware / Fault injection / Dead). > 4. Only then write or run a test. > > Skipping this check is the single most common cause of zero-delta test additions. ### 1. Get the raw BRDA data ```bash llvm-profdata merge -sparse *.profraw -o merged.profdata llvm-cov export -format=lcov -instr-profile=merged.profdata ./binary \ -sources src/your_file.cc > merged.lcov.info genhtml --branch-coverage merged.lcov.info -o html/ # Extract uncovered branches (count == 0) awk -F: '/^SF:.*your_file/{found=1} found && /^BRDA:/ && $NF==0 \ {print} /^end_of_record/{found=0}' merged.lcov.info | sort -t: -k2 -n ``` ### 2. Read source context around every uncovered line Read ±10 lines. A branch at a given line may be a trivial env-var check or a hardware-only fault path — the line number tells you nothing without context. ### 3. Verify existing tests before claiming a gap **Anti-pattern:** "All tests call `PostRecv(n=1)`, so the `nreqs > 1` path is uncovered." **What actually happened:** `MultiRecv` and `MultiRecvShuffled` already called `PostRecv(n=8)` with shuffled tag order, covering the FIFO slot scan for r > 0. The BRDA count showed > 0. The assumption was wrong. **Rule:** Before adding a test to cover branch X, check the BRDA count for that exact line in the merged profile. If count > 0, it is already covered. --- ## Branch Classification Classify every uncovered branch before deciding whether to test it: | Class | Description | Technique | |-------|-------------|-----------| | **Trivial** | One env var or API param change | Set `NCCL_IB_X=1`, call with `n=9` | | **Structural** | State the harness can't easily create | Cross-host run, `tc qdisc netem delay` | | **Hardware** | Needs specific HW (GDR, >1 NIC, non-loopback) | Different node, or `GTEST_SKIP()` | | **Fault injection** | Needs a mock/shim returning errors | LD_PRELOAD, compile-time hook | | **Dead/unreachable** | Kernel or driver guarantees make it impossible | Document and skip | **Ceiling calculation:** count only Trivial + Structural (with infra) + Fault injection. Never include Dead branches in projected delta. **What moves coverage in net_ib.cc (measured on one cluster — verify on yours):** - Multi-QP split (`NCCL_IB_SPLIT_DATA_ON_QPS=1`) — split alignment branches, QP distribution