Improve evals mock generation - #687
Conversation
WalkthroughThis change refactors and enhances the LLM test infrastructure and mock toolset management. It removes YAML-based mock configuration in favor of CLI flags, introduces a new file-based mock toolset system with improved error handling, updates test code to use a centralized mock configuration object, and expands test coverage for the new mock system. Documentation is updated to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Pytest
participant MockGenerationConfig
participant MockToolsetManager
participant MockFileManager
participant ToolExecutor
participant RealTool
Tester->>Pytest: Run test with --generate-mocks/--regenerate-all-mocks
Pytest->>MockGenerationConfig: Create config from CLI/env
Pytest->>MockToolsetManager: Initialize with config, test folder, request
MockToolsetManager->>MockFileManager: Set up file management
MockToolsetManager->>ToolExecutor: Provide wrapped tools
loop For each tool invocation
ToolExecutor->>MockableToolWrapper: Invoke tool
alt Mode is MOCK
MockableToolWrapper->>MockFileManager: Read mock (by tool/params)
alt Mock found
MockFileManager-->>MockableToolWrapper: Return mock result
else Mock missing
MockFileManager-->>MockableToolWrapper: Raise MockDataNotFoundError
end
else Mode is GENERATE
MockableToolWrapper->>RealTool: Call real tool
RealTool-->>MockableToolWrapper: Return result
MockableToolWrapper->>MockFileManager: Write result as mock
else Mode is LIVE
MockableToolWrapper->>RealTool: Call real tool
RealTool-->>MockableToolWrapper: Return result
end
end
Pytest->>MockToolsetManager: Collect results, track generated/failed mocks
Pytest->>Tester: Output summary and mock operations report
Estimated code review effort4 (~90 minutes) Possibly related PRs
Suggested labels
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: 3
🔭 Outside diff range comments (1)
tests/llm/test_investigate.py (1)
51-56: Critical: Remove references to removedtool_mocksattributeThe following tests still loop over
self._test_case.tool_mocks, which was removed intests/llm/utils/test_case_utils.py. Mocks are now loaded on-demand byMockFileManager, so these loops will raise anAttributeError.Affected files:
- tests/llm/test_investigate.py (lines 51–56)
- tests/llm/test_workload_health.py (around lines 50–55)
Suggested update:
- for tool_mock in self._test_case.tool_mocks: - mock.mock_tool(tool_mock) - expected_tools.append(tool_mock.tool_name) + # Load mocks on-demand instead of accessing the removed attribute + tools = MockFileManager().load_tools(self._test_case) + for tool in tools: + mock.mock_tool(tool) + expected_tools.append(tool.tool_name)If there’s a test fixture or helper (e.g. on
mock_generation_config) that exposes the list of tools, prefer using that instead of instantiatingMockFileManagerdirectly.
♻️ Duplicate comments (1)
tests/llm/test_workload_health.py (1)
54-56: Critical:tool_mocksattribute no longer exists in test casesSimilar to
test_investigate.py, this code referencesself._test_case.tool_mockswhich was removed from theHolmesTestCasemodel. This will cause anAttributeErrorat runtime.
🧹 Nitpick comments (20)
tests/llm/utils/braintrust.py (3)
68-90: Stray “f” prefixes in log messages create noisy output
logging.info(f"Uploading f{len(test_cases)} …"),Updating dataset item f{…},Creating dataset item f{…}all emit an extra literal"f"(e.g. “Uploading f5 test cases”).-logging.info(f"Uploading f{len(test_cases)} test cases to braintrust") +logging.info(f"Uploading {len(test_cases)} test cases to braintrust") … -logging.info(f"Updating dataset item f{test_case.id}") +logging.info(f"Updating dataset item {test_case.id}") … -logging.info(f"Creating dataset item f{test_case.id}") +logging.info(f"Creating dataset item {test_case.id}")
82-96: Avoid shadowing the built-ininputname
Using the builtin identifier as a local variable can confuse linters & IDEs.- input=input, + input=dataset_input,with a prior assignment:
dataset_input = test_case.expected_input if hasattr(test_case, "expected_input") else ""Repeat for the
insertand_root_span.logcalls.Also applies to: 153-160
5-6: Modern generics preferred
Style guide preferslist[...],dict[...]overList[...],Dict[...]. Consider updating the typing imports when touching this file next.tests/llm/utils/langfuse.py (1)
48-56: Sameinputshadowing issue as in braintrust helper
Renaming the localinputdict (e.g. topayload) would avoid masking the builtin.CLAUDE.md (1)
125-129: Tiny wording nit“tests work correctly when run in parallel with
-nflag” → consider “when run in parallel withpytest -n” for clarity.tests/llm/utils/mock_dal.py (1)
40-42: Fix log message typo
"contentof"→"content of"to keep logs readable.- f"A mock file was generated for you at {file_path} with the contentof dal.get_issue_data({issue_id})" + f"A mock file was generated for you at {file_path} with the content of dal.get_issue_data({issue_id})"docs/development/evals/writing.md (3)
56-56: Fix typo: "preent" should be "present"-The resulting score is called `correctness` and is a binary score with a value of either `0` or `1`. HolmesGPT's answer is score `0` is any of the expected element is not present in the answer, `1` if all expected elements are preent in the answer. +The resulting score is called `correctness` and is a binary score with a value of either `0` or `1`. HolmesGPT's answer is score `0` is any of the expected element is not present in the answer, `1` if all expected elements are present in the answer.
70-70: Consider reducing the number of iterations for mock generationUsing
ITERATIONS=100for mock generation seems excessive. Mock generation typically only needs to capture the different tool execution paths, which usually doesn't require 100 iterations. Consider using a smaller number like 10-20 to reduce execution time while still capturing the necessary mock data.
94-94: Enhance deprecation notice with migration guidanceThe deprecation notice could be more helpful by providing migration instructions for users with existing test cases.
-| ~~`generate_mocks`~~ | ~~boolean~~ | **DEPRECATED**: Use `--generate-mocks` or `--regenerate-all-mocks` CLI flags instead | +| ~~`generate_mocks`~~ | ~~boolean~~ | **DEPRECATED**: Use `--generate-mocks` or `--regenerate-all-mocks` CLI flags instead. If this field exists in your test case YAML, it will be ignored. |tests/llm/utils/test_case_utils.py (1)
48-48: Consider removing deprecatedgenerate_mocksfieldThe
generate_mocksfield is still present in the model but is marked as deprecated in the documentation. To avoid confusion and ensure consistency with the new CLI-based approach, consider removing this field entirely or adding a deprecation comment/warning.- generate_mocks: bool = False # If True, generate mocks + # generate_mocks: bool = False # DEPRECATED: Use --generate-mocks CLI flag insteadtest_mock_simple.py (2)
21-21: Consider increasing timeout for robustnessThe 30-second timeout might be insufficient for some test scenarios, especially when running with multiple iterations or in CI environments with limited resources. Consider increasing it to 60 or 120 seconds.
- cmd, shell=True, capture_output=True, text=True, env=full_env, timeout=30 + cmd, shell=True, capture_output=True, text=True, env=full_env, timeout=120
83-83: Potential issue with partial output checkingChecking only the first 1000 characters might miss mock errors that appear later in the output. Consider checking the entire output or at least increasing the limit.
-if "MockDataError" not in result.stdout[:1000]: # Check first 1000 chars +if "MockDataError" not in result.stdout: # Check entire outputtests/llm/test_ask_holmes.py (1)
131-134: Consider using isinstance() for cleaner type checking.While the current approach works, using
isinstance()would be more Pythonic and cleaner.- # Check if this is a MockDataError - is_mock_error = "MockDataError" in type(e).__name__ or any( - "MockData" in base.__name__ for base in type(e).__mro__ - ) + # Check if this is a MockDataError + from tests.llm.utils.mock_toolset import MockDataError + is_mock_error = isinstance(e, MockDataError)tests/llm/utils/test_mock_toolset.py (4)
353-357: Combine nestedwithstatements for better readability.Static analysis correctly identifies that these can be combined.
- with patch( - "holmes.plugins.toolsets.service_discovery.find_service_url", - return_value="http://mock-prometheus:9090", - ): - with tempfile.TemporaryDirectory() as tmpdir: + with ( + patch( + "holmes.plugins.toolsets.service_discovery.find_service_url", + return_value="http://mock-prometheus:9090", + ), + tempfile.TemporaryDirectory() as tmpdir, + ):
404-408: Combine nestedwithstatements.- with patch( - "holmes.plugins.toolsets.service_discovery.find_service_url", - return_value="http://mock-prometheus:9090", - ): - with tempfile.TemporaryDirectory() as tmpdir: + with ( + patch( + "holmes.plugins.toolsets.service_discovery.find_service_url", + return_value="http://mock-prometheus:9090", + ), + tempfile.TemporaryDirectory() as tmpdir, + ):
471-475: Combine nestedwithstatements.- with patch( - "holmes.plugins.toolsets.service_discovery.find_service_url", - return_value="http://mock-prometheus:9090", - ): - with tempfile.TemporaryDirectory() as tmpdir: + with ( + patch( + "holmes.plugins.toolsets.service_discovery.find_service_url", + return_value="http://mock-prometheus:9090", + ), + tempfile.TemporaryDirectory() as tmpdir, + ):
513-517: Combine nestedwithstatements.- with patch( - "holmes.plugins.toolsets.service_discovery.find_service_url", - return_value="http://mock-prometheus:9090", - ): - with tempfile.TemporaryDirectory() as tmpdir: + with ( + patch( + "holmes.plugins.toolsets.service_discovery.find_service_url", + return_value="http://mock-prometheus:9090", + ), + tempfile.TemporaryDirectory() as tmpdir, + ):tests/llm/conftest.py (2)
51-53: Simplify the return statement.Static analysis correctly identifies this can be simplified.
- if self.actual_score == 0 and self.expected_score == 0: - return False - return True + return not (self.actual_score == 0 and self.expected_score == 0)
103-108: Consider using dataclass for MockGenerationConfig.The internal class could be more concise as a dataclass.
- class MockGenerationConfig: - def __init__(self, generate_mocks_enabled, regenerate_all_enabled, mock_mode): - self.generate_mocks = generate_mocks_enabled - self.regenerate_all_mocks = regenerate_all_enabled - self.mode = mock_mode + from dataclasses import dataclass + + @dataclass + class MockGenerationConfig: + generate_mocks: bool + regenerate_all_mocks: bool + mode: MockModetests/llm/utils/mock_toolset.py (1)
387-394: Simplify nested if statements.Static analysis correctly identifies this can be simplified.
- if mock_generation_tracker and mock_generation_tracker.regenerate_all_mocks: - if request and test_case_folder not in getattr( - mock_generation_tracker, "_cleared_folders", set() - ): + if ( + mock_generation_tracker + and mock_generation_tracker.regenerate_all_mocks + and request + and test_case_folder not in getattr(mock_generation_tracker, "_cleared_folders", set()) + ): self.file_manager.clear_mocks(request) if not hasattr(mock_generation_tracker, "_cleared_folders"): mock_generation_tracker._cleared_folders = set() mock_generation_tracker._cleared_folders.add(test_case_folder)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (76)
CLAUDE.md(1 hunks)docs/development/evals/index.md(1 hunks)docs/development/evals/writing.md(4 hunks)test_mock_simple.py(1 hunks)tests/llm/conftest.py(18 hunks)tests/llm/fixtures/test_ask_holmes/01_how_many_pods/kubernetes_countitems_select_.metadata.namespace_test-1_.metadata.name_pod.txt(1 hunks)tests/llm/fixtures/test_ask_holmes/02_what_is_wrong_with_pod/kubectl_describe.txt(0 hunks)tests/llm/fixtures/test_ask_holmes/02_what_is_wrong_with_pod/kubectl_find_resource.txt(0 hunks)tests/llm/fixtures/test_ask_holmes/02_what_is_wrong_with_pod/kubectl_logs.txt(0 hunks)tests/llm/fixtures/test_ask_holmes/02_what_is_wrong_with_pod/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/06_explain_issue/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/08_sock_shop_frontend/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/13_pending_node_selector/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/16_failed_no_toolset_found/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/20_long_log_file_search/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/23_app_error_in_current_logs/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/24_misconfigured_pvc/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/25_misconfigured_ingress_class/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/27_permissions_error_no_helm_tools/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/28_permissions_error_helm_tools_enabled/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/30_basic_promql_graph_cluster_memory/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/31_basic_promql_graph_pod_memory/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/33_http_latency_graph/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/36_argocd_find_resource/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/37_argocd_wrong_namespace/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/38_rabbitmq_split_head/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/39_failed_toolset/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/40_disabled_toolset/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/41_setup_argo/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/42_dns_issues_result_all_tools/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/42_dns_issues_result_new_tools/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/42_dns_issues_result_old_tools/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/42_dns_issues_steps_new_all_tools/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/42_dns_issues_steps_new_tools/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/42_dns_issues_steps_old_tools/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/43_current_datetime_from_prompt/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/43_slack_deployment_logs/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/44_slack_statefulset_logs/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/45_fetch_deployment_logs_simple/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/46_job_crashing_no_longer_exists/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/47_truncated_logs_context_window/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/48_logs_since_thursday/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/49_logs_since_last_week/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/50_logs_since_specific_date/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/51_logs_summarize_errors/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/52_logs_login_issues/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/53_logs_find_term/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/54_not_truncated_when_getting_pods/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/55_kafka_runbook/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/56_kafka_runbook_no_tool/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/97_mock_error_partial_files/kubernetes_countitems_select_.metadata.namespace_kube-system_.metadata.name_pod.txt(1 hunks)tests/llm/fixtures/test_investigate/03_cpu_throttling/test_case.yaml(0 hunks)tests/llm/fixtures/test_investigate/05_crashpod/test_case.yaml(0 hunks)tests/llm/fixtures/test_investigate/06_job_failure/test_case.yaml(0 hunks)tests/llm/fixtures/test_investigate/07_job_syntax_error/test_case.yaml(0 hunks)tests/llm/fixtures/test_investigate/08_memory_pressure/test_case.yaml(0 hunks)tests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/test_case.yaml(0 hunks)tests/llm/fixtures/test_investigate/11_KubePodCrashLooping/test_case.yaml(0 hunks)tests/llm/fixtures/test_investigate/12_KubePodNotReady/test_case.yaml(0 hunks)tests/llm/fixtures/test_investigate/13_Watchdog/test_case.yaml(0 hunks)tests/llm/fixtures/test_investigate/14_tempo/test_case.yaml(0 hunks)tests/llm/fixtures/test_investigate/15_dns_resolution/test_case.yaml(0 hunks)tests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/test_case.yaml(0 hunks)tests/llm/test_ask_holmes.py(5 hunks)tests/llm/test_investigate.py(4 hunks)tests/llm/test_mocks.py(0 hunks)tests/llm/test_workload_health.py(3 hunks)tests/llm/utils/braintrust.py(1 hunks)tests/llm/utils/commands.py(1 hunks)tests/llm/utils/langfuse.py(1 hunks)tests/llm/utils/mock_dal.py(2 hunks)tests/llm/utils/mock_toolset.py(2 hunks)tests/llm/utils/tags.py(1 hunks)tests/llm/utils/test_case_utils.py(3 hunks)tests/llm/utils/test_mock_toolset.py(3 hunks)
🧠 Learnings (9)
tests/llm/utils/braintrust.py (2)
Learnt from: nherment
PR: #436
File: tests/llm/utils/mock_utils.py:240-249
Timestamp: 2025-06-05T12:23:27.634Z
Learning: The holmesgpt project uses Python >= 3.10 and prefers modern type hint syntax like list[str], dict[str, int] over importing equivalent types from the typing module like List[str], Dict[str, int].
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.
tests/llm/fixtures/test_ask_holmes/01_how_many_pods/kubernetes_countitems_select_.metadata.namespace_test-1_.metadata.name_pod.txt (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.
tests/llm/utils/tags.py (1)
Learnt from: nherment
PR: #436
File: tests/llm/utils/mock_utils.py:240-249
Timestamp: 2025-06-05T12:23:27.634Z
Learning: The holmesgpt project uses Python >= 3.10 and prefers modern type hint syntax like list[str], dict[str, int] over importing equivalent types from the typing module like List[str], Dict[str, int].
tests/llm/fixtures/test_ask_holmes/02_what_is_wrong_with_pod/test_case.yaml (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.
CLAUDE.md (2)
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.
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.
docs/development/evals/writing.md (2)
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.
tests/llm/fixtures/test_ask_holmes/97_mock_error_partial_files/kubernetes_countitems_select_.metadata.namespace_kube-system_.metadata.name_pod.txt (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.
tests/llm/test_ask_holmes.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.
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🧬 Code Graph Analysis (5)
tests/llm/utils/commands.py (1)
tests/llm/utils/test_case_utils.py (1)
HolmesTestCase(43-58)
tests/llm/utils/tags.py (1)
tests/llm/utils/test_case_utils.py (1)
HolmesTestCase(43-58)
tests/llm/utils/mock_dal.py (1)
tests/llm/utils/test_case_utils.py (1)
read_file(18-20)
tests/llm/test_workload_health.py (4)
tests/llm/utils/mock_toolset.py (1)
MockToolsetManager(359-472)tests/llm/utils/test_case_utils.py (1)
HealthCheckTestCase(73-77)tests/llm/conftest.py (1)
mock_generation_config(86-109)tests/llm/utils/mock_dal.py (1)
MockSupabaseDal(14-73)
tests/llm/utils/mock_toolset.py (14)
holmes/core/tools.py (12)
StructuredToolResult(45-68)Tool(126-168)Toolset(331-473)ToolsetStatusEnum(102-105)_invoke(163-164)_invoke(215-243)invoke(142-160)get_parameterized_one_liner(167-168)get_parameterized_one_liner(194-201)get_example_config(465-466)get_example_config(484-485)get_example_config(517-518)holmes/plugins/toolsets/__init__.py (2)
load_builtin_toolsets(98-122)load_toolsets_from_file(48-62)tests/llm/utils/mock_dal.py (1)
_get_mock_file_path(66-67)test_mock_simple.py (1)
clear_mocks(32-35)holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py (7)
_invoke(129-190)_invoke(216-244)_invoke(265-281)get_parameterized_one_liner(192-193)get_parameterized_one_liner(246-247)get_parameterized_one_liner(283-284)get_example_config(48-54)holmes/core/toolset_manager.py (1)
load_custom_toolsets(384-421)holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (1)
get_example_config(44-48)holmes/plugins/toolsets/opensearch/opensearch_logs.py (1)
get_example_config(49-55)holmes/plugins/toolsets/internet/internet.py (1)
get_example_config(253-256)holmes/plugins/toolsets/grafana/base_grafana_toolset.py (1)
get_example_config(48-54)tests/test_check_prerequisites.py (1)
get_example_config(43-44)tests/test_holmes_sync_toolsets.py (1)
get_example_config(39-40)tests/plugins/toolsets/test_toolset_utils.py (1)
get_example_config(160-161)holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (1)
get_example_config(212-222)
🪛 Ruff (0.12.2)
tests/llm/conftest.py
51-53: Return the negated condition directly
Inline condition
(SIM103)
tests/llm/utils/test_mock_toolset.py
353-357: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
404-408: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
471-475: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
513-517: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
tests/llm/utils/mock_toolset.py
159-162: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
164-166: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
387-390: Use a single if statement instead of nested if statements
(SIM102)
💤 Files with no reviewable changes (57)
- tests/llm/fixtures/test_ask_holmes/40_disabled_toolset/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/31_basic_promql_graph_pod_memory/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/42_dns_issues_steps_old_tools/test_case.yaml
- tests/llm/fixtures/test_investigate/11_KubePodCrashLooping/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/37_argocd_wrong_namespace/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/42_dns_issues_result_new_tools/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/13_pending_node_selector/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/20_long_log_file_search/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/42_dns_issues_result_all_tools/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/08_sock_shop_frontend/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/45_fetch_deployment_logs_simple/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/16_failed_no_toolset_found/test_case.yaml
- tests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/56_kafka_runbook_no_tool/test_case.yaml
- tests/llm/fixtures/test_investigate/15_dns_resolution/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/42_dns_issues_result_old_tools/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/test_case.yaml
- tests/llm/fixtures/test_investigate/05_crashpod/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/06_explain_issue/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/54_not_truncated_when_getting_pods/test_case.yaml
- tests/llm/fixtures/test_investigate/13_Watchdog/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/51_logs_summarize_errors/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/33_http_latency_graph/test_case.yaml
- tests/llm/fixtures/test_investigate/03_cpu_throttling/test_case.yaml
- tests/llm/fixtures/test_investigate/08_memory_pressure/test_case.yaml
- tests/llm/fixtures/test_investigate/14_tempo/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/49_logs_since_last_week/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/46_job_crashing_no_longer_exists/test_case.yaml
- tests/llm/fixtures/test_investigate/12_KubePodNotReady/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/23_app_error_in_current_logs/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/30_basic_promql_graph_cluster_memory/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/50_logs_since_specific_date/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/55_kafka_runbook/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/39_failed_toolset/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/42_dns_issues_steps_new_tools/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/38_rabbitmq_split_head/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/27_permissions_error_no_helm_tools/test_case.yaml
- tests/llm/fixtures/test_investigate/06_job_failure/test_case.yaml
- tests/llm/fixtures/test_investigate/16_dns_resolution_no_tool/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/24_misconfigured_pvc/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/53_logs_find_term/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/48_logs_since_thursday/test_case.yaml
- tests/llm/fixtures/test_investigate/07_job_syntax_error/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/44_slack_statefulset_logs/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/43_slack_deployment_logs/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/47_truncated_logs_context_window/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/52_logs_login_issues/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/42_dns_issues_steps_new_all_tools/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/28_permissions_error_helm_tools_enabled/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/43_current_datetime_from_prompt/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/25_misconfigured_ingress_class/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/41_setup_argo/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/02_what_is_wrong_with_pod/kubectl_find_resource.txt
- tests/llm/fixtures/test_ask_holmes/02_what_is_wrong_with_pod/kubectl_describe.txt
- tests/llm/fixtures/test_ask_holmes/36_argocd_find_resource/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/02_what_is_wrong_with_pod/kubectl_logs.txt
- tests/llm/test_mocks.py
🧰 Additional context used
🧠 Learnings (9)
tests/llm/utils/braintrust.py (2)
Learnt from: nherment
PR: #436
File: tests/llm/utils/mock_utils.py:240-249
Timestamp: 2025-06-05T12:23:27.634Z
Learning: The holmesgpt project uses Python >= 3.10 and prefers modern type hint syntax like list[str], dict[str, int] over importing equivalent types from the typing module like List[str], Dict[str, int].
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.
tests/llm/fixtures/test_ask_holmes/01_how_many_pods/kubernetes_countitems_select_.metadata.namespace_test-1_.metadata.name_pod.txt (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.
tests/llm/utils/tags.py (1)
Learnt from: nherment
PR: #436
File: tests/llm/utils/mock_utils.py:240-249
Timestamp: 2025-06-05T12:23:27.634Z
Learning: The holmesgpt project uses Python >= 3.10 and prefers modern type hint syntax like list[str], dict[str, int] over importing equivalent types from the typing module like List[str], Dict[str, int].
tests/llm/fixtures/test_ask_holmes/02_what_is_wrong_with_pod/test_case.yaml (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.
CLAUDE.md (2)
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.
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.
docs/development/evals/writing.md (2)
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.
tests/llm/fixtures/test_ask_holmes/97_mock_error_partial_files/kubernetes_countitems_select_.metadata.namespace_kube-system_.metadata.name_pod.txt (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.
tests/llm/test_ask_holmes.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.
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🧬 Code Graph Analysis (5)
tests/llm/utils/commands.py (1)
tests/llm/utils/test_case_utils.py (1)
HolmesTestCase(43-58)
tests/llm/utils/tags.py (1)
tests/llm/utils/test_case_utils.py (1)
HolmesTestCase(43-58)
tests/llm/utils/mock_dal.py (1)
tests/llm/utils/test_case_utils.py (1)
read_file(18-20)
tests/llm/test_workload_health.py (4)
tests/llm/utils/mock_toolset.py (1)
MockToolsetManager(359-472)tests/llm/utils/test_case_utils.py (1)
HealthCheckTestCase(73-77)tests/llm/conftest.py (1)
mock_generation_config(86-109)tests/llm/utils/mock_dal.py (1)
MockSupabaseDal(14-73)
tests/llm/utils/mock_toolset.py (14)
holmes/core/tools.py (12)
StructuredToolResult(45-68)Tool(126-168)Toolset(331-473)ToolsetStatusEnum(102-105)_invoke(163-164)_invoke(215-243)invoke(142-160)get_parameterized_one_liner(167-168)get_parameterized_one_liner(194-201)get_example_config(465-466)get_example_config(484-485)get_example_config(517-518)holmes/plugins/toolsets/__init__.py (2)
load_builtin_toolsets(98-122)load_toolsets_from_file(48-62)tests/llm/utils/mock_dal.py (1)
_get_mock_file_path(66-67)test_mock_simple.py (1)
clear_mocks(32-35)holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py (7)
_invoke(129-190)_invoke(216-244)_invoke(265-281)get_parameterized_one_liner(192-193)get_parameterized_one_liner(246-247)get_parameterized_one_liner(283-284)get_example_config(48-54)holmes/core/toolset_manager.py (1)
load_custom_toolsets(384-421)holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (1)
get_example_config(44-48)holmes/plugins/toolsets/opensearch/opensearch_logs.py (1)
get_example_config(49-55)holmes/plugins/toolsets/internet/internet.py (1)
get_example_config(253-256)holmes/plugins/toolsets/grafana/base_grafana_toolset.py (1)
get_example_config(48-54)tests/test_check_prerequisites.py (1)
get_example_config(43-44)tests/test_holmes_sync_toolsets.py (1)
get_example_config(39-40)tests/plugins/toolsets/test_toolset_utils.py (1)
get_example_config(160-161)holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (1)
get_example_config(212-222)
🪛 Ruff (0.12.2)
tests/llm/conftest.py
51-53: Return the negated condition directly
Inline condition
(SIM103)
tests/llm/utils/test_mock_toolset.py
353-357: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
404-408: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
471-475: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
513-517: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
tests/llm/utils/mock_toolset.py
159-162: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
164-166: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
387-390: Use a single if statement instead of nested if statements
(SIM102)
⏰ 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: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (31)
docs/development/evals/index.md (1)
147-148: Doc update looks good – just ensure consistency across all docs
The wording now correctly references the new CLI flags. Double-check that every other doc (e.g.writing.md, README snippets, CLI--help) no longer mentions the deprecatedgenerate_mocks: trueYAML key so readers don’t get mixed messages.tests/llm/utils/braintrust.py (1)
9-9: Import path update LGTM
The module relocation is reflected correctly.tests/llm/utils/commands.py (1)
7-7: Import path update acknowledged
No further issues spotted.tests/llm/utils/tags.py (1)
2-2: Import path update acknowledged
Looks correct.tests/llm/utils/langfuse.py (1)
7-11: Import path update acknowledged
All three case classes now come from the new central module – good catch.tests/llm/fixtures/test_ask_holmes/02_what_is_wrong_with_pod/test_case.yaml (1)
1-1: Verify fixture consistency after pod-name changeThe prompt now references
foo-bar-6958c5bdd8-69gtn. Please double-check that all related mock files and any hard-coded expectations were updated accordingly; otherwise this test will start failing.tests/llm/fixtures/test_ask_holmes/01_how_many_pods/kubernetes_countitems_select_.metadata.namespace_test-1_.metadata.name_pod.txt (1)
2-2: Confirm parser tolerates droppeddatakeyThe
"data": nullproperty was removed. If any code deserialises this fixture and expects adatafield (even when null), it will now raiseKeyError. Please verify downstream parsing logic.tests/llm/fixtures/test_ask_holmes/97_mock_error_partial_files/kubernetes_countitems_select_.metadata.namespace_kube-system_.metadata.name_pod.txt (1)
1-2: Ensure schema consistency across fixturesThis new file keeps
"data": nullwhile other updated fixtures dropped the field. Aligning the schema in all fixtures avoids brittle parsers and confusing diffs.tests/llm/test_ask_holmes.py (5)
20-20: LGTM!The import correctly reflects the new mock toolset management system.
65-71: LGTM!The updated function signature properly integrates with the new mock generation configuration system.
136-160: LGTM!Comprehensive mock failure data recording that integrates well with the test reporting infrastructure.
264-274: LGTM!The function correctly initializes the new MockToolsetManager with proper configuration passing.
276-278: LGTM!Good documentation of the architectural change - the new lazy-loading approach is more efficient.
tests/llm/utils/test_mock_toolset.py (6)
2-21: LGTM!Comprehensive imports covering all components of the new mock toolset system.
104-104: LGTM!Function signature correctly updated to use MockToolsetManager.
130-210: LGTM!Comprehensive test coverage for MockFileManager including path generation, I/O operations, error handling, and mock clearing with proper pytest tracking.
212-333: LGTM!Excellent test coverage for all MockableToolWrapper modes (LIVE, MOCK, GENERATE) with proper verification of behavior and error handling.
335-346: LGTM!Simple but adequate test for ToolsetConfigurator with proper mocking.
511-573: LGTM!Comprehensive test for generate mode ensuring mocks are created without errors when missing.
tests/llm/conftest.py (5)
31-83: Great abstraction for test status handling!The TestStatus class provides a clean, centralized way to determine test status with proper handling of mock failures.
85-110: LGTM!Well-designed session-scoped fixture that properly handles configuration precedence and mode determination.
127-154: LGTM!Well-implemented properties with robust parsing logic and proper error handling for various nodeid formats.
366-485: Excellent enhancement to test result collection!The function now comprehensively tracks mock operations and detects mock failures from multiple sources, providing rich data for reporting.
755-825: Excellent mock operations reporting!The function provides comprehensive feedback about mock operations with a helpful review checklist. This will greatly assist users in maintaining consistent mock data.
tests/llm/utils/mock_toolset.py (7)
24-59: Excellent exception hierarchy design!The custom exceptions provide clear, actionable error messages that guide users through resolution options. The separation of concerns (not found, corrupted, validation) enables precise error handling.
84-106: LGTM!Comprehensive filename sanitization handling URLs, special characters, and edge cases. Well-tested as evidenced by the extensive test cases.
108-228: Well-designed file management abstraction!MockFileManager cleanly encapsulates all file operations with proper error handling and pytest integration.
230-298: Excellent tool wrapper implementation!Clean separation of concerns for each mode (LIVE, MOCK, GENERATE) with proper pytest integration for tracking generated mocks.
300-347: LGTM!Clean separation of toolset configuration responsibilities with proper error handling.
349-357: LGTM!Minimal but sufficient mock toolset implementation for testing purposes.
359-476: Well-architected mock toolset manager!The class effectively orchestrates all components of the mock system with proper mode handling and toolset wrapping logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/llm/utils/mock_toolset.py (1)
158-166: Add exception chaining for better error traceability.tests/llm/utils/test_mock_toolset.py (1)
114-117: Consider fixing or removing this test.
🧹 Nitpick comments (3)
tests/llm/utils/mock_toolset.py (1)
378-385: Simplify nested if statements.The nested if statements can be combined for better readability.
- if mock_generation_config.regenerate_all_mocks: - if request and test_case_folder not in getattr( - mock_generation_config, "_cleared_folders", set() - ): + if (mock_generation_config.regenerate_all_mocks and + request and + test_case_folder not in getattr(mock_generation_config, "_cleared_folders", set())): self.file_manager.clear_mocks(request) if not hasattr(mock_generation_config, "_cleared_folders"): mock_generation_config._cleared_folders = set() mock_generation_config._cleared_folders.add(test_case_folder)tests/llm/utils/test_mock_toolset.py (1)
358-362: Combine nested with statements for cleaner code.Multiple locations in the test file use nested
withstatements that can be combined.Example refactor for lines 358-362:
- with patch( - "holmes.plugins.toolsets.service_discovery.find_service_url", - return_value="http://mock-prometheus:9090", - ): - with tempfile.TemporaryDirectory() as tmpdir: + with ( + patch( + "holmes.plugins.toolsets.service_discovery.find_service_url", + return_value="http://mock-prometheus:9090", + ), + tempfile.TemporaryDirectory() as tmpdir + ):Apply similar refactoring to the other locations (lines 415-419, 487-491, 535-539).
Also applies to: 415-419, 487-491, 535-539
tests/llm/conftest.py (1)
47-53: Simplify the negated condition.The static analysis tool correctly identifies that this condition can be simplified by returning the negated condition directly.
- @property - def is_regression(self) -> bool: - if self.passed or self.is_mock_failure: - return False - # Known failure (expected to fail) - if self.actual_score == 0 and self.expected_score == 0: - return False - return True + @property + def is_regression(self) -> bool: + if self.passed or self.is_mock_failure: + return False + # Known failure (expected to fail) + return not (self.actual_score == 0 and self.expected_score == 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/development/evals/writing.md(3 hunks)tests/llm/conftest.py(17 hunks)tests/llm/test_ask_holmes.py(5 hunks)tests/llm/test_investigate.py(4 hunks)tests/llm/test_workload_health.py(3 hunks)tests/llm/utils/mock_toolset.py(2 hunks)tests/llm/utils/test_case_utils.py(1 hunks)tests/llm/utils/test_mock_toolset.py(3 hunks)
🧠 Learnings (2)
tests/llm/test_ask_holmes.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.
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🧬 Code Graph Analysis (1)
tests/llm/test_ask_holmes.py (4)
tests/llm/utils/mock_toolset.py (1)
MockToolsetManager(359-463)tests/llm/utils/test_case_utils.py (3)
AskHolmesTestCase(59-61)Evaluation(27-29)MockHelper(78-155)tests/llm/conftest.py (1)
mock_generation_config(86-109)tests/llm/utils/commands.py (1)
set_test_env_vars(54-80)
🪛 Ruff (0.12.2)
tests/llm/utils/test_mock_toolset.py
358-362: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
415-419: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
487-491: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
535-539: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
tests/llm/conftest.py
51-53: Return the negated condition directly
Inline condition
(SIM103)
tests/llm/utils/mock_toolset.py
159-162: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
164-166: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
378-381: Use a single if statement instead of nested if statements
(SIM102)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/development/evals/writing.md
- tests/llm/test_workload_health.py
🧰 Additional context used
🧠 Learnings (2)
tests/llm/test_ask_holmes.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.
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🧬 Code Graph Analysis (1)
tests/llm/test_ask_holmes.py (4)
tests/llm/utils/mock_toolset.py (1)
MockToolsetManager(359-463)tests/llm/utils/test_case_utils.py (3)
AskHolmesTestCase(59-61)Evaluation(27-29)MockHelper(78-155)tests/llm/conftest.py (1)
mock_generation_config(86-109)tests/llm/utils/commands.py (1)
set_test_env_vars(54-80)
🪛 Ruff (0.12.2)
tests/llm/utils/test_mock_toolset.py
358-362: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
415-419: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
487-491: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
535-539: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
tests/llm/conftest.py
51-53: Return the negated condition directly
Inline condition
(SIM103)
tests/llm/utils/mock_toolset.py
159-162: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
164-166: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
378-381: Use a single if statement instead of nested if statements
(SIM102)
⏰ 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
🔇 Additional comments (21)
tests/llm/utils/mock_toolset.py (4)
24-59: Well-designed exception hierarchy with helpful error messages.The custom exception classes provide clear error categorization and actionable instructions for users. The error messages effectively guide users through different resolution options (live mode, generate mocks, regenerate all).
84-106: Comprehensive filename sanitization implementation.The function properly handles URL schemes, percent-encoding, special characters, and edge cases. Good attention to detail with consolidating underscores and stripping invalid prefixes/suffixes.
230-298: Clean implementation of mock mode handling.The wrapper correctly dispatches tool execution based on mode, with proper error handling for missing mocks and tracking for generated mocks. The integration with pytest's user_properties for tracking is well done.
300-347: Well-structured toolset configuration logic.Good separation of concerns with static methods for loading and configuring toolsets. The prerequisite checking with proper error logging is a nice touch.
tests/llm/utils/test_case_utils.py (1)
14-15: Clean separation of mock handling concerns.Good refactoring to remove mock-related functionality from test case utilities. The comment clearly indicates where the functionality has moved.
tests/llm/test_investigate.py (1)
23-51: Successful integration of the new mock system.The migration from MockToolsets to MockToolsetManager is clean, and the removal of explicit mock_tool() calls simplifies the test code. The comment explaining automatic mock loading is helpful.
tests/llm/test_ask_holmes.py (2)
130-159: Excellent mock error detection and reporting.The enhanced exception handling properly detects mock data errors through both exception type checking and inheritance hierarchy inspection. The detailed failure information recorded in user_properties enables comprehensive test reporting.
242-257: Good defensive check for mock errors in output.Smart addition to check the output string for mock error indicators, catching cases where errors might be captured as output rather than exceptions.
tests/llm/utils/test_mock_toolset.py (2)
24-102: Excellent test coverage for filename sanitization.The parametrized test covers a comprehensive set of edge cases including URL schemes, encoding, special characters, and various edge conditions. Well done!
353-600: Successful migration of parameter matching tests.The tests from the deleted test_mocks.py have been properly adapted to the new file-based mock system. Good coverage of exact matching, parameter-agnostic matching, and error cases.
tests/llm/conftest.py (11)
32-83: Excellent encapsulation of test status logic.The TestStatus class provides clean separation of concerns and consistent status determination across different output formats. The mock failure integration is well-designed.
85-109: Well-designed centralized mock configuration.The session-scoped fixture provides excellent centralization of mock generation logic. The mode determination is clear and the relationship between CLI flags is properly handled.
Consider moving the MockGenerationConfig class to a separate module if it grows more complex, but the current nested approach is clean and appropriate.
127-154: Robust test identification from pytest nodeids.The test_id and test_name properties provide clean extraction of test information from pytest nodeids with proper error handling. The implementation correctly handles edge cases and provides fallbacks.
156-169: Clean CLI option definitions.The pytest command line options are well-defined with clear help text that explains the relationship between the options. The naming is intuitive and follows pytest conventions.
366-485: Comprehensive test result collection with robust mock tracking.The enhanced collection function provides excellent mock tracking capabilities and thorough error detection. The xdist compatibility through user_properties is crucial for parallel test execution. While complex, the function appropriately handles the various data sources and edge cases.
The function is necessarily complex due to the comprehensive tracking requirements, but the logic is well-structured and handles edge cases appropriately.
488-516: Clean URL generation with proper encoding.The standalone get_braintrust_url function provides good separation of concerns with explicit parameters and proper URL encoding. The implementation is clean and maintainable.
519-598: Enhanced markdown reporting with consistent status handling.The markdown report generation effectively uses the new TestStatus class for consistent status determination and symbols. The integration with Braintrust URLs and comprehensive legend provides excellent user experience.
614-621: Improved table formatting for better readability.The column width adjustments and text wrapping improvements enhance terminal output readability. The specific width constraints help ensure the table fits within standard terminal widths.
722-747: Excellent context-aware analysis for mock failures.The addition of mock data context to the LLM analysis prompt provides clear guidance for resolving mock-related test failures. The detailed instructions with multiple solution options help developers understand their choices.
761-830: Comprehensive mock operations reporting.The _report_mock_operations function provides excellent feedback on mock generation activities. The detailed breakdown by test case, clear statistics, and helpful review checklist significantly improve the developer experience when working with mocks.
The checklist items are particularly valuable for ensuring mock data quality and consistency.
19-19: MockMode import is valid
TheMockModeenum is defined intests/llm/utils/mock_toolset.py, so the import is correct and the type exists. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/llm/conftest.py (1)
23-23: LLM_TEST_TYPES updated correctly.The addition of "test_workload_health" to the list addresses the previous review comment about missing workload health tests. This ensures all LLM tests are properly recognized.
tests/llm/utils/mock_toolset.py (1)
108-289: Comprehensive MockFileManager with efficient caching.The file manager provides robust mock file operations with parameter matching, caching for performance, and proper error handling. The
_params_matchmethod correctly handles both specific and wildcard parameter matching.However, there are missing exception chains as flagged in past reviews:
The exception chaining issue in lines 159-166 needs to be addressed as identified in the previous review.
🧹 Nitpick comments (1)
tests/llm/utils/mock_toolset.py (1)
439-446: Consider simplifying nested if statement.Static analysis correctly identifies an opportunity to combine nested conditions for better readability.
- if mock_generation_config.regenerate_all_mocks: - if request and test_case_folder not in getattr( - mock_generation_config, "_cleared_folders", set() - ): + if (mock_generation_config.regenerate_all_mocks and + request and test_case_folder not in getattr( + mock_generation_config, "_cleared_folders", set() + )):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/llm/conftest.py(17 hunks)tests/llm/utils/mock_dal.py(2 hunks)tests/llm/utils/mock_toolset.py(2 hunks)
🧠 Learnings (1)
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🧬 Code Graph Analysis (1)
tests/llm/conftest.py (1)
tests/llm/utils/mock_toolset.py (1)
MockMode(61-66)
🪛 Ruff (0.12.2)
tests/llm/conftest.py
51-53: Return the negated condition directly
Inline condition
(SIM103)
tests/llm/utils/mock_toolset.py
439-442: Use a single if statement instead of nested if statements
(SIM102)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/llm/utils/mock_dal.py
🧰 Additional context used
🧠 Learnings (1)
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🧬 Code Graph Analysis (1)
tests/llm/conftest.py (1)
tests/llm/utils/mock_toolset.py (1)
MockMode(61-66)
🪛 Ruff (0.12.2)
tests/llm/conftest.py
51-53: Return the negated condition directly
Inline condition
(SIM103)
tests/llm/utils/mock_toolset.py
439-442: Use a single if statement instead of nested if statements
(SIM102)
⏰ 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 (11)
tests/llm/conftest.py (6)
32-83: Well-designed TestStatus class with comprehensive status logic.The new TestStatus class effectively centralizes test status determination, regression detection, and provides consistent symbols for different output formats. The logic correctly handles mock failures, regressions, and known failures.
85-109: Excellent centralized mock generation configuration.The session-scoped fixture properly consolidates mock generation logic from CLI options and environment variables. The configuration class approach provides clean encapsulation of the three related settings.
127-154: Smart property-based approach for test ID/name extraction.Moving the test ID and name extraction logic into TestResult properties eliminates helper functions and provides cleaner, more maintainable code. The fallback logic for parsing nodeids is robust.
156-169: Well-designed CLI options for mock generation control.The two CLI flags provide clear separation between generating missing mocks vs. regenerating all mocks, with proper help text explaining the difference. The implicit relationship where
--regenerate-all-mocksimplies--generate-mocksis logical.
369-429: Comprehensive mock tracking data collection.The enhanced
_collect_test_results_from_statsfunction properly tracks mock operations, failures, and integrates with the new mock toolset system. The mock error type detection covers multiple sources (longrepr, logs) for robust failure identification.
761-830: Excellent mock operations reporting functionality.The
_report_mock_operationsfunction provides detailed, well-formatted summaries of mock file operations with helpful checklists for reviewing generated mocks. The grouping by test case and comprehensive statistics enhance the developer experience.tests/llm/utils/mock_toolset.py (5)
24-67: Well-designed exception hierarchy with helpful error messages.The custom exception classes provide clear categorization of mock data errors and include helpful guidance for users on how to resolve issues. The base MockDataError class includes actionable suggestions with multiple resolution options.
84-106: Robust filename sanitization with comprehensive URL handling.The sanitization function properly handles URL schemes, percent-encoding, and invalid filename characters. The consolidation of consecutive underscores and trimming of edge characters creates clean, readable filenames.
291-359: Clean tool wrapper implementation with proper mode handling.The MockableToolWrapper correctly implements the three modes (LIVE, MOCK, GENERATE) with appropriate error handling and mock file tracking via pytest user_properties. The delegation pattern maintains the original tool interface.
361-407: Well-structured toolset configuration management.The ToolsetConfigurator provides clean separation of concerns for loading builtin/custom toolsets and applying configuration. The prerequisite checking with proper error logging is a good practice.
420-524: Comprehensive MockToolsetManager with smart toolset wrapping.The main manager class effectively orchestrates the mock system lifecycle, from initialization through toolset wrapping. The logic for determining which toolsets to include based on enabled status or existing mocks is well-designed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/llm/conftest.py (1)
23-23: Past concern about missing test_workload_health has been addressed.The
test_workload_healthhas been correctly added to theLLM_TEST_TYPESlist, resolving the previous review comment's concern.
🧹 Nitpick comments (2)
tests/llm/conftest.py (1)
47-53: Simplify boolean logic.The condition can be simplified for better readability.
Apply this simplification:
@property def is_regression(self) -> bool: - if self.passed or self.is_mock_failure: - return False - # Known failure (expected to fail) - if self.actual_score == 0 and self.expected_score == 0: - return False - return True + # Not a regression if test passed, is mock failure, or is known failure + return not ( + self.passed + or self.is_mock_failure + or (self.actual_score == 0 and self.expected_score == 0) + )tests/llm/utils/mock_toolset.py (1)
444-451: Simplify nested if statements.The nested if conditions can be combined for better readability.
Apply this simplification:
- if mock_generation_config.regenerate_all_mocks: - if request and test_case_folder not in getattr( - mock_generation_config, "_cleared_folders", set() - ): + if (mock_generation_config.regenerate_all_mocks and + request and test_case_folder not in getattr( + mock_generation_config, "_cleared_folders", set() + )): self.file_manager.clear_mocks(request) if not hasattr(mock_generation_config, "_cleared_folders"): mock_generation_config._cleared_folders = set() mock_generation_config._cleared_folders.add(test_case_folder)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/llm/conftest.py(17 hunks)tests/llm/test_ask_holmes.py(6 hunks)tests/llm/test_investigate.py(5 hunks)tests/llm/test_workload_health.py(3 hunks)tests/llm/utils/mock_toolset.py(2 hunks)
🧠 Learnings (2)
tests/llm/test_ask_holmes.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.
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🧬 Code Graph Analysis (1)
tests/llm/conftest.py (1)
tests/llm/utils/mock_toolset.py (1)
MockMode(61-66)
🪛 Ruff (0.12.2)
tests/llm/conftest.py
51-53: Return the negated condition directly
Inline condition
(SIM103)
tests/llm/utils/mock_toolset.py
444-447: Use a single if statement instead of nested if statements
(SIM102)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/llm/test_workload_health.py
🧰 Additional context used
🧠 Learnings (2)
tests/llm/test_ask_holmes.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.
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🧬 Code Graph Analysis (1)
tests/llm/conftest.py (1)
tests/llm/utils/mock_toolset.py (1)
MockMode(61-66)
🪛 Ruff (0.12.2)
tests/llm/conftest.py
51-53: Return the negated condition directly
Inline condition
(SIM103)
tests/llm/utils/mock_toolset.py
444-447: Use a single if statement instead of nested if statements
(SIM102)
⏰ 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 (11)
tests/llm/test_investigate.py (3)
37-41: LGTM! Centralized mock generation configuration.The addition of the
mock_generation_configparameter properly integrates with the new centralized mock generation approach via pytest fixtures, replacing the previous per-test-case configuration flags.
44-51: Excellent refactoring to file-based mock system.The transition from
MockToolsetstoMockToolsetManagerwith automatic mock loading from disk is a clear architectural improvement. The helpful comments explain the new approach well.
150-158: Good addition for test traceability.Storing span IDs in user properties enables external access to tracing information and supports pytest-xdist parallel execution. This enhances integration with Braintrust evaluation links.
tests/llm/test_ask_holmes.py (3)
140-168: Excellent mock error detection and reporting.The sophisticated exception handling properly detects
MockDataErrorand its subclasses using both type name checking and MRO inspection. Recording failure metadata in user properties enables better test result analysis and debugging.
252-266: Comprehensive mock failure detection.The output scanning for mock error types provides an additional safety net beyond exception handling. The comprehensive list of error types and descriptive failure messages will help with debugging test issues.
273-283: Clean integration with new mock system.The function signature updates and
MockToolsetManagerintegration properly align with the file-based mock system. The comment about automatic mock loading clarifies the new approach.tests/llm/conftest.py (2)
85-109: Excellent centralized mock generation configuration.The session-scoped fixture successfully centralizes mock generation configuration, replacing per-test-case flags. The mode determination logic is clear and the MockGenerationConfig class provides a clean interface.
127-154: Clean property-based approach for test identification.The
test_idandtest_nameproperties provide a cleaner interface than separate helper functions. The parsing logic is robust with appropriate error handling for malformed nodeids.tests/llm/utils/mock_toolset.py (3)
25-58: Excellent exception hierarchy with helpful user guidance.The custom exception classes provide a clear hierarchy and include helpful error messages with specific fix suggestions. This will significantly improve the debugging experience for mock-related issues.
84-105: Robust filename sanitization implementation.The function comprehensively handles URL scheme removal, percent-decoding, invalid character replacement, and cleanup. This should handle most problematic filename scenarios effectively.
296-364: Clean and well-structured tool wrapper design.The
MockableToolWrapperclass effectively handles the three modes with clear separation of concerns. The error handling for missing mocks and tracking of generated files via pytest user properties is well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/llm/utils/mock_toolset.py (1)
306-312: Missing exception chaining for error traceability.The exception handling lacks proper chaining which hinders debugging.
Apply this fix for proper exception chaining:
- except Exception as e: - logging.warning(f"Failed to load mock file {file_path}: {e}") - continue + except Exception as e: + logging.warning(f"Failed to load mock file {file_path}: {e}") + continueNote: The ValueError re-raise is correct to propagate format errors.
🧹 Nitpick comments (1)
tests/llm/utils/mock_toolset.py (1)
478-485: Simplify nested conditional structure.The nested if statements can be consolidated for better readability.
- if mock_generation_config.regenerate_all_mocks: - if request and test_case_folder not in getattr( - mock_generation_config, "_cleared_folders", set() - ): + if (mock_generation_config.regenerate_all_mocks and + request and test_case_folder not in getattr( + mock_generation_config, "_cleared_folders", set() + )): self.file_manager.clear_mocks(request) if not hasattr(mock_generation_config, "_cleared_folders"): mock_generation_config._cleared_folders = set() mock_generation_config._cleared_folders.add(test_case_folder)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/llm/utils/mock_toolset.py(2 hunks)
🧠 Learnings (1)
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🪛 Ruff (0.12.2)
tests/llm/utils/mock_toolset.py
281-283: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
478-481: Use a single if statement instead of nested if statements
(SIM102)
🧰 Additional context used
🧠 Learnings (1)
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🪛 Ruff (0.12.2)
tests/llm/utils/mock_toolset.py
281-283: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
478-481: Use a single if statement instead of nested if statements
(SIM102)
⏰ 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
🔇 Additional comments (9)
tests/llm/utils/mock_toolset.py (9)
24-67: Well-designed exception hierarchy and enum.The exception classes provide clear, actionable error messages with helpful guidance for users, and the MockMode enum is clean and descriptive.
84-106: Comprehensive filename sanitization logic.The
sanitize_filenamefunction handles URL schemes, percent-encoding, invalid characters, and edge cases effectively. Good defensive programming.
108-164: Solid mock file management design.The MockFileManager class provides a well-structured approach to file-based mocking with caching, parameter matching, and clear separation of concerns.
267-284: Excellent file format migration handling.The detailed error message and format migration logic provides clear guidance for users upgrading from the old format. The PR reference and specific format explanation are very helpful.
316-398: Well-implemented tool wrapper with comprehensive mode handling.The MockableToolWrapper effectively handles all three modes with appropriate error handling and helpful diagnostics. The pytest user_properties integration for tracking is a nice touch.
400-447: Clean separation of toolset configuration concerns.The ToolsetConfigurator provides a well-structured approach to loading and configuring toolsets with proper error handling for prerequisites.
449-488: Well-designed mock toolset management with clear initialization.The MockToolsetManager provides good orchestration of the mock system with proper lifecycle management and configuration.
509-563: Intelligent toolset wrapping logic.The _wrap_toolsets method effectively handles different modes and includes smart logic to only wrap toolsets that are enabled or have existing mocks. This prevents unnecessary toolset initialization.
566-582: Good backward compatibility and comprehensive exports.The backward compatibility alias and complete all list ensure smooth migration and clear public API definition.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/llm/utils/mock_toolset.py (1)
281-284: Exception chaining issue already identified.This is the same exception chaining issue flagged in previous reviews. The fix is to add
from eto the raise statement as suggested in the existing comment.tests/llm/conftest.py (1)
32-83: TestStatus class design is solid but TODO remains unresolved.The class provides good encapsulation of test status logic. The TODO comment on line 44 about mock failures affecting the
passedproperty should be resolved as mentioned in previous reviews.
🧹 Nitpick comments (2)
tests/llm/utils/mock_toolset.py (1)
479-486: Consider simplifying nested if statements.The nested if statements could be combined for better readability:
- if mock_generation_config.regenerate_all_mocks: - if request and test_case_folder not in getattr( - mock_generation_config, "_cleared_folders", set() - ): + if (mock_generation_config.regenerate_all_mocks and + request and test_case_folder not in getattr( + mock_generation_config, "_cleared_folders", set() + )):tests/llm/conftest.py (1)
51-53: Consider simplifying the condition in is_regression property.The condition can be simplified by returning the negated result directly:
- if self.actual_score == 0 and self.expected_score == 0: - return False - return True + return not (self.actual_score == 0 and self.expected_score == 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/llm/conftest.py(17 hunks)tests/llm/utils/mock_toolset.py(2 hunks)
🧠 Learnings (1)
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🪛 Ruff (0.12.2)
tests/llm/conftest.py
51-53: Return the negated condition directly
Inline condition
(SIM103)
tests/llm/utils/mock_toolset.py
281-284: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
479-482: Use a single if statement instead of nested if statements
(SIM102)
🧰 Additional context used
🧠 Learnings (1)
tests/llm/utils/mock_toolset.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🪛 Ruff (0.12.2)
tests/llm/conftest.py
51-53: Return the negated condition directly
Inline condition
(SIM103)
tests/llm/utils/mock_toolset.py
281-284: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
479-482: Use a single if statement instead of nested if statements
(SIM102)
⏰ 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
🔇 Additional comments (13)
tests/llm/utils/mock_toolset.py (7)
24-59: Excellent custom exception hierarchy design.The exception classes provide clear separation of concerns and helpful error messages with actionable solutions. The conditional message formatting based on
tool_namepresence is a thoughtful UX improvement.
61-66: Good use of enum for mode management.The
MockModeenum provides better type safety and readability compared to boolean flags, making the code more maintainable.
84-105: Well-implemented filename sanitization utility.The function properly handles URL schemes, percent-encoding, and filesystem-safe character conversion with good edge case handling.
108-314: Well-designed MockFileManager with comprehensive functionality.The class effectively encapsulates mock file operations with proper caching, parameter matching logic, and format validation. The old format detection and helpful error messages are particularly valuable for migration.
317-399: Excellent tool wrapper design with clear mode separation.The
MockableToolWrapperprovides clean abstractions for different execution modes while maintaining the original tool interface. The pytest integration for tracking mock operations is well implemented.
401-447: Clean toolset configuration with good separation of concerns.The static methods provide clear utility functions for loading and configuring toolsets. The prerequisite checking with proper error logging is well implemented.
460-583: Comprehensive mock toolset management with good integration.The
MockToolsetManagereffectively orchestrates the mock system components and integrates well with pytest fixtures. The backward compatibility alias and comprehensive exports show good API design considerations.tests/llm/conftest.py (6)
23-29: Good addition of test_workload_health to LLM_TEST_TYPES.The inclusion of
test_workload_healthresolves the previous concern about workload health tests not being recognized as LLM tests.
85-109: Excellent centralized mock generation configuration.The session-scoped fixture provides clean centralization of mock configuration with proper handling of CLI options, environment variables, and mode determination logic.
112-154: Well-designed TestResult enhancements with robust property extraction.The addition of
mock_data_failurefield and the computed properties for extracting test ID and name from nodeid are cleanly implemented with proper error handling.
156-169: Clear and useful CLI options for mock generation control.The
--generate-mocksand--regenerate-all-mocksoptions provide intuitive control over mock data generation with helpful descriptions.
376-505: Comprehensive test result collection with excellent mock failure detection.The enhanced collection logic properly tracks mock operations and detects failures through multiple channels (user_properties, longrepr, captured logs). The integration with TestResult properties is clean.
822-892: Excellent mock operations reporting with helpful user guidance.The detailed reporting of mock operations, failures, and the review checklist provides valuable feedback for users managing mock data. The organized presentation and actionable guidance are well thought out.
aantn
changed the title
2 participants
No description provided.