Add unit tests, CI, lint targets; fix example arg parser and GiB/s naming - #1
Merged
Conversation
The Makefile hardcoded cuda-12.9 paths, breaking builds on systems with other CUDA versions, and the monitor rule lacked the NVML include path (nvml.h lives under the CUDA include dir), causing "nvml.h: No such file or directory". Use the standard /usr/local/cuda symlink for both include and lib paths, and add $(CUDA_INCLUDE) to the monitor compile rule. Signed-off-by: staryxchen <staryxchen@tencent.com>
Extract pure-logic functions from both binaries into separate translation
units so they can be tested without a GPU:
- monitor/bandwidth_calc.{h,cpp}: calculateBandwidth() moved verbatim
from NvLinkMonitor::calculateBandwidth (KiB->GiB/s conversion,
counter-overflow clamping, per-link + per-GPU totals).
- monitor/arg_parser.{h,cpp}: parseMonitorArgs() extracted from main()
hand-rolled parser; errors now reported via struct fields instead of
exit(), enabling testability.
- example/bw_stats.{h,cpp}: computeBandwidthStats() extracted from
testCopyPerformance() (avg/min/max bandwidth + latency math).
Add 34 tests (build/run without a GPU; test binary links only against
the extracted .cpp files + gtest, no NVML/CUDA library symbols):
- test_bandwidth_calc: positive/zero/overflow deltas, multi-link
aggregation, mismatched snapshot sizes, multi-GPU totals.
- test_arg_parser: defaults, all flags, invalid values, missing values,
unknown flags, combined options.
- test_bw_stats: known values, single iteration, empty input, all-equal
timings, min/max inversion.
Makefile: add `test` target (not in default `all`); update monitor/
example source lists to include the extracted .cpp files.
All refactors are behavior-preserving; `make` builds clean with
-Wall -Wextra and `make test` passes 34/34.
Signed-off-by: staryxchen <staryxchen@tencent.com>
Mechanical reformatting of the three pre-existing source files
(monitor/nvlink_monitor.{cpp,h}, example/nvlink_bw_test.cpp) to match
the .clang-format Google-based style. The extracted pure-logic files
and tests added earlier were already format-clean.
No logic changes; only whitespace, line wrapping, and brace placement.
Signed-off-by: staryxchen <staryxchen@tencent.com>
Makefile: - `make format`: apply clang-format -i to all C++ sources/headers. - `make check-format`: dry-run with --Werror, fails on violations (CI gate). Neither target is in the default `all` build. CI (.github/workflows/ci.yml), two jobs on ubuntu-22.04: - format-check: installs clang-format, runs `make check-format`. - test: installs libnvidia-ml-dev + libgtest-dev, runs `make test`. The test job needs no GPU: <nvml.h> comes from libnvidia-ml-dev (/usr/include), gtest ships prebuilt .a on 22.04, and the test binary links no NVML/CUDA library symbols. $(CUDA_INCLUDE) points to a nonexistent path in CI; GCC silently ignores nonexistent -I dirs. The monitor/example GPU binaries are not built in CI (need CUDA toolkit + driver); CODEBUDDY.md documents this limitation. Signed-off-by: staryxchen <staryxchen@tencent.com>
Two deferred issues from prior plans:
1. example/nvlink_bw_test.cpp parseCommandLine() called exit(1) on bad
input (untestable) and `-b -1` wrapped size_t to SIZE_MAX because
std::atoi("-1") assigned to size_t bypassed the <= 0 check (unsigned).
Extracted to example/arg_parser.{h,cpp} as parseBwTestArgs(), mirroring
monitor/arg_parser: errors reported via TestConfig.ok/errorMessage
(no exit). Parsing switched to std::strtol with full validation
(non-numeric, ERANGE, sign) so -b -1 is now correctly rejected.
Added 18 tests in test/test_bw_test_args.cpp including a -b -1
regression test.
2. txGBps/rxGBps/totalTxGBps/totalRxGBps field names and the example's
"GB/s" print labels were GiB/s mislabeled as GB. Renamed to
txGiBps/rxGiBps/totalTxGiBps/totalRxGiBps (matching the existing
txGiBps local var in bandwidth_calc.cpp), and avgGbps/minGbps/maxGbps
-> avgGiBps/minGiBps/maxGiBps. Example print labels fixed GB/s->GiB/s.
Monitor prints already said GiB/s; only field names changed there.
Removed obsolete "legacy naming" comments.
Makefile: add example/arg_parser.{cpp,h} to example + test build.
make check-format clean; make builds with zero -Wall -Wextra warnings;
make test passes 52/52 (34 existing + 18 new).
Signed-off-by: staryxchen <staryxchen@tencent.com>
- Fix monitor usage examples: single-dash long flags (-continuous, -interval, -verbose) did not work with the hand-rolled parser, which matches exact strings "-c"/"--continuous" etc. Changed to --continuous, --interval, --verbose. - Update Project Structure tree to list the extracted pure-logic modules (bandwidth_calc, arg_parser, bw_stats), the test/ directory, CI workflow, and .clang-format. - Add a Testing section documenting `make test` / `make format` / `make check-format`, the no-GPU test binary strategy, and the CI configuration. Signed-off-by: staryxchen <staryxchen@tencent.com>
staryxchen
force-pushed
the
add-tests-ci-and-fixes
branch
from
July 10, 2026 04:02
42add31 to
5731b20
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds unit test infrastructure, CI, and lint/format tooling, plus fixes two deferred code issues and brings docs up to date. All work is behavior-preserving except the two called-out bug fixes.
Changes
Build & tooling
/usr/local/cudasymlink (was hardcodedcuda-12.9); add NVML include path to the monitor build.make format(apply clang-format in place) andmake check-format(--dry-run --Werror, CI gate)..github/workflows/ci.yml): runsmake check-format+make teston ubuntu-22.04 for push/PR. No GPU needed — the test binary links only extracted pure-logic modules + gtest, with no NVML/CUDA library symbols.Unit tests (GoogleTest 1.11.0, 52 tests, no GPU required)
Extracted pure-logic functions into testable translation units (mirrored
monitor/arg_parserpattern for both binaries):monitor/bandwidth_calc.{h,cpp}— bandwidth math (KiB→GiB/s, overflow clamping)monitor/arg_parser.{h,cpp}—parseMonitorArgs()(errors via struct, noexit(1))example/bw_stats.{h,cpp}—computeBandwidthStats()example/arg_parser.{h,cpp}—parseBwTestArgs()(errors via struct, noexit(1))52 tests in
test/covering bandwidth math, both arg parsers, and stats aggregation.Code fixes
-b -1size_t wraparound:parseCommandLineusedstd::atoi→size_t, so-1becameSIZE_MAXand bypassed the unsigned<= 0check, then attempted an absurdcudaMalloc. Switched tostd::strtolwith full validation (non-numeric, ERANGE, sign); negatives now rejected. Includes a regression test.txGBps/rxGBps/totalTxGBps/avgGbps/minGbps/maxGbpsheld GiB/s values but were named "GB". Renamed totxGiBps/avgGiBpsetc. (matching the existingtxGiBpslocal var); example print labels fixedGB/s→GiB/s(monitor prints were already correct).Style & docs
-continuous/-interval/-verbose) didn't work with the hand-rolled parser (exact-string match), changed to--continuousetc.; updated project structure tree; added a Testing section documentingmake test/format/check-formatand CI.Verification
make check-format→ cleanmake→ both GPU binaries build with zero-Wall -Wextrawarningsmake test→ 52/52 passbuild/nvlink_bw_test -b -1→ prints "Error: buffer size must be positive", exits 1 (previously attemptedcudaMalloc(SIZE_MAX * 1MB))Notes
nvlink_monitor,nvlink_bw_test) are not built in CI — they require a CUDA toolkit + driver. This is documented in CODEBUDDY.md and the README."GB"(also GiB, but memory capacity not bandwidth);std::localtimethread-safety informatGPUResult.