Repository navigation
Feat/langsmith support - #2
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds comprehensive test coverage for the EvalForge evaluation pipeline, including CLI integration tests, configuration validation, failure clustering logic, case generation, and evaluation runner execution. Configuration changes enable CodeRabbit review with per-path instructions and adjust coverage thresholds. ChangesTest Suite for EvalForge Pipeline
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_generator.py (1)
1-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply Ruff formatting for this test module
CI indicates this file would be reformatted. Run
ruff format tests/test_generator.py(orruff format .) to pass the formatting check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_generator.py` around lines 1 - 117, The test module (containing make_cluster, TestEvalGenerator and all test_* functions that reference EvalGenerator) needs Ruff formatting fixes; run `ruff format` on this module (or your repo) and commit the reformatted file so import/order/spacing/line-length and other Ruff rules are applied to tests/test_generator.py, ensuring the file's whitespace, imports, and docstring conform to the project's Ruff configuration.tests/test_runner.py (1)
1-120:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix Ruff formatting to unblock CI
CI reports this file needs formatting. Please run
ruff format tests/test_runner.py(orruff format .) and commit the result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_runner.py` around lines 1 - 120, CI failed due to ruff formatting issues in the tests; run an automatic formatter and commit the changes: run `ruff format tests/test_runner.py` (or `ruff format .`) to fix formatting across the file, verify tests/TestEvalRunner and its methods (test_score_to_level_*, test_run_*, test_log_summary_empty) remain unchanged semantically, then add and commit the formatted file so CI will pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyproject.toml`:
- Around line 131-139: The coverage configuration weakens checks by excluding
runtime code and lowering the threshold: remove the production/runtime modules
from the omit list (specifically the entries "evalforge/agent/*",
"evalforge/ui/*", and "evalforge/exporters/html_reporter.py" in the omit array)
so they are measured by coverage, and restore the coverage threshold controlled
by the fail_under setting to its prior stricter value (or set it to a sensible
minimum such as >=80) so the CI will fail on regressions.
In `@tests/test_cli.py`:
- Around line 13-17: In test_version, avoid asserting the hard-coded "0.1.0";
instead obtain the package/CLI version dynamically and compare it to the CLI
output: call runner.invoke(app, ["--version"]) as before, then get the
authoritative version via package metadata (e.g.,
importlib.metadata.version("evalforge")) or the app/module version attribute,
and assert that the retrieved version string appears in result.output; update
the test_version function to use that dynamic lookup rather than the literal
"0.1.0".
In `@tests/test_runner.py`:
- Around line 63-69: The async test mock mock_score is flaky because it uses a
shared call_count to pick mock_judgment_pass or mock_judgment_fail; change
mock_score to be deterministic by deriving the intended judgment from the inputs
rather than call order (for example inspect case or output fields such as
case["id"] or output["text"] to decide whether to return mock_judgment_pass or
mock_judgment_fail), or accept an explicit mapping/dict keyed by case id passed
into the test and look up the expected judgment there; update mock_score to
compute score from the selected judgment and avoid any shared mutable call_count
so run_with_outputs can schedule concurrently without flakiness.
---
Outside diff comments:
In `@tests/test_generator.py`:
- Around line 1-117: The test module (containing make_cluster, TestEvalGenerator
and all test_* functions that reference EvalGenerator) needs Ruff formatting
fixes; run `ruff format` on this module (or your repo) and commit the
reformatted file so import/order/spacing/line-length and other Ruff rules are
applied to tests/test_generator.py, ensuring the file's whitespace, imports, and
docstring conform to the project's Ruff configuration.
In `@tests/test_runner.py`:
- Around line 1-120: CI failed due to ruff formatting issues in the tests; run
an automatic formatter and commit the changes: run `ruff format
tests/test_runner.py` (or `ruff format .`) to fix formatting across the file,
verify tests/TestEvalRunner and its methods (test_score_to_level_*, test_run_*,
test_log_summary_empty) remain unchanged semantically, then add and commit the
formatted file so CI will pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2914f5ef-a3b4-4cd2-94d1-6cbf3ddc5507
📒 Files selected for processing (7)
.coderabbit.yamlpyproject.tomltests/test_cli.pytests/test_clusterer.pytests/test_config.pytests/test_generator.pytests/test_runner.py
Comment on lines
+131
to
+139
| omit = [ | ||
| "tests/*", | ||
| "scripts/*", | ||
| "examples/*", | ||
| # Integration-layer modules — tested via integration tests, not unit tests | ||
| "evalforge/agent/*", | ||
| "evalforge/ui/*", | ||
| "evalforge/exporters/html_reporter.py", | ||
| ] |
There was a problem hiding this comment.
Coverage gate is weakened for production code paths
Line [136]-Line [138] exclude runtime modules from coverage, and Line [144] lowers fail_under simultaneously. This materially reduces regression detection in non-test code.
Suggested adjustment
[tool.coverage.run]
source = ["evalforge"]
omit = [
"tests/*",
"scripts/*",
"examples/*",
- # Integration-layer modules — tested via integration tests, not unit tests
- "evalforge/agent/*",
- "evalforge/ui/*",
- "evalforge/exporters/html_reporter.py",
]
[tool.coverage.report]
show_missing = true
skip_covered = false
-fail_under = 55
+fail_under = 60Also applies to: 144-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` around lines 131 - 139, The coverage configuration weakens
checks by excluding runtime code and lowering the threshold: remove the
production/runtime modules from the omit list (specifically the entries
"evalforge/agent/*", "evalforge/ui/*", and
"evalforge/exporters/html_reporter.py" in the omit array) so they are measured
by coverage, and restore the coverage threshold controlled by the fail_under
setting to its prior stricter value (or set it to a sensible minimum such as
>=80) so the CI will fail on regressions.
Comment on lines
+13
to
+17
| def test_version(self): | ||
| result = runner.invoke(app, ["--version"]) | ||
| assert result.exit_code == 0 | ||
| assert "evalforge" in result.output | ||
| assert "0.1.0" in result.output |
There was a problem hiding this comment.
Avoid hard-coding the release version in CLI tests
Line [17] ties the test to a specific package version and will fail on routine version bumps without behavioral regression.
Suggested change
+import re
@@
def test_version(self):
result = runner.invoke(app, ["--version"])
assert result.exit_code == 0
assert "evalforge" in result.output
- assert "0.1.0" in result.output
+ assert re.search(r"\b\d+\.\d+\.\d+\b", result.output)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_version(self): | |
| result = runner.invoke(app, ["--version"]) | |
| assert result.exit_code == 0 | |
| assert "evalforge" in result.output | |
| assert "0.1.0" in result.output | |
| def test_version(self): | |
| result = runner.invoke(app, ["--version"]) | |
| assert result.exit_code == 0 | |
| assert "evalforge" in result.output | |
| assert re.search(r"\b\d+\.\d+\.\d+\b", result.output) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_cli.py` around lines 13 - 17, In test_version, avoid asserting the
hard-coded "0.1.0"; instead obtain the package/CLI version dynamically and
compare it to the CLI output: call runner.invoke(app, ["--version"]) as before,
then get the authoritative version via package metadata (e.g.,
importlib.metadata.version("evalforge")) or the app/module version attribute,
and assert that the retrieved version string appears in result.output; update
the test_version function to use that dynamic lookup rather than the literal
"0.1.0".
Comment on lines
+63
to
+69
| call_count = 0 | ||
|
|
||
| async def mock_score(case, output, semaphore): | ||
| nonlocal call_count | ||
| j = mock_judgment_pass if call_count == 0 else mock_judgment_fail | ||
| call_count += 1 | ||
| score = float(j["score"]) |
There was a problem hiding this comment.
Make async scoring mock deterministic instead of call-order based
Line [67]-Line [68] depends on invocation order via shared call_count. If run_with_outputs schedules concurrently, this can become flaky.
Suggested change
- call_count = 0
+ judgment_by_case_id = {
+ "c1": {"score": 9, "reasoning": "Mentions days correctly"},
+ "c2": {"score": 2, "reasoning": "No useful information"},
+ }
async def mock_score(case, output, semaphore):
- nonlocal call_count
- j = mock_judgment_pass if call_count == 0 else mock_judgment_fail
- call_count += 1
+ j = judgment_by_case_id[case.case_id]
score = float(j["score"])
return EvalResult(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| call_count = 0 | |
| async def mock_score(case, output, semaphore): | |
| nonlocal call_count | |
| j = mock_judgment_pass if call_count == 0 else mock_judgment_fail | |
| call_count += 1 | |
| score = float(j["score"]) | |
| judgment_by_case_id = { | |
| "c1": {"score": 9, "reasoning": "Mentions days correctly"}, | |
| "c2": {"score": 2, "reasoning": "No useful information"}, | |
| } | |
| async def mock_score(case, output, semaphore): | |
| j = judgment_by_case_id[case.case_id] | |
| score = float(j["score"]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_runner.py` around lines 63 - 69, The async test mock mock_score is
flaky because it uses a shared call_count to pick mock_judgment_pass or
mock_judgment_fail; change mock_score to be deterministic by deriving the
intended judgment from the inputs rather than call order (for example inspect
case or output fields such as case["id"] or output["text"] to decide whether to
return mock_judgment_pass or mock_judgment_fail), or accept an explicit
mapping/dict keyed by case id passed into the test and look up the expected
judgment there; update mock_score to compute score from the selected judgment
and avoid any shared mutable call_count so run_with_outputs can schedule
concurrently without flakiness.
There was a problem hiding this comment.
Pull request overview
Adds repository configuration for CodeRabbit-driven PR reviews and expands the unit test suite for core EvalForge components (clusterer/generator/runner/config/CLI), alongside adjusting coverage settings to reflect the intended test scope.
Changes:
- Add
.coderabbit.yamlwith path-scoped review instructions and enabled static analysis tools. - Introduce new pytest coverage for
FailureClusterer,EvalGenerator,EvalRunner,Settings, and the Typer CLI. - Update coverage configuration (omit paths + lower
fail_under) inpyproject.toml.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_runner.py | Adds unit tests for runner scoring, async execution paths, and summary logging. |
| tests/test_generator.py | Adds unit tests for example formatting and raw-case validation/conversion. |
| tests/test_config.py | Adds unit tests for Settings defaults and env overrides. |
| tests/test_clusterer.py | Adds unit tests for TF-IDF embedding, clustering, cluster building, and labeling behavior. |
| tests/test_cli.py | Adds smoke tests for CLI help/version and missing API-key error behavior. |
| pyproject.toml | Adjusts coverage omit list and lowers minimum coverage threshold. |
| .coderabbit.yaml | Adds CodeRabbit configuration with project-specific review guidance and tooling toggles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+63
to
+78
| call_count = 0 | ||
|
|
||
| async def mock_score(case, output, semaphore): | ||
| nonlocal call_count | ||
| j = mock_judgment_pass if call_count == 0 else mock_judgment_fail | ||
| call_count += 1 | ||
| score = float(j["score"]) | ||
| return EvalResult( | ||
| case_id=case.case_id, | ||
| input=case.input, | ||
| expected_output=case.expected_output, | ||
| actual_output=output, | ||
| score=score, | ||
| level=runner._score_to_level(score), | ||
| judge_reasoning=j["reasoning"], | ||
| cluster_label=case.cluster_label, |
Comment on lines
+108
to
+121
| async def test_label_clusters_no_api_key(self): | ||
| """Without a Gemini key, labels stay as cluster_N placeholders.""" | ||
| clusterer = FailureClusterer() | ||
| clusters = [ | ||
| Cluster( | ||
| cluster_id=0, | ||
| label="cluster_0", | ||
| traces=make_traces(3), | ||
| failure_rate=1.0, | ||
| ) | ||
| ] | ||
| # No API key set — should return unchanged | ||
| result = await clusterer._label_clusters(clusters) | ||
| assert result[0].label == "cluster_0" |
| clusterer = FailureClusterer(num_clusters=2) | ||
| traces = make_traces(20, score=0.2) | ||
|
|
||
| with patch.object(clusterer, "_label_clusters", new=AsyncMock(side_effect=lambda c: c)): |
Comment on lines
+10
to
+18
| def test_defaults(self): | ||
| from evalforge.config import Settings | ||
|
|
||
| s = Settings() | ||
| assert s.phoenix_endpoint == "http://localhost:6006" | ||
| assert s.gemini_model == "gemini-2.0-flash" | ||
| assert s.evalforge_trace_limit == 500 | ||
| assert s.evalforge_failure_threshold == 0.5 | ||
| assert s.evalforge_cases_per_cluster == 10 |
Comment on lines
+13
to
+18
| def test_version(self): | ||
| result = runner.invoke(app, ["--version"]) | ||
| assert result.exit_code == 0 | ||
| assert "evalforge" in result.output | ||
| assert "0.1.0" in result.output | ||
|
|
Comment on lines
+135
to
+138
| # Integration-layer modules — tested via integration tests, not unit tests | ||
| "evalforge/agent/*", | ||
| "evalforge/ui/*", | ||
| "evalforge/exporters/html_reporter.py", |
Comment on lines
+1
to
+12
| # CodeRabbit configuration for EvalForge | ||
| # Docs: https://docs.coderabbit.ai/reference/configuration | ||
|
|
||
| language: "en-US" | ||
|
|
||
| reviews: | ||
| profile: "assertive" # thorough reviews — this is a public OSS project | ||
| request_changes_workflow: false | ||
| high_level_summary: true | ||
| high_level_summary_placeholder: "@coderabbitai summary" | ||
| poem: false # skip the poem, keep it professional | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
.coderabbit.yamlto enable CodeRabbit AI reviews on all PRs.Summary by CodeRabbit
Tests
Chores