add missing namespace cleanup to evals - #752
Conversation
WalkthroughThe cleanup procedures in multiple test case YAML files were updated to include commands that delete the corresponding Kubernetes namespaces after test execution. This change ensures that, in addition to removing specific resources like jobs, manifests, and secrets, the entire namespace and all its contained resources are also deleted during test teardown. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 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: 0
🔭 Outside diff range comments (3)
tests/llm/fixtures/test_ask_holmes/71_connection_pool_starvation/test_case.yaml (1)
8-13: Rename the namespace to follow theapp-<id>convention.All LLM tests must operate in a dedicated namespace named
app-<testid>(see long-term learnings).
Usingnamespace-71risks clashes with parallel test runs that might pick the same numeric suffix but a different prefix.Diff sketch:
- kubectl create namespace namespace-71 || true + kubectl create namespace app-71 || true ... - -n namespace-71 --dry-run=client -o yaml | kubectl apply -f - + -n app-71 --dry-run=client -o yaml | kubectl apply -f - ... - kubectl delete secret backend-service-logs-script -n namespace-71 --ignore-not-found + kubectl delete secret backend-service-logs-script -n app-71 --ignore-not-found - kubectl delete namespace namespace-71 --ignore-not-found + kubectl delete namespace app-71 --ignore-not-foundAlso applies to: 16-18
tests/llm/fixtures/test_ask_holmes/69_rate_limit_exhaustion/test_case.yaml (1)
10-13: Rename resources to neutral identifiers to avoid leaking test intentNames like
api-limiter-logs-script(and anyapi-limiterpods in the manifest) reveal the rate-limit scenario to the LLM, violating the fixture guideline that resource names must not hint at the underlying issue.
Switch to neutral, unique names (e.g.,giant-narwhal-script) and update references throughout the fixture.tests/llm/fixtures/test_ask_holmes/68_cascading_failures/test_case.yaml (1)
9-11: Enforceapp-<id>namespace naming in LLM fixturesScript output reveals multiple tests still creating namespaces outside the
app-<testid>convention—e.g.,namespace-66,ask-holmes-namespace-51,production-62,staging-63,ts-46,rabbitmq,28-test, etc. This breaks our guideline and risks collisions when suites run in parallel.• Rename in tests/llm/fixtures/test_ask_holmes/68_cascading_failures/test_case.yaml (lines 9–11):
- kubectl create namespace namespace-68 || true + kubectl create namespace app-68 || true(and update any matching
kubectl delete namespace namespace-68calls)• Apply the same refactor to all non-
app-<id>entries found by:rg --no-heading -N 'create namespace ' tests/llm/fixtures | grep -v 'app-'—e.g. tests/…/66_http_error_needle, …/67_performance_degradation, …/69_rate_limit_exhaustion, …/70_memory_leak_detection, …/71_connection_pool_starvation, …/73_time_window_anomaly, …/74_config_change_impact, …/75_network_flapping, tests with
ask-holmes-namespace-*,production-*,staging-*,ts-*,rabbitmq, and28-test.Ensuring every fixture uses
app-<testid>will prevent namespace collisions across parallel runs.
🧹 Nitpick comments (6)
tests/llm/fixtures/test_ask_holmes/74_config_change_impact/test_case.yaml (1)
11-13: Consider switching to the canonical per-test namespace patternInternal conventions (see previous PR feedback) recommend
app-<id>namespaces (e.g.,app-74) to avoid clashes with external environments that may use anamespace-*naming scheme. Future migrations or parallel test runs will be safer if this test follows the same pattern.No action required for this PR, but worth aligning in the next refactor pass.
Also applies to: 19-21
tests/llm/fixtures/test_ask_holmes/75_network_flapping/test_case.yaml (1)
16-19: Namespace-level delete renders previous resource deletes redundant—simplify & speed up teardownSince deleting the namespace (
kubectl delete namespace namespace-75) already garbage-collects every namespaced resource, the preceding two commands are no longer needed and only slow the pipeline (they also riskNotFoundnoise for the manifest). A more concise, resilient block would be:-after_test: | - kubectl delete -f ./manifest.yaml - kubectl delete secret frontend-logs-script -n namespace-75 --ignore-not-found - kubectl delete namespace namespace-75 --ignore-not-found +after_test: | + # One command is enough; --ignore-not-found avoids noise, --wait=false speeds CI + kubectl delete namespace namespace-75 --ignore-not-found --wait=falseThis keeps teardown idempotent, avoids duplicate work, and shortens CI wall-clock time.
tests/llm/fixtures/test_ask_holmes/71_connection_pool_starvation/test_case.yaml (1)
15-18: Redundant deletions—namespace removal already covers child resources.Deleting
manifest.yamland the secret right before deleting the whole namespace is unnecessary; namespace deletion tears down everything inside it.
Keeping only the namespace deletion simplifies teardown and reduces runtime.- kubectl delete -f ./manifest.yaml - kubectl delete secret backend-service-logs-script -n <namespace> --ignore-not-found - kubectl delete namespace <namespace> --ignore-not-found + kubectl delete namespace <namespace> --ignore-not-found(Replace
<namespace>with the updated name once the rename is applied.)tests/llm/fixtures/test_ask_holmes/20_long_log_file_search/test_case.yaml (1)
12-15: You can drop the per-resource deletes for a leaner teardown.Once the namespace deletion is in place, the preceding
kubectl delete -f ./manifest.yamlandkubectl delete secret …commands are redundant—the namespace garbage-collection will wipe everything inside it. Removing them will shorten test duration and avoid double work:after_test: | - kubectl delete -f ./manifest.yaml - kubectl delete secret network-connector-logs-script -n app-20 --ignore-not-found kubectl delete namespace app-20 --ignore-not-foundtests/llm/fixtures/test_ask_holmes/69_rate_limit_exhaustion/test_case.yaml (1)
16-18: Namespace deletion added – streamline teardown & wait for completionDeleting the namespace is the right call, but once you do that, the explicit secret deletion is unnecessary. Removing it cuts one API call and reduces teardown time.
Also add--waitso the command blocks until the namespace is fully removed, preventing resource collisions in subsequent tests.kubectl delete -f ./manifest.yaml - kubectl delete secret api-limiter-logs-script -n namespace-69 --ignore-not-found - kubectl delete namespace namespace-69 --ignore-not-found + kubectl delete namespace namespace-69 --wait --ignore-not-foundtests/llm/fixtures/test_ask_holmes/67_performance_degradation/test_case.yaml (1)
18-18: Good addition, but consider non-blocking deletionAdding a namespace-wide delete guarantees full cleanup.
However,kubectl delete namespaceis synchronous and can keep the test runner waiting ~45-60 s until the namespace reachesTerminating. Using--wait=false(followed by a short sleep if needed) can shave seconds off every test without sacrificing correctness.-kubectl delete namespace namespace-67 --ignore-not-found +kubectl delete namespace namespace-67 --ignore-not-found --wait=false +# Optionally give the API server a moment to mark it terminating +sleep 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tests/llm/fixtures/test_ask_holmes/12_job_crashing/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/20_long_log_file_search/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/66_http_error_needle/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/67_performance_degradation/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/68_cascading_failures/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/69_rate_limit_exhaustion/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/71_connection_pool_starvation/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/74_config_change_impact/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/75_network_flapping/test_case.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/llm/fixtures/**/*
📄 CodeRabbit Inference Engine (CLAUDE.md)
tests/llm/fixtures/**/*: Mock data: tests/llm/fixtures/{test_name}/
All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Never use resource names that hint at the problem or expected behavior (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
Files:
tests/llm/fixtures/test_ask_holmes/71_connection_pool_starvation/test_case.yamltests/llm/fixtures/test_ask_holmes/66_http_error_needle/test_case.yamltests/llm/fixtures/test_ask_holmes/75_network_flapping/test_case.yamltests/llm/fixtures/test_ask_holmes/68_cascading_failures/test_case.yamltests/llm/fixtures/test_ask_holmes/69_rate_limit_exhaustion/test_case.yamltests/llm/fixtures/test_ask_holmes/20_long_log_file_search/test_case.yamltests/llm/fixtures/test_ask_holmes/67_performance_degradation/test_case.yamltests/llm/fixtures/test_ask_holmes/12_job_crashing/test_case.yamltests/llm/fixtures/test_ask_holmes/74_config_change_impact/test_case.yaml
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (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
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/**/*.py : Each LLM test must use a dedicated namespace app-<testid> (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
tests/llm/fixtures/test_ask_holmes/71_connection_pool_starvation/test_case.yaml (2)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (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
tests/llm/fixtures/test_ask_holmes/66_http_error_needle/test_case.yaml (3)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (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
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/fixtures/test_ask_holmes/75_network_flapping/test_case.yaml (2)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (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
tests/llm/fixtures/test_ask_holmes/68_cascading_failures/test_case.yaml (2)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (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
tests/llm/fixtures/test_ask_holmes/69_rate_limit_exhaustion/test_case.yaml (5)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (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
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
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: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/**/*.py : Each LLM test must use a dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Mock data: tests/llm/fixtures/{test_name}/
tests/llm/fixtures/test_ask_holmes/20_long_log_file_search/test_case.yaml (4)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (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
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/**/*.py : Each LLM test must use a dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
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/fixtures/test_ask_holmes/67_performance_degradation/test_case.yaml (2)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (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
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
tests/llm/fixtures/test_ask_holmes/12_job_crashing/test_case.yaml (4)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (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
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/**/*.py : Each LLM test must use a dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: LLM evaluation tests run automatically in CI
⏰ 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 (7)
tests/llm/fixtures/test_ask_holmes/12_job_crashing/test_case.yaml (1)
15-15: Namespace deletion ensures full cleanup – nice catchAdding
kubectl delete namespace app-12 --ignore-not-foundguarantees the test environment is completely torn down, preventing cross-test interference and dangling resources in CI.tests/llm/fixtures/test_ask_holmes/66_http_error_needle/test_case.yaml (1)
18-18: Complete teardown handled correctlyIncluding
kubectl delete namespace namespace-66 --ignore-not-foundfinalizes resource cleanup and avoids leftovers that could pollute subsequent runs.tests/llm/fixtures/test_ask_holmes/74_config_change_impact/test_case.yaml (1)
19-21: Namespace deletion step added – good catchAdding an explicit
kubectl delete namespace namespace-74 --ignore-not-foundensures cluster hygiene and prevents flake-inducing residue between tests. Ordering is correct (manifests → secret → namespace).tests/llm/fixtures/test_ask_holmes/20_long_log_file_search/test_case.yaml (1)
15-15: Namespace deletion added – looks good.Adding
kubectl delete namespace app-20 --ignore-not-foundguarantees a full teardown of all resources created during the test.tests/llm/fixtures/test_ask_holmes/69_rate_limit_exhaustion/test_case.yaml (1)
8-9: Namespace name deviates fromapp-<id>conventionFixtures are expected to use the
app-<testid>pattern (e.g.,app-69) for predictable isolation.namespace-69works functionally but breaks that convention and may trip tooling that relies on it.
Please rename the namespace and all references accordingly, or confirm that the deviation is intentional.tests/llm/fixtures/test_ask_holmes/67_performance_degradation/test_case.yaml (1)
10-13: Confirmed resource-name uniqueness—no collisions foundVerified that both
api-gateway-logs-scriptandapi-gatewayonly appear undertests/llm/fixtures/test_ask_holmes/67_performance_degradationwith no occurrences in other fixtures. No renaming required.tests/llm/fixtures/test_ask_holmes/68_cascading_failures/test_case.yaml (1)
17-19: Good call — full namespace cleanup avoids resource leakageAdding an explicit
kubectl delete namespace namespace-68 --ignore-not-foundguarantees all child resources are wiped, preventing cross-test interference and freeing cluster quota.
2 participants
No description provided.