test: add coverage for all cli commands#1848
Conversation
|
Main remaining items:
|
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
sricketts
force-pushed
the
test-cli-general
branch
from
cd56909 to
570e320
Compare
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds CLI test helpers and two new test modules (general and GPU), integrates them into the test-run script, and removes a redundant standalone show-config test. Tests use monkeypatching and temporary directories and are guarded for GPU availability where applicable. Changes
Sequence Diagram(s)(omitted — changes are test additions and helpers without new multi-component runtime control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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. 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 |
Add test_clear_cache_cmd_real that actually invokes the cache clearing logic using a temporary directory to avoid side effects. The test: - Creates a temp cache directory with dummy files - Uses monkeypatch to redirect FLASHINFER_JIT_DIR - Verifies cache is cleared successfully - Ensures no impact on the real environment Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add test_clear_cubin_cmd_real that actually invokes the cubin clearing logic using a temporary directory to avoid side effects. The test: - Creates a temp cubin directory with dummy files - Uses monkeypatch to redirect FLASHINFER_CUBIN_DIR in both import locations - Verifies cubins are cleared successfully - Ensures no impact on the real environment Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add comprehensive tests for module-status and list-modules CLI commands: - test_module_status_cmd_with_filters: Tests --filter option with compiled, not-compiled, and all filters - test_module_status_cmd_detailed: Tests --detailed flag for verbose output - test_list_modules_cmd_with_module_name: Tests inspecting a specific module by name, including both existing and non-existent modules All tests use proper mocking to avoid GPU architecture issues and ensure deterministic behavior. Mock module statuses are created with realistic data to test all output formats. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add CLI unit tests to the CI test suite by including them in task_jit_run_tests_part5.sh: - tests/cli/test_cli_cmds.py: Non-GPU CLI command tests - tests/cli/test_cli_cmds_gpu.py: GPU-dependent CLI command tests Part 5 was chosen as it's the lightest shard with only one existing test. Both test files are now included in the CI pipeline and will run on PRs and pushes to main. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
sricketts
requested review from
jimmyzho,
kahyunnam,
nvmbreughe and
yzh119
as code owners
|
@yongwww this PR adds a new category of unit tests. Did I add them correctly to the CI scripts? I added them to Note that only the tests in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/cli/test_cli_cmds_gpu.py`:
- Around line 7-12: Add a runtime skip guard to the GPU tests by importing
pytest and the flashinfer.utils helpers (e.g., get_compute_capability,
is_sm90a_supported, is_sm100a_supported) at the top of the file and, before
using _MOCKED_CUDA_ARCH_LIST or any GPU tests, call get_compute_capability() (or
the is_sm90a_supported/is_sm100a_supported predicates) and call pytest.skip(...)
with a clear reason when the current device arch is not in the expected list;
ensure the guard sits alongside the existing _MOCKED_CUDA_ARCH_LIST declaration
so tests are skipped early on unsupported architectures.
In `@tests/cli/test_cli_cmds.py`:
- Around line 83-85: The test function test_list_cubins_cmd_real currently
declares an unused fixture parameter monkeypatch; remove monkeypatch from the
function signature so the test reads def test_list_cubins_cmd_real(): and ensure
no other references to monkeypatch remain in that test (the test body should
still call _test_cmd_helper(["list-cubins"]) and assert the same strings).
🧹 Nitpick comments (1)
tests/cli/cli_cmd_helpers.py (1)
6-12: Use proper typing forcmdinstead of a list literal.Line 6 uses
cmd: [str], which is a list literal rather than a type. This is non-idiomatic and can confuse type checkers and IDEs. PreferSequence[str]orlist[str].♻️ Suggested refactor
+from typing import Sequence from click.testing import CliRunner from flashinfer.__main__ import cli -def _test_cmd_helper(cmd: [str]): +def _test_cmd_helper(cmd: Sequence[str]) -> str: """ Helper for command tests """ runner = CliRunner() result = runner.invoke(cli, cmd) assert result.exit_code == 0, result.output return result.output
sricketts
changed the title
Fix failing test that was trying to mock a non-existent function. The test now correctly: - Mocks get_subdir_file_list (not get_cubin_file_list) - Returns proper (filename, checksum) tuples - Mocks download_file to avoid network calls - Mocks verify_cubin to skip checksum verification All 11 tests in test_cli_cmds.py now pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/cli/test_cli_cmds.py`:
- Around line 68-80: The test test_download_cubin_cmd_mocked currently only
stubs flashinfer.artifacts.get_cubin_file_list but still triggers real
network/storage I/O during the download phase called by _test_cmd_helper;
replace or monkeypatch the actual downloader used by the CLI (the function that
performs cubin fetches inside the download-cubin flow) to a no-op or a fake that
records calls and returns success, so the test asserts behavior without network
access. Specifically, in the test replace the downloader function invoked by
_test_cmd_helper (the module/function responsible for fetching cubins in the
download-cubin command) with a stub, then assert that get_cubin_file_list was
used and that the CLI output contains "All cubin download tasks completed
successfully" without performing real downloads.
Fix module-level GPU check that was calling get_compute_capability() without the required device argument. Replace with simpler torch.cuda.is_available() check to skip tests when no GPU is present. All 5 tests in test_cli_cmds_gpu.py now pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Change invalid type hint `[str]` to proper `list[str]` syntax. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1. Add __init__.py to make tests/cli a proper package - Consistent with other test subdirectories (attention, gemm, etc.) - Allows running tests from repository root 2. Update imports to use relative imports - Change `from cli_cmd_helpers` to `from .cli_cmd_helpers` 3. Refactor test_cli_cmds_gpu.py to reduce code duplication - Extract common mocks into mock_module_registry fixture - Add docstrings to MockModuleStatus and _create_mock_statuses - Fix import ordering (stdlib first, then third-party) - Simplify test functions by using the shared fixture All 16 CLI tests pass when run from the repository root. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
/bot run |
I think it makes sense to put this in scripts/task_jit_run_tests_part5.sh. |
|
[FAILED] Pipeline #42353599: 11/20 passed |
Add three new tests covering the export-compile-commands CLI command: - test_export_compile_commands_mocked: verifies JSON output with mock specs - test_export_compile_commands_output_option: verifies --output overrides PATH - test_export_compile_commands_no_modules: verifies empty module registry handling Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
jimmyzho
left a comment
jimmyzho
left a comment
There was a problem hiding this comment.
small nit, but everything looks good to me!
|
|
||
| def test_list_cubins_cmd_real(): | ||
| out = _test_cmd_helper(["list-cubins"]) | ||
| assert "Cubin" in out and "Status" in out |
There was a problem hiding this comment.
For assertions like this we could make a simple helper function
def assert_output_contains_all(output, *expected_strings):
"""Assert that output contains all expected strings."""
missing = [s for s in expected_strings if s not in output]
assert not missing, f"Missing strings in output: {missing}\n\nActual output:\n{output}"
so it is easier to read the expected strings in output
| assert "=== Summary ===" in out | ||
| assert "Total modules:" in out | ||
| assert "Compiled:" in out | ||
| assert "Not compiled:" in out |
Add comment to test_cli_cmds.py docstring explaining that the replay CLI command is tested in tests/utils/test_logging_replay.py since it's tightly coupled with the logging/replay feature tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/cli/test_cli_cmds_gpu.py`:
- Around line 94-176: Several tests declare the mock_module_registry fixture as
a function parameter but never use its return value, causing ARG001 warnings;
replace the unused parameter with a decorator. For each test function named
test_module_status_cmd_mocked, test_module_status_cmd_with_filters,
test_module_status_cmd_detailed, test_list_modules_cmd_mocked, and
test_list_modules_cmd_with_module_name, remove mock_module_registry from the
function signature and add `@pytest.mark.usefixtures`("mock_module_registry")
above the function; ensure pytest is imported at the top of the file and keep
other parameters (e.g., monkeypatch) intact where used.
In `@tests/cli/test_cli_cmds.py`:
- Around line 90-96: Update the two monkeypatch lambda stubs so their unused
parameters are prefixed with an underscore to silence Ruff ARG005: in the
monkeypatch.setattr call for "flashinfer.artifacts.download_file" change the
lambda signature from lambda *args, **kwargs to lambda *_args, **_kwargs and in
the monkeypatch.setattr call for "flashinfer.artifacts.verify_cubin" change
lambda *args to lambda *_args; keep both lambdas returning True and leave the
monkeypatch.setattr targets unchanged.
5 participants
📌 Description
For #1833 - adds coverage for all CLI commands in
flashinfer/__main__.py.🔍 Related Issues
🚀 Pull Request Checklist
Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete.
✅ Pre-commit Checks
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
unittest, etc.).Reviewer Notes
Do we want all tests in a single file or split by command?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.