fix(tests): stop a live-provider eval test from grading the mock - #502
Merged
Merged
Conversation
Five tests under `tests/eval/` build a real `LiteLLMClient` and say so -- "Real end-to-end smoke: live extractors + real LLM", "Smoke test against a real judge model". None of them can reach a provider. Per `.claude/rules/testing-conventions.md` a live test does that only if it lives under `tests/e2e_tests/` AND carries `requires_credentials`; these satisfy neither, so `_is_e2e_test_run` installs the session-wide `litellm.completion` patch and they talk to canned text. Measured with `RUN_LOW_PRIORITY=1`, which is the only way these ever run: before: 4 failed, 44 passed after: 5 failed, 43 passed The single test that flipped is `test_live_extraction_provider_real`. It was PASSING -- reporting a green live-LLM smoke while grading mock output. That is the bug. The other four were already failing, but for a reason that named the wrong thing: `StructuredOutputParseError` / `StructuredOutputRepairError`, i.e. "the canned response does not fit the schema", when the actual problem is that a canned response is being used at all. Each now calls `assert_litellm_unpatched()` -- the backstop this repo already ships for exactly this, and which the e2e live tests already use. One false pass becomes an honest failure; four confusing failures name their real cause: AssertionError: litellm.completion is patched, so this live-provider test would assert against canned mock text. Live tests belong under tests/e2e_tests/ and must carry the requires_credentials marker; the e2e conftest lifts the session patch for those. Not moved under `tests/e2e_tests/`, which is the other half of the documented recipe and would make them actually run: they depend on `tests/eval/`'s own fixtures and providers, so relocating them is a larger change than making them honest. They remain `@skip_low_priority`, so the default suite is unaffected -- `tests/eval` is 93 passed, 5 skipped, and the full OSS suite 6220 passed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe evaluation smoke tests now verify that ChangesEvaluation guards
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The tests now fail explicitly instead of silently grading mocked responses. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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
1 participant
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.
The claim that was false
Five tests under
tests/eval/build a realLiteLLMClientand say so in their docstrings — "Real end-to-end smoke: live extractors + real LLM", "Smoke test against a real judge model".None of them can reach a provider. Per
.claude/rules/testing-conventions.md, a live test does that only if it lives undertests/e2e_tests/and carriesrequires_credentials. These satisfy neither, so_is_e2e_test_runinstalls the session-widelitellm.completionpatch and they grade canned text.Measured, not argued
RUN_LOW_PRIORITY=1is the only way these ever run. With it:One test flipped:
test_live_extraction_provider_real. It was PASSING — reporting a green live-LLM smoke while grading mock output. That is the bug.The other four were already failing, but for a reason that named the wrong thing —
StructuredOutputParseError/StructuredOutputRepairError, i.e. "the canned response does not fit the schema", when the actual problem is that a canned response is in play at all.The fix
Each of the five now calls
assert_litellm_unpatched()— the backstop this repo already ships for exactly this case, and which the e2e live tests already use. One false pass becomes an honest failure; four confusing failures name their real cause:What this deliberately does not do
It does not move them under
tests/e2e_tests/. That is the other half of the documented recipe and would make them actually run — but they depend ontests/eval/'s own fixtures and provider factories, so relocating them is a larger change than making them honest. Worth doing separately if you want these live smokes to earn their keep.They remain
@skip_low_priority, so the default suite is unchanged.Test Plan
Local, as Actions has no budget:
ruff check+ruff format --checkontests/eval/pytest tests/eval(normal run)The before/after comparison above was produced by reverting the three files, re-running, and restoring them from byte-exact copies verified by checksum — not by trusting
git checkout --to bring the work back.Summary by CodeRabbit