show evals summary at end of pytest run - #620
Conversation
WalkthroughThis change refactors LLM evaluation test reporting by overhauling the pytest terminal summary logic. It now collects test results from pytest's internal stats, attaches detailed metadata to each test via user properties, generates markdown and console summary reports, and uses GPT-4o via LiteLLM for automated failure analysis. Documentation and CI/CD workflow environment variables are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Pytest
participant TestFunction
participant conftest.py
participant LiteLLM
participant Braintrust
Pytest->>TestFunction: Run test (with request)
TestFunction->>TestFunction: Append metadata to request.node.user_properties
Pytest->>conftest.py: Call pytest_terminal_summary
conftest.py->>conftest.py: Collect test results from terminalreporter.stats
conftest.py->>conftest.py: Build TestResult objects
conftest.py->>conftest.py: Generate markdown & Rich reports
alt If Braintrust API key present
conftest.py->>Braintrust: Generate links (if enabled)
end
alt For failed tests
conftest.py->>LiteLLM: Request GPT-4o analysis via API
LiteLLM-->>conftest.py: Return failure analysis
end
conftest.py->>Pytest: Print summary table and markdown report
Estimated code review effort3 (~45 minutes) Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/llm/utils/summary_plugin.py (4)
5-5: Remove duplicate import statement.
textwrapis imported twice - on line 5 and again on line 184 inside the method.Remove the duplicate import on line 184:
def _print_summary_table(self, test_results: Dict[str, TestResult]): """Print formatted summary table""" - import textwrap - table_data = []Also applies to: 184-184
254-260: Make the LLM model configurable.The model "gpt-4o" is hardcoded. Consider making it configurable via environment variable to match the pattern used in other test files.
try: response = completion( - model="gpt-4o", + model=os.environ.get("MODEL", "gpt-4o"), messages=[{"role": "user", "content": prompt}], max_tokens=200, temperature=0.1, )Don't forget to import
osat the top of the file:+import os from typing import Dict, List, Optional
234-263: Add error handling for LLM configuration issues.The
_get_llm_analysismethod might fail if LiteLLM is not properly configured with API keys. Consider adding more specific error handling to provide better debugging information.The current error handling is basic. Consider catching specific exceptions:
try: response = completion( model="gpt-4o", messages=[{"role": "user", "content": prompt}], max_tokens=200, temperature=0.1, ) return response.choices[0].message.content.strip() + except ImportError as e: + return f"LiteLLM not available: {e}" + except AttributeError as e: + return f"LLM configuration error: {e}" except Exception as e: - return f"Analysis failed: {e}" + return f"Analysis failed: {type(e).__name__}: {e}"
57-59: Consider making debug output conditional.The debug print statement will appear for every test, which might clutter the output.
Consider making the debug output conditional based on an environment variable:
# Debug output - print( - f"[DEBUG] Plugin captured test {test_id} ({test_name}): expected='{expected[:50]}...', actual='{actual[:50]}...'" - ) + if os.environ.get("HOLMES_DEBUG"): + print( + f"[DEBUG] Plugin captured test {test_id} ({test_name}): expected='{expected[:50]}...', actual='{actual[:50]}...'" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/llm/conftest.py(1 hunks)tests/llm/test_ask_holmes.py(2 hunks)tests/llm/test_investigate.py(2 hunks)tests/llm/utils/summary_plugin.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/llm/conftest.py (1)
tests/llm/utils/summary_plugin.py (1)
pytest_configure(265-269)
tests/llm/test_investigate.py (1)
tests/llm/utils/mock_utils.py (1)
InvestigateTestCase(65-69)
tests/llm/test_ask_holmes.py (1)
tests/llm/utils/mock_utils.py (1)
AskHolmesTestCase(60-62)
tests/llm/utils/summary_plugin.py (2)
holmes/core/tool_calling_llm.py (1)
call(237-368)tests/llm/conftest.py (1)
pytest_configure(14-19)
🪛 GitHub Actions: Evaluate LLM test cases
tests/llm/utils/summary_plugin.py
[error] 3-3: ModuleNotFoundError: No module named 'tabulate'. The test run failed due to missing dependency.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (3)
tests/llm/conftest.py (1)
10-20: LGTM! Clean plugin registration.The implementation correctly registers the custom summary plugin and reduces logging noise during test runs.
tests/llm/test_investigate.py (1)
95-186: LGTM! Proper integration with summary plugin.The test correctly stores expected output, actual output, and tools called as user properties for the summary plugin to collect.
tests/llm/test_ask_holmes.py (1)
63-172: LGTM! Consistent implementation with test_investigate.The test properly stores test metadata as user properties for the summary plugin.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/llm/utils/summary_plugin.py (1)
3-3: Address the missing dependency issue.The
tabulatedependency issue flagged in the previous review hasn't been resolved yet. This will cause runtime failures when the plugin is used.
🧹 Nitpick comments (4)
tests/llm/utils/summary_plugin.py (4)
176-176: Remove redundant import.
textwrapis already imported at the module level (line 5).- import textwrap
246-252: Improve error handling for LLM calls.The LLM call should handle specific exceptions and provide more informative error messages.
try: response = completion( model="gpt-4o", messages=[{"role": "user", "content": prompt}], max_tokens=200, temperature=0.1, ) - return response.choices[0].message.content.strip() - except Exception as e: - return f"Analysis failed: {e}" + if response.choices and response.choices[0].message.content: + return response.choices[0].message.content.strip() + else: + return "Analysis failed: Empty response from LLM" + except ImportError: + return "Analysis failed: LiteLLM not available" + except Exception as e: + return f"Analysis failed: {type(e).__name__}: {str(e)}"
268-268: Add missing newline at end of file.Python files should end with a newline character for better compatibility.
config.pluginmanager.unregister(config.summary_plugin) +
21-91: Consider adding configuration options for the plugin.The plugin currently captures all "holmes" tests but lacks configuration options for controlling its behavior (e.g., enabling/disabling LLM analysis, controlling output verbosity).
Consider adding configuration options that can be set via pytest command line or configuration files:
--holmes-summaryto enable/disable the plugin--holmes-analysisto enable/disable LLM analysis--holmes-debugto control debug output
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/llm/utils/summary_plugin.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/llm/utils/summary_plugin.py (2)
holmes/core/tool_calling_llm.py (1)
call(237-368)tests/llm/conftest.py (1)
pytest_configure(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit checks
🔇 Additional comments (1)
tests/llm/utils/summary_plugin.py (1)
143-150: Add bounds checking for array access.The code assumes specific array indices exist without validation, which could cause
IndexError.# Look for ** EXPECTED ** section if "** EXPECTED **" in all_output: lines = all_output.split("\n") for i, line in enumerate(lines): - if "** EXPECTED **" in line and i + 1 < len(lines): - expected_line = lines[i + 1] - if expected_line.startswith("- "): - expected = expected_line[3:] # Remove "- " prefix - break + if "** EXPECTED **" in line and i + 1 < len(lines): + expected_line = lines[i + 1] + if expected_line.startswith("- ") and len(expected_line) > 3: + expected = expected_line[3:] # Remove "- " prefix + breakLikely an incorrect or invalid review comment.
…olmesgpt into show-evals-summary
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/llm/utils/summary_plugin.py (3)
68-70: Remove or make debug output conditionalDebug print statements should not be left in production code as they clutter the test output.
Either remove the debug output:
- # Debug output - print( - f"[DEBUG] Plugin captured test {test_id} ({test_name}): expected='{expected[:50]}...', actual='{actual[:50]}...'" - )Or make it conditional based on verbosity:
- # Debug output - print( - f"[DEBUG] Plugin captured test {test_id} ({test_name}): expected='{expected[:50]}...', actual='{actual[:50]}...'" - ) + # Debug output (only in verbose mode) + if hasattr(item.config.option, 'verbose') and item.config.option.verbose > 1: + print( + f"[DEBUG] Plugin captured test {test_id} ({test_name}): expected='{expected[:50]}...', actual='{actual[:50]}...'" + )
104-110: Add error handling for test ID extractionThe string parsing logic is fragile and could raise
IndexErrorif test names don't follow the expected format.def _extract_test_id(self, test_name: str) -> str: """Extract test ID from test name like test_ask_holmes[01_how_many_pods]""" - if "[" in test_name and "]" in test_name: - test_case = test_name.split("[")[1].split("]")[0] - # Extract number from start of test case name - return test_case.split("_")[0] if "_" in test_case else test_case - return "unknown" + try: + if "[" in test_name and "]" in test_name: + test_case = test_name.split("[")[1].split("]")[0] + # Extract number from start of test case name + return test_case.split("_")[0] if "_" in test_case else test_case + except (IndexError, AttributeError): + pass + return "unknown"
112-119: Add error handling for test name extractionSimilar fragile string parsing that needs protection against malformed input.
def _extract_test_name(self, test_name: str) -> str: """Extract readable test name""" - if "[" in test_name and "]" in test_name: - test_case = test_name.split("[")[1].split("]")[0] - # Remove number prefix and convert underscores to spaces - parts = test_case.split("_")[1:] if "_" in test_case else [test_case] - return "_".join(parts) - return test_name + try: + if "[" in test_name and "]" in test_name: + test_case = test_name.split("[")[1].split("]")[0] + # Remove number prefix and convert underscores to spaces + parts = test_case.split("_")[1:] if "_" in test_case else [test_case] + return "_".join(parts) + except (IndexError, AttributeError): + pass + return test_name
🧹 Nitpick comments (1)
tests/llm/utils/summary_plugin.py (1)
256-285: Consider making the LLM model configurableThe LLM analysis implementation is excellent with well-structured prompts and proper error handling. However, the model is hardcoded to "gpt-4o".
Consider making the model configurable via environment variable or pytest configuration:
def _get_llm_analysis(self, result: TestResult) -> str: """Get LLM analysis of test failure""" + import os + model = os.getenv("HOLMES_TEST_ANALYSIS_MODEL", "gpt-4o") + prompt = textwrap.dedent(f"""\ Analyze this failed eval for an AIOps agent why it failed. TEST: {result.test_name} EXPECTED: {result.expected} ACTUAL: {result.actual} TOOLS CALLED: {', '.join(result.tools_called)} ERROR: {result.error_message or 'Test assertion failed'} LOGS: {result.logs if result.logs else 'No logs available'} Please provide a concise analysis (2-3 sentences) and categorize this as one of: - Problem with mock data - the test is failing due to incorrect or incomplete mock data, but the agent itself did the correct queries you would expect it to do - Setup issue - the test is failing due to an issue with the test setup, such as missing tools or incorrect before_test/after_test configuration - Real failure - the test is failing because the agent did not perform as expected, and this is a real issue that needs to be fixed """) try: response = completion( - model="gpt-4o", + model=model, messages=[{"role": "user", "content": prompt}], max_tokens=200, temperature=0.1, ) return response.choices[0].message.content.strip() except Exception as e: return f"Analysis failed: {e}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/llm/utils/summary_plugin.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/llm/utils/summary_plugin.py (1)
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
🧬 Code Graph Analysis (1)
tests/llm/utils/summary_plugin.py (2)
holmes/core/tool_calling_llm.py (1)
call(244-384)tests/llm/conftest.py (1)
pytest_configure(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (5)
tests/llm/utils/summary_plugin.py (5)
10-22: Well-structured dataclass designThe
TestResultdataclass is properly typed with appropriate optional fields and default values.
24-51: Proper pytest plugin structure with appropriate hooksThe plugin correctly implements pytest hooks for test lifecycle management and log capture.
121-172: Robust test data extraction with multiple fallbacksThe method effectively extracts test data from multiple sources (user properties and captured output) with appropriate fallback mechanisms.
174-255: Excellent summary table implementation with Rich formattingThe summary generation provides a comprehensive view of test results with proper text wrapping and visual indicators. The integration of LLM analysis for failed tests adds significant value.
287-298: Proper pytest plugin registration implementationThe plugin registration and unregistration functions correctly follow pytest conventions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…olmesgpt into show-evals-summary
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/llm/conftest.py (4)
14-16: Add error handling for plugin registrationThe current implementation doesn't handle the case where the plugin might already be registered in the plugin manager. Consider checking if it's already registered before attempting to register it again.
- if not hasattr(config, "summary_plugin"): - config.summary_plugin = SummaryPlugin() - config.pluginmanager.register(config.summary_plugin, "summary_plugin") + if not hasattr(config, "summary_plugin"): + config.summary_plugin = SummaryPlugin() + if not config.pluginmanager.is_registered(config.summary_plugin): + config.pluginmanager.register(config.summary_plugin, "summary_plugin")
22-26: Improve plugin unregistration safetyThe unconfigure function should verify the plugin is registered before attempting to unregister it to avoid potential errors.
def pytest_unconfigure(config): """Unregister the plugin""" if hasattr(config, "summary_plugin"): - config.pluginmanager.unregister(config.summary_plugin) + if config.pluginmanager.is_registered(config.summary_plugin): + config.pluginmanager.unregister(config.summary_plugin) + delattr(config, "summary_plugin")
328-339: Improve error handling in test name extractionThe current implementation uses a bare except clause for specific exceptions. Be more explicit about exception handling and avoid catching all exceptions.
def _extract_test_name_from_nodeid(nodeid: str) -> str: """Extract readable test name from nodeid""" - try: - if "[" in nodeid and "]" in nodeid: - test_case = nodeid.split("[")[1].split("]")[0] - # Remove number prefix and convert underscores to spaces - parts = test_case.split("_")[1:] if "_" in test_case else [test_case] - return "_".join(parts) - except (IndexError, AttributeError): - pass - return nodeid.split("::")[-1] if "::" in nodeid else nodeid + if "[" in nodeid and "]" in nodeid: + try: + test_case = nodeid.split("[")[1].split("]")[0] + # Remove number prefix and convert underscores to spaces + parts = test_case.split("_")[1:] if "_" in test_case else [test_case] + return "_".join(parts) + except IndexError: + # Fall through to default handling + pass + return nodeid.split("::")[-1] if "::" in nodeid else nodeid
183-318: Consider refactoring pytest_terminal_summary for better maintainabilityThe function is quite long and handles multiple responsibilities. Consider breaking it down into smaller, focused functions for better maintainability and testability.
Consider refactoring into separate functions:
_collect_test_results(terminalreporter)- Extract test results from stats_calculate_summary_stats(test_results)- Calculate pass/fail/regression counts_generate_markdown_report(test_results, stats)- Generate the markdown content_write_report_files(markdown, regressions)- Handle file writing with error handlingThis would make the code more modular, easier to test, and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/llm/conftest.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/llm/conftest.py (2)
287-292: Add error handling for score conversions.The float conversions could raise ValueError if the user properties contain invalid data. This matches a previous review comment that should be addressed for robustness.
- expected_correctness_score = float( - user_props.get("expected_correctness_score", 1.0) - ) - actual_correctness_score = float( - user_props.get("actual_correctness_score", 0.0) - ) + try: + expected_correctness_score = float( + user_props.get("expected_correctness_score", 1.0) + ) + actual_correctness_score = float( + user_props.get("actual_correctness_score", 0.0) + ) + except (ValueError, TypeError): + # Skip test if scores are invalid + continue
422-428: Add error handling for file operations.File write operations should include error handling to prevent crashes during report generation, matching a previous review comment.
- with open("evals_report.txt", "w", encoding="utf-8") as file: - file.write(markdown) - - # Write regressions file if needed - if total_regressions > 0: - with open("regressions.txt", "w", encoding="utf-8") as file: - file.write(f"{total_regressions}") + try: + with open("evals_report.txt", "w", encoding="utf-8") as file: + file.write(markdown) + except IOError as e: + print(f"Failed to write evals report: {e}") + + # Write regressions file if needed + if total_regressions > 0: + try: + with open("regressions.txt", "w", encoding="utf-8") as file: + file.write(f"{total_regressions}") + except IOError as e: + print(f"Failed to write regressions file: {e}")
🧹 Nitpick comments (1)
tests/llm/conftest.py (1)
521-524: Apply static analysis suggestion for cleaner code.The ternary operator suggestion from Ruff would make the code more concise.
- if "PASS" in result.pass_fail: - status = "[green]✓[/green]" - else: - status = "[red]✗[/red]" + status = "[green]✓[/green]" if "PASS" in result.pass_fail else "[red]✗[/red]"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/llm-evaluation.yaml(0 hunks)CLAUDE.md(1 hunks)docs/development/evals/index.md(1 hunks)docs/development/evals/reporting.md(0 hunks)tests/llm/conftest.py(2 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/llm-evaluation.yaml
- docs/development/evals/reporting.md
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- docs/development/evals/index.md
🧰 Additional context used
🧠 Learnings (1)
tests/llm/conftest.py (2)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
🧬 Code Graph Analysis (1)
tests/llm/conftest.py (4)
holmes/core/llm.py (2)
completion(48-58)completion(203-244)holmes/core/tracing.py (1)
completion(182-183)tests/llm/utils/braintrust.py (1)
get_experiment_name(166-171)tests/llm/utils/classifiers.py (1)
create_llm_client(16-44)
🪛 Ruff (0.12.2)
tests/llm/conftest.py
521-524: Use ternary operator status = "[green]✓[/green]" if "PASS" in result.pass_fail else "[red]✗[/red]" instead of if-else-block
Replace if-else-block with status = "[green]✓[/green]" if "PASS" in result.pass_fail else "[red]✗[/red]"
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (9)
tests/llm/conftest.py (9)
1-18: LGTM! Well-organized imports and dependencies.The import structure is clean and logically organized into standard library, third-party, and local imports. The addition of Rich, LiteLLM, and other dependencies supports the enhanced reporting functionality.
24-38: LGTM! Comprehensive TestResult dataclass design.The dataclass effectively encapsulates all necessary test metadata with appropriate defaults and optional fields. The structure supports both basic test information and enhanced analysis features.
40-44: Good addition of LiteLLM logging configuration.Suppressing noisy LiteLLM logs during testing improves the developer experience by reducing log noise.
194-247: Excellent refactoring of terminal summary logic.The new approach of collecting results from
terminalreporter.statsis much cleaner and more pytest-native than the previous Braintrust API approach. The separation of concerns with helper functions makes the code maintainable.
312-334: LGTM! Clean Braintrust URL generation with proper fallbacks.The function handles missing API keys gracefully and constructs URLs with appropriate defaults for organization names.
336-416: Excellent implementation of regression detection logic.The distinction between known failures (expected_score = 0, actual_score = 0) and actual regressions is well-implemented. The sorting logic ensures consistent test ordering in reports.
431-465: LGTM! Robust utility functions with proper error handling.The test ID and name extraction functions include appropriate try-catch blocks and fallback logic for malformed nodeids.
484-542: Excellent Rich table implementation with proper formatting.The table design with text wrapping, color coding, and proper column sizing enhances the developer experience significantly. The integration with LLM analysis provides valuable insights.
557-593: No additional LLM error-handling or API-key checks neededThe
_get_llm_analysishelper already wraps thelitellm.completioncall in a broad try/except that returns a clear failure message, and the existingcreate_llm_clientintests/llm/utils/classifiers.pyenforces presence of AZURE_API_KEY/OPENAI_API_KEY (and AZURE_API_BASE/ AZURE_API_VERSION). Other LLM calls inholmes/core/tool_calling_llm.pylikewise catch and log exceptions.No further validation or exception branches are required here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/llm/conftest.py (3)
250-255: Add error handling for score conversionsThe float conversions could raise ValueError if the user properties contain invalid data. This addresses a previous review comment that remains valid.
- expected_correctness_score = float( - user_props.get("expected_correctness_score", 1.0) - ) - actual_correctness_score = float( - user_props.get("actual_correctness_score", 0.0) - ) + try: + expected_correctness_score = float( + user_props.get("expected_correctness_score", 1.0) + ) + actual_correctness_score = float( + user_props.get("actual_correctness_score", 0.0) + ) + except (ValueError, TypeError): + # Skip test if scores are invalid + continue
389-396: Add error handling for file operationsFile write operations should include error handling to prevent crashes during report generation. This addresses a previous review comment that remains valid.
- with open("evals_report.txt", "w", encoding="utf-8") as file: - file.write(markdown) - - # Write regressions file if needed - if total_regressions > 0: - with open("regressions.txt", "w", encoding="utf-8") as file: - file.write(f"{total_regressions}") + try: + with open("evals_report.txt", "w", encoding="utf-8") as file: + file.write(markdown) + except IOError as e: + print(f"Failed to write evals report: {e}") + + # Write regressions file if needed + if total_regressions > 0: + try: + with open("regressions.txt", "w", encoding="utf-8") as file: + file.write(f"{total_regressions}") + except IOError as e: + print(f"Failed to write regressions file: {e}")
577-585: Improve error handling specificity for LLM callsThe current broad exception handling could mask important errors. This addresses a previous review comment that remains valid.
- try: - response = completion( - model="gpt-4o", - messages=[{"role": "user", "content": prompt}], - max_tokens=200, - temperature=0.1, - ) - return response.choices[0].message.content.strip() - except Exception as e: - return f"Analysis failed: {e}" + try: + response = completion( + model="gpt-4o", + messages=[{"role": "user", "content": prompt}], + max_tokens=200, + temperature=0.1, + ) + if response.choices and response.choices[0].message.content: + return response.choices[0].message.content.strip() + else: + return "Analysis failed: Empty response from LLM" + except ImportError as e: + return f"Analysis failed: LiteLLM import error - {e}" + except Exception as e: + return f"Analysis failed: {type(e).__name__}: {e}"
🧹 Nitpick comments (1)
tests/llm/conftest.py (1)
498-501: Consider using ternary operator for cleaner codeThe static analysis tool suggests a valid simplification that would improve readability.
- # Convert pass/fail to check/x status with colors - if "PASS" in pass_fail: - status = "[green]✓[/green]" - else: - status = "[red]✗[/red]" + # Convert pass/fail to check/x status with colors + status = "[green]✓[/green]" if "PASS" in pass_fail else "[red]✗[/red]"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/llm/conftest.py(2 hunks)
🧠 Learnings (1)
tests/llm/conftest.py (3)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
🧬 Code Graph Analysis (1)
tests/llm/conftest.py (2)
holmes/core/llm.py (2)
completion(48-58)completion(203-244)tests/llm/utils/braintrust.py (1)
get_experiment_name(166-171)
🪛 Ruff (0.12.2)
tests/llm/conftest.py
498-501: Use ternary operator status = "[green]✓[/green]" if "PASS" in pass_fail else "[red]✗[/red]" instead of if-else-block
Replace if-else-block with status = "[green]✓[/green]" if "PASS" in pass_fail else "[red]✗[/red]"
(SIM108)
🧰 Additional context used
🧠 Learnings (1)
tests/llm/conftest.py (3)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
🧬 Code Graph Analysis (1)
tests/llm/conftest.py (2)
holmes/core/llm.py (2)
completion(48-58)completion(203-244)tests/llm/utils/braintrust.py (1)
get_experiment_name(166-171)
🪛 Ruff (0.12.2)
tests/llm/conftest.py
498-501: Use ternary operator status = "[green]✓[/green]" if "PASS" in pass_fail else "[red]✗[/red]" instead of if-else-block
Replace if-else-block with status = "[green]✓[/green]" if "PASS" in pass_fail else "[red]✗[/red]"
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (5)
tests/llm/conftest.py (5)
1-17: LGTM: Clean imports and structureThe imports are well-organized into standard library, third-party, and local imports. The addition of
litellm,rich, andtextwrapdependencies aligns with the new functionality for LLM analysis and enhanced console output.
24-38: Good dataclass design for structured test resultsThe
TestResultdataclass provides a clean structure for encapsulating test metadata with appropriate defaults and type hints.
40-44: Appropriate logging configuration for third-party librariesSuppressing noisy LiteLLM logs during testing is a good practice to keep test output clean and focused.
186-187: Good use of environment variable with fallbackThe code correctly uses
BRAINTRUST_ORGenvironment variable with a sensible default fallback to "robustadev".
194-210: Well-structured terminal summary hookThe refactored
pytest_terminal_summaryfunction has a clean separation of concerns, delegating to focused helper functions for different output types.
2 participants
No description provided.