add skip tag for broken evals - #714
Conversation
|
""" WalkthroughThe changes introduce a new pytest marker "skip" for broken or non-reproducible tests, update the GitHub Actions workflow to exclude tests marked with "skip" during LLM evaluation runs, add the "skip" tag with explanatory comments to specific test cases, and update Kubernetes job metadata and related test prompts in a fixture. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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 (1)
pyproject.toml (1)
88-111: Remove duplicated markers to avoid confusion and future merge-noise
"missing-tool"and"kafka"appear twice (lines 98 & 109, 97 & 110). Keeping duplicates clutters the config and can cause hard-to-spot merge conflicts later.- "missing-tool: Verify holmes knows how to communicate this to the user", - "kafka: Tests involving Kafka functionality", "skip: Broken or non-reproducible tests"(A single definition of each existing marker already appears a few lines above.)
🧹 Nitpick comments (1)
.github/workflows/llm-evaluation.yaml (1)
48-48: Quote the marker expression to prevent shell-level globbingIn bash, single quotes already protect the expression, but double-quoting avoids accidental glob expansion by the shell before pytest receives it (e.g., if someone replaces
skipwith a pattern containing*).- poetry run pytest --no-cov tests/llm/test_ask_holmes.py tests/llm/test_investigate.py -m 'not skip' -n 6 + poetry run pytest --no-cov tests/llm/test_ask_holmes.py tests/llm/test_investigate.py -m "not skip" -n 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/llm-evaluation.yaml(1 hunks)pyproject.toml(1 hunks)tests/llm/fixtures/test_ask_holmes/47_truncated_logs_context_window/test_case.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (1)
tests/llm/fixtures/test_ask_holmes/47_truncated_logs_context_window/test_case.yaml (1)
5-5: LGTM – clear skip rationaleThe added
skiptag with an inline comment documents the reason succinctly and keeps the flaky eval out of CI.
… on the cluster with the same name
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/llm/fixtures/test_ask_holmes/46_job_crashing_no_longer_exists/test_case.yaml (1)
12-16: Replace arbitrary sleeps with deterministic waitsThe
sleep 60/sleep 20blocks make the fixture slow and still nondeterministic on busy clusters. Consider waiting for the resource to reach the desired state instead:- sleep 60 + kubectl wait --for=condition=complete --timeout=90s job/typescript-transpiler -n ts-46 || true ... - sleep 20 + kubectl wait --for=delete job/typescript-transpiler -n ts-46 --timeout=60s || trueThis shortens runtime and avoids brittle timing assumptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/llm/fixtures/test_ask_holmes/46_job_crashing_no_longer_exists/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/46_job_crashing_no_longer_exists/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/63_fetch_error_logs_no_errors/test_case.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/llm/fixtures/test_ask_holmes/63_fetch_error_logs_no_errors/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/46_job_crashing_no_longer_exists/manifest.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). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (2)
tests/llm/fixtures/test_ask_holmes/46_job_crashing_no_longer_exists/test_case.yaml (2)
5-8: 👍 Good use of the newskiptagAdding the explanatory comments followed by the
skipmarker cleanly disables the flaky eval while documenting the rationale. No action needed.
1-1: No action needed – prompt and manifest identifiers are consistentVerified that
tests/llm/fixtures/test_ask_holmes/46_job_crashing_no_longer_exists/manifest.yamlcontains:
name: typescript-transpilernamespace: ts-46The user prompt already matches these identifiers.
2 participants
No description provided.