Skip to content

Conversation

Pouyanpi
Copy link
Collaborator

@Pouyanpi Pouyanpi commented Aug 18, 2025

tests for nemoguardrails CLI

requires #1343

Copilot

This comment was marked as outdated.

@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.78%. Comparing base (2af64d6) to head (0aee5a0).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1339      +/-   ##
===========================================
+ Coverage    71.66%   73.78%   +2.11%     
===========================================
  Files          171      171              
  Lines        17020    17020              
===========================================
+ Hits         12198    12558     +360     
+ Misses        4822     4462     -360     
Flag Coverage Δ
python 73.78% <ø> (+2.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Pouyanpi Pouyanpi requested review from tgasser-nv and Copilot August 28, 2025 10:49
@Pouyanpi Pouyanpi added this to the v0.17.0 milestone Aug 28, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@tgasser-nv tgasser-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few nits. Curious to learn where Colang 2's syntax is written down to write tests against


class TestRunChat:
@patch("asyncio.run")
@patch.object(chat_module, "LLMRails")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the same chat_module used throughout all these tests? Should we create it from scratch in each test to avoid mutations in a previous test changing the response?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants