move some evals out of easy + delete bad eval - #828
Conversation
WalkthroughAdjusted test metadata: multiple test_case.yaml files retagged from easy to medium; one test marked to skip due to missing data; one test scenario and its Kubernetes manifest removed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
🔭 Outside diff range comments (4)
tests/llm/fixtures/test_ask_holmes/73a_time_window_anomaly/test_case.yaml (1)
1-1: Fix namespace to app-73a to comply with test guidelines.Per coding guidelines: “Each test must use a dedicated namespace app-.” This test uses namespace-73a in user_prompt and kubectl commands. Please rename to app-73a across the file to avoid conflicts and ensure consistency.
Outside the changed lines, suggested updates:
# user_prompt user_prompt: "Were there issues with the task-scheduler pod in app-73a?" # before_test kubectl create namespace app-73a || true kubectl create secret generic task-scheduler-logs-script --from-file=generate_logs.py=./generate_logs.py -n app-73a --dry-run=client -o yaml | kubectl apply -f - # after_test kubectl delete secret task-scheduler-logs-script -n app-73a --ignore-not-found kubectl delete namespace app-73a --ignore-not-foundAlso applies to: 11-12, 16-18
tests/llm/fixtures/test_ask_holmes/52_logs_login_issues/test_case.yaml (1)
11-12: Fix namespace to app-52 to comply with test guidelines.Per coding guidelines: “Each test must use a dedicated namespace app-.” This test uses ask-holmes-namespace-52. Please rename to app-52 in setup/teardown commands.
Outside the changed lines, suggested updates:
# before_test kubectl create namespace app-52 || true kubectl create secret generic my-app-52-logs-script \ --from-file=generate_logs.py=./generate_logs.py \ -n app-52 --dry-run=client -o yaml | kubectl apply -f - # after_test kubectl delete secret my-app-52-logs-script -n app-52 --ignore-not-found kubectl delete namespace app-52 --ignore-not-foundAlso applies to: 19-20
tests/llm/fixtures/test_ask_holmes/77_liveness_probe_misconfiguration/test_case.yaml (2)
1-1: Namespace naming violates tests policy; use app- (app-77), not “namespace-77”.Per guidelines, each test must use a dedicated namespace app-. Please update the prompt and underlying manifest to app-77. Also prefer unique, neutral pod names; suggest web-app-77 to satisfy “unique across tests.”
Apply this diff in this file:
-user_prompt: "Why does the web-app pod keep restarting in namespace-77?" +user_prompt: "Why does the web-app-77 pod keep restarting in app-77?"And update your manifest accordingly (outside this file), e.g.:
apiVersion: v1 kind: Namespace metadata: name: app-77 --- apiVersion: apps/v1 kind: Deployment metadata: name: web-app-77 namespace: app-77 # ...
4-8: Validate manifest compliance: use Secrets for scripts, neutral names, minimal footprint, and uniqueness.I verified the contents of
tests/llm/fixtures/test_ask_holmes/77_liveness_probe_misconfiguration/manifest.yaml:
- Namespace is set to
namespace-77(a neutral, test-specific name).- Only a Deployment is defined—no ConfigMaps or inline shell scripts.
- All
metadata.namefields are free of diagnostic hints (no “liveness”, “probe”, “misconfig”, etc.).- There is no
resources.requestsorresources.limitssection under the container spec.Please address the following before merging:
- Add minimal
resources.requestsandresources.limitsto each container inspec.template.spec.containers.- Confirm that
namespace-77and the defined resource names are unique across all test fixtures to avoid cross-test collisions.
🧹 Nitpick comments (1)
tests/llm/fixtures/test_ask_holmes/77_liveness_probe_misconfiguration/test_case.yaml (1)
4-8: Avoid fixed sleeps; prefer event/condition-based waits to reduce flakiness.Sleeping 45s can slow CI or still be insufficient on slower clusters. Consider waiting on a concrete condition or observable (e.g., at least one restart observed, or probe failures recorded), then proceed. If the harness lacks helpers, we can add a small wait utility in the framework.
I can propose a portable “wait for restart” helper for your test harness if you share the common utilities available to tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tests/llm/fixtures/test_ask_holmes/100_historical_logs/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/29_events_from_alert_manager/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/52_logs_login_issues/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/73a_time_window_anomaly/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/77_liveness_probe_misconfiguration/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/88_affinity_like_but_taints/manifest.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/88_affinity_like_but_taints/test_case.yaml(0 hunks)tests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/test_case.yaml(1 hunks)
💤 Files with no reviewable changes (2)
- tests/llm/fixtures/test_ask_holmes/88_affinity_like_but_taints/manifest.yaml
- tests/llm/fixtures/test_ask_holmes/88_affinity_like_but_taints/test_case.yaml
🧰 Additional context used
📓 Path-based instructions (3)
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/llm/fixtures/test_ask_holmes/29_events_from_alert_manager/test_case.yamltests/llm/fixtures/test_ask_holmes/73a_time_window_anomaly/test_case.yamltests/llm/fixtures/test_ask_holmes/77_liveness_probe_misconfiguration/test_case.yamltests/llm/fixtures/test_ask_holmes/100_historical_logs/test_case.yamltests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/test_case.yamltests/llm/fixtures/test_ask_holmes/52_logs_login_issues/test_case.yaml
**/*.yaml
📄 CodeRabbit Inference Engine (CLAUDE.md)
ALWAYS use Secrets for scripts, not inline manifests or ConfigMaps (prevents code visibility with kubectl describe)
Files:
tests/llm/fixtures/test_ask_holmes/29_events_from_alert_manager/test_case.yamltests/llm/fixtures/test_ask_holmes/73a_time_window_anomaly/test_case.yamltests/llm/fixtures/test_ask_holmes/77_liveness_probe_misconfiguration/test_case.yamltests/llm/fixtures/test_ask_holmes/100_historical_logs/test_case.yamltests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/test_case.yamltests/llm/fixtures/test_ask_holmes/52_logs_login_issues/test_case.yaml
tests/**/*.yaml
📄 CodeRabbit Inference Engine (CLAUDE.md)
tests/**/*.yaml: Never use names that hint at the problem or expected behavior in resource names (e.g., avoid 'broken-pod', 'test-project-that-does-not-exist', 'crashloop-app'). Use neutral names that don't give away what the LLM should discover
Each test must use a dedicated namespace app- to prevent conflicts
All pod names must be unique across tests
Resource naming should be neutral, not hint at the problem
Use minimal resource footprints (e.g., reduce memory/CPU for Loki in tests)
Files:
tests/llm/fixtures/test_ask_holmes/29_events_from_alert_manager/test_case.yamltests/llm/fixtures/test_ask_holmes/73a_time_window_anomaly/test_case.yamltests/llm/fixtures/test_ask_holmes/77_liveness_probe_misconfiguration/test_case.yamltests/llm/fixtures/test_ask_holmes/100_historical_logs/test_case.yamltests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/test_case.yamltests/llm/fixtures/test_ask_holmes/52_logs_login_issues/test_case.yaml
⏰ 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). (5)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (6)
tests/llm/fixtures/test_ask_holmes/100_historical_logs/test_case.yaml (1)
13-13: Retag to medium is appropriate and aligned with PR objective.Change looks good and matches the test’s complexity.
tests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/test_case.yaml (1)
5-5: Retag to medium is appropriate and aligned with PR objective.No other changes required here.
tests/llm/fixtures/test_ask_holmes/73a_time_window_anomaly/test_case.yaml (1)
6-6: Retag to medium is appropriate and aligned with PR objective.Change looks good.
tests/llm/fixtures/test_ask_holmes/52_logs_login_issues/test_case.yaml (1)
4-4: Retag to medium is appropriate and aligned with PR objective.Change looks good.
tests/llm/fixtures/test_ask_holmes/29_events_from_alert_manager/test_case.yaml (1)
7-8: Skip metadata addition looks good.Skipping due to missing mock data is reasonable; this prevents flaky or failing runs while data is unavailable.
tests/llm/fixtures/test_ask_holmes/77_liveness_probe_misconfiguration/test_case.yaml (1)
9-11: Difficulty tag reclassification to medium — LGTMTag change is consistent with the PR objective of moving some evals out of “easy.” No functional impact on the test harness.
2 participants
I deleted eval 88 as its broken and I didn't see a path to fix it (it would require tainting nodes in your cluster which we don't want to do from evals)