test(cli): add comprehensive CLI test suite and reorganize files #1339
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Update verbose logging to safely handle cases where log records may not have 'id' or 'task' attributes. Prevents potential AttributeError and improves robustness of LLM and prompt log output formatting.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1339 +/- ##
===========================================
+ Coverage 70.70% 73.75% +3.04%
===========================================
Files 161 171 +10
Lines 16312 17025 +713
===========================================
+ Hits 11533 12556 +1023
+ Misses 4779 4469 -310
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test coverage for the NemoGuardRails CLI and reorganizes the existing test files into a more structured directory layout.
Key changes:
- Move existing CLI tests to a dedicated
tests/cli/directory structure - Add extensive test coverage for CLI commands including migration, provider management, debugger, and chat functionality
- Fix a potential AttributeError in the verbose logging module when handling log records
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_cli_migration.py | Removed - moved to tests/cli/test_migration.py |
| tests/test_cli.py | Removed - moved to tests/cli/test_cli_main.py |
| tests/cli/test_migration.py | New comprehensive test suite for CLI migration functionality |
| tests/cli/test_llm_providers.py | New test suite for LLM provider selection and management |
| tests/cli/test_debugger.py | New test suite for CLI debugger commands |
| tests/cli/test_cli_main.py | New test suite for main CLI commands and functionality |
| tests/cli/test_chat.py | New test suite for chat interface functionality |
| nemoguardrails/logging/verbose.py | Fix AttributeError when log records lack 'id' attribute |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| id_str = getattr(record, "id", None) | ||
| id_display = f"({id_str[:5]}..)" if id_str else "" | ||
| console.print(f"[cyan]LLM {title} {id_display}[/]") |
There was a problem hiding this comment.
The getattr call should provide a default value that prevents IndexError when id_str is not None but has fewer than 5 characters. Consider using id_str[:5] if id_str and len(id_str) >= 5 else id_str or handle empty/short strings explicitly.
Copilot uses AI. Check for mistakes.
| id_str = getattr(record, "id", None) | ||
| id_display = f"({id_str[:5]}..)" if id_str else "" |
There was a problem hiding this comment.
Same issue as above - the slicing operation id_str[:5] could fail if id_str is not None but has fewer than 5 characters. The conditional should verify the string length before slicing.
Copilot uses AI. Check for mistakes.
tests for nemoguardrails CLI
requires #1343