Add unit test suite and code coverage enforcement#135
Add unit test suite and code coverage enforcement#135mmccarty merged 6 commits intorapidsai:mainfrom
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR adds comprehensive test coverage infrastructure to the RAPIDS CLI project, including pytest-cov dependency addition, coverage configuration to pyproject.toml, seven new test modules covering CLI/config/debug/doctor/GPU/memory/NVLink functionality, and documentation guidance (CLAUDE.md). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CLAUDE.md`:
- Around line 29-30: The README example references a non-existent test file
`rapids_cli/tests/test_cuda.py`; update the example command to point to an
actual test file (e.g., `rapids_cli/tests/test_gpu.py` or one of `test_cli.py`,
`test_config.py`, `test_debug.py`, `test_doctor.py`, `test_memory.py`,
`test_nvlink.py`) so the documented pytest invocation works; replace
`test_cuda.py` with a real test filename in the CLAUDE.md example line.
🧹 Nitpick comments (5)
CLAUDE.md (1)
100-100: Nit: "CLI interface" is redundant."CLI" already stands for "Command-Line Interface", so "CLI interface" expands to "Command-Line Interface interface."
Proposed fix
-- `rich` and `rich-click` for terminal output and CLI interface +- `rich` and `rich-click` for terminal output and CLIrapids_cli/tests/test_nvlink.py (1)
34-38: Unescaped regex metacharacters inmatchpattern.The dots in
"GPU not found. Please ensure GPUs are installed."are regex metacharacters that match any character. While unlikely to cause false matches here, usere.escape()or escape the dots for correctness.Proposed fix
+import re + def test_check_nvlink_status_no_gpu(): import pynvml with patch("pynvml.nvmlInit", side_effect=pynvml.NVMLError(1)): with pytest.raises( - ValueError, match="GPU not found. Please ensure GPUs are installed." + ValueError, match=re.escape("GPU not found. Please ensure GPUs are installed.") ): check_nvlink_status(verbose=False)rapids_cli/tests/test_debug.py (1)
84-88: Broadly patchingpathlib.Path.globandpathlib.Path.read_textis fragile.These patches affect all
Pathinstances process-wide, not just the ones indebug.py. If pytest internals, coverage tooling, or rich usePath.globorPath.read_textduring the test, it could cause subtle failures. Consider patching at the module level (e.g.,rapids_cli.debug.debug.Path) or using more targeted mocking if issues arise.rapids_cli/tests/test_doctor.py (1)
101-117: Unusedcapsysparameter and weak assertion — consider verifying only the filtered check ran.
capsysis declared but never used (Ruff ARG001). More importantly, the test only assertsresult is Truebut doesn't verify that only thecudfentry point was loaded/executed (andcumlwas skipped). Without that, the test doesn't actually confirm filtering behavior — it only confirms no crash.Consider calling
mock_ep2.load.assert_not_called()and removing the unusedcapsys, or using it to verify relevant output.Proposed improvement
-def test_doctor_check_with_filters(capsys): +def test_doctor_check_with_filters(): """Test doctor_check with filters.""" mock_ep1 = MagicMock() mock_ep1.name = "cudf_check" mock_ep1.value = "cudf.module:check" mock_ep1.load.return_value = mock_passing_check mock_ep2 = MagicMock() mock_ep2.name = "cuml_check" mock_ep2.value = "cuml.module:check" mock_ep2.load.return_value = mock_passing_check with patch( "rapids_cli.doctor.doctor.entry_points", return_value=[mock_ep1, mock_ep2] ): result = doctor_check(verbose=False, dry_run=False, filters=["cudf"]) assert result is True + mock_ep1.load.assert_called_once() + mock_ep2.load.assert_not_called()rapids_cli/tests/test_memory.py (1)
75-82: Use a raw string for thematchpattern to avoid unescaped regex metacharacters.The
matchargument is a regex pattern, and the dots in"GPU not found. Please ensure GPUs are installed."are metacharacters that match any character. While unlikely to cause a false positive here, using a raw string with escaped dots is more correct. Flagged by Ruff (RUF043).Proposed fix
with pytest.raises( - ValueError, match="GPU not found. Please ensure GPUs are installed." + ValueError, match=r"GPU not found\. Please ensure GPUs are installed\." ):
jameslamb
left a comment
There was a problem hiding this comment.
Packaging changes look good to me. I guess pyyaml must previously have been getting into the environment some other way, but the dependency is needed because rapids-cli has direct usage:
rapids-cli/rapids_cli/config.py
Line 7 in ba0763c
I didn't review the other changes, will leave that to Jacob and Naty.
## Summary - Overhaul Sphinx docs: add user guide, troubleshooting guide, plugin development guide, and full API reference (cli, doctor, debug, checks) - Switch Sphinx theme to pydata-sphinx-theme with RAPIDS branding, enable autodoc/napoleon/intersphinx/viewcode extensions - Remove stale doc pages (old `doctor.rst`, `cuda_driver.rst`) and restructure `index.rst` with proper toctree sections - Fix `doctor_check()` docstring from NumPy-style to Google-style (matches ruff pydocstyle convention) - Add SPDX license headers to docs and `dependencies.yaml` - Fix Sphinx cross-references for `run_debug` and `CheckResult` to use fully qualified module paths - Add `sphinx-llm` extension to generate `llms.txt` for LLM consumption - Add `sphinx-autobuild` with `make serve` target for live-reload docs development - Rewrite doctor.rst with user-facing content explaining cross-layer compatibility checks and library-bundled health checks - Move check execution flow details to plugin_development.rst - Fix incorrect `Raises` section in `doctor_check` docstring (replace with `Returns`) Note: This branch also includes the commits from #135 (unit tests and coverage). The docs-specific changes are in commits after `2137644`. ## Test plan - [ ] `make -C docs clean html` builds with 0 warnings - [ ] `pre-commit run --all-files` passes - [ ] `llms.txt` and `llms-full.txt` generated in build output - [ ] `make serve` starts live-reload server 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Mike McCarty <mmccarty@nvidia.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: James Lamb <jaylamb20@gmail.com>
Summary
[tool.pytest]→[tool.pytest.ini_options],tests→rapids_cli/tests)CLAUDE.mdwith project architecture, development commands, code style, and testing guidancedependencies.yamlTest plan
pytestpasses all 53 testspre-commit run --all-filespasses🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Documentation
Tests
Chores