docs: refresh project documentation
This commit is contained in:
Mikhail Chusavitin
@@ -1,134 +1,54 @@
|
||||
# 09 — Testing
|
||||
|
||||
## Required before merge
|
||||
## Baseline
|
||||
|
||||
Required before merge:
|
||||
|
||||
```bash
|
||||
go test ./...
|
||||
```
|
||||
|
||||
All tests must pass before any change is merged.
|
||||
## Test locations
|
||||
|
||||
## Where to add tests
|
||||
|
||||
| Change area | Test location |
|
||||
|-------------|---------------|
|
||||
| Collectors | `internal/collector/*_test.go` |
|
||||
| HTTP handlers | `internal/server/*_test.go` |
|
||||
| Area | Location |
|
||||
|------|----------|
|
||||
| Collectors and replay | `internal/collector/*_test.go` |
|
||||
| HTTP handlers and jobs | `internal/server/*_test.go` |
|
||||
| Exporters | `internal/exporter/*_test.go` |
|
||||
| Parsers | `internal/parser/vendors/<vendor>/*_test.go` |
|
||||
| Vendor parsers | `internal/parser/vendors/<vendor>/*_test.go` |
|
||||
|
||||
## Exporter tests
|
||||
## General rules
|
||||
|
||||
The Reanimator exporter has comprehensive coverage:
|
||||
- Prefer table-driven tests
|
||||
- No network access in unit tests
|
||||
- Cover happy path and realistic failure/partial-data cases
|
||||
- New vendor parsers need both detection and parse coverage
|
||||
|
||||
| Test file | Coverage |
|
||||
|-----------|----------|
|
||||
| `reanimator_converter_test.go` | Unit tests per conversion function |
|
||||
| `reanimator_integration_test.go` | Full export with realistic `AnalysisResult` |
|
||||
## Mandatory coverage for dedup/filter/classify logic
|
||||
|
||||
Any new deduplication, filtering, or classification function must have:
|
||||
|
||||
1. A true-positive case
|
||||
2. A true-negative case
|
||||
3. A regression case for the vendor or topology that motivated the change
|
||||
|
||||
This is mandatory for inventory logic, firmware filtering, and similar code paths where silent data drift is likely.
|
||||
|
||||
## Mandatory coverage for expensive path selection
|
||||
|
||||
Any function that decides whether to crawl or probe an expensive path must have:
|
||||
|
||||
1. A positive selection case
|
||||
2. A negative exclusion case
|
||||
3. A topology-level count/integration case
|
||||
|
||||
The goal is to catch runaway I/O regressions before they ship.
|
||||
|
||||
## Useful focused commands
|
||||
|
||||
Run exporter tests only:
|
||||
```bash
|
||||
go test ./internal/exporter/...
|
||||
go test ./internal/exporter/... -v -run Reanimator
|
||||
go test ./internal/exporter/... -cover
|
||||
```
|
||||
|
||||
## Guidelines
|
||||
|
||||
- Prefer table-driven tests for parsing logic (multiple input variants).
|
||||
- Do not rely on network access in unit tests.
|
||||
- Test both the happy path and edge cases (missing fields, empty collections).
|
||||
- When adding a new vendor parser, include at minimum:
|
||||
- `Detect()` test with a positive and a negative sample file list.
|
||||
- `Parse()` test with a minimal but representative archive.
|
||||
|
||||
## Dedup and filtering functions — mandatory coverage
|
||||
|
||||
Any function that deduplicates, filters, or classifies hardware inventory items
|
||||
**must** have tests covering all three axes before the code is considered done:
|
||||
|
||||
| Axis | What to test | Why |
|
||||
|------|-------------|-----|
|
||||
| **True positive** | Items that ARE duplicates are collapsed to one | Proves the function works |
|
||||
| **True negative** | Items that are NOT duplicates are kept separate | Proves the function doesn't over-collapse |
|
||||
| **Counter-case** | The scenario that motivated the original code still works after changes | Prevents regression from future fixes |
|
||||
|
||||
### Worked example — GPU dedup regression (2026-03-11)
|
||||
|
||||
`collectGPUsFromProcessors` was added for MSI (chassis Id matches processor Id).
|
||||
No tests → when Supermicro HGX arrived (chassis Id = "HGX_GPU_SXM_1", processor Id = "GPU_SXM_1"),
|
||||
the chassis lookup silently returned nothing, serial stayed empty, UUID was new → 8 duplicate GPUs.
|
||||
|
||||
Simultaneously, fixing `gpuDocDedupKey` to use `slot|model` before path collapsed two distinct
|
||||
GraphicsControllers GPUs with the same model into one — breaking an existing test that had no
|
||||
counter-case for the path-fallback scenario.
|
||||
|
||||
**Required test matrix for any dedup function:**
|
||||
|
||||
```
|
||||
TestXxx_CollapsesDuplicates — same item via two sources → 1 result
|
||||
TestXxx_KeepsDistinct — two different items with same model → 2 results
|
||||
TestXxx_<VendorThatMotivated> — the specific vendor/setup that triggered the code
|
||||
```
|
||||
|
||||
### Worked example — firmware filter regression (2026-03-12)
|
||||
|
||||
`collectFirmwareInventory` was added in `6c19a58` without coverage for Supermicro naming.
|
||||
`isDeviceBoundFirmwareName` had patterns for Dell-style names (`"GPU SomeDevice"`, `"NIC OnboardLAN"`)
|
||||
but Supermicro Redfish uses `"GPU1 System Slot0"` and `"NIC1 System Slot0 ..."` — digit follows
|
||||
immediately after the type prefix. 29 device-bound entries leaked into `hardware.firmware`.
|
||||
|
||||
`9c5512d` attempted to fix this with HGX ID patterns (`_fw_gpu_`, etc.) in the wrong field:
|
||||
the filter checked `DeviceName` but `collectFirmwareInventory` populates it from `Name` first
|
||||
(`"Software Inventory"` for all HGX per-component slots), not from the `Id` field that contains
|
||||
the firmware ID like `"HGX_FW_GPU_SXM_1"`. The patterns were effectively dead code from day one.
|
||||
|
||||
**Required test matrix for any filter function:**
|
||||
|
||||
```
|
||||
TestXxx_FiltersDeviceBound_Dell — Dell-style names that motivated the original code
|
||||
TestXxx_FiltersDeviceBound_Supermicro — Supermicro names with digit suffix (GPU1/NIC1)
|
||||
TestXxx_KeepsSystemLevel — BIOS, BMC, CPLD names must NOT be filtered
|
||||
```
|
||||
|
||||
### Practical rule
|
||||
|
||||
When you write a new filter/dedup/classify function, ask:
|
||||
1. Does my test cover the vendor that motivated this code?
|
||||
2. Does my test cover a *different* vendor or naming convention where the function must NOT fire?
|
||||
3. If I change the dedup key logic, do existing tests still exercise the old correct behavior?
|
||||
4. When the filter checks a field on a model struct, does my test verify that the field is
|
||||
actually populated by the collector? (Dead-code filter pattern: `9c5512d` `_fw_gpu_` check.)
|
||||
|
||||
If any answer is "no" — add the missing test before committing.
|
||||
|
||||
## Collector candidate-selection functions — mandatory coverage
|
||||
|
||||
Any function that selects paths for an expensive operation (probing, crawling, plan-B retry)
|
||||
**must** have tests covering:
|
||||
|
||||
| Axis | What to test | Why |
|
||||
|------|-------------|-----|
|
||||
| **Positive** | Paths that should be selected ARE selected | Proves the feature works |
|
||||
| **Negative** | Paths that should be excluded ARE excluded | Prevents runaway I/O |
|
||||
| **Topology integration** | Given a realistic `out` map, the count of selected paths matches expectations | Catches implicit coupling between the selector and the surrounding data shape |
|
||||
|
||||
### Worked example — NVMe post-probe regression (2026-03-12)
|
||||
|
||||
`shouldAdaptiveNVMeProbe` was added in `2fa4a12` for Supermicro NVMe backplanes that return
|
||||
`Members: []` but serve disks at `Disk.Bay.N` paths. No topology-level test was added.
|
||||
|
||||
When SYS-A21GE-NBRT (HGX B200) arrived, its 35 sub-chassis (GPU, NVSwitch, PCIeRetimer,
|
||||
ERoT, IRoT, BMC, FPGA) all have `ChassisType=Module/Component/Zone` and empty `/Drives` →
|
||||
all 35 passed the filter → 35 × 384 = 13 440 HTTP requests → 22 min extra per collection.
|
||||
|
||||
A topology integration test (`TestNVMePostProbeSkipsNonStorageChassis`) would have caught
|
||||
this at commit time: given GPU chassis + backplane, exactly 1 candidate must be selected.
|
||||
|
||||
**Required test matrix for any path-selection function:**
|
||||
|
||||
```
|
||||
TestXxx_SelectsTargetPath — the path that motivated the code IS selected
|
||||
TestXxx_SkipsIrrelevantPath — a path that must never be selected IS skipped
|
||||
TestXxx_TopologyCount — given a realistic multi-chassis map, selected count = N
|
||||
go test ./internal/collector/...
|
||||
go test ./internal/server/...
|
||||
go test ./internal/parser/vendors/...
|
||||
```
|
||||
|
||||
Reference in New Issue
Block a user