Uh oh!
There was an error while loading. Please reload this page.
Codecov: Upgrade the Codecov Version & Test Cases Updates - #924
Conversation
📝 WalkthroughWalkthroughThis PR refactors the single-trace merge logic in the evaluation CRUD module to conditionally preserve the ChangesEvaluation trace merging and CI tooling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
OpenAPI changes ⚪ No API surface changesNote This PR does not modify the API contract.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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)
backend/app/tests/crud/evaluations/test_merge.py (2)
22-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd return type annotations to all test methods.
All test methods in the class are missing the
-> Nonereturn type annotation. As per coding guidelines, all function return values must have type hints.🔧 Proposed fix
Add
-> Noneto each test method signature:- def test_empty_cache_returns_fresh(self):+ def test_empty_cache_returns_fresh(self) -> None: fresh = [_trace("0"), _trace("1")] - def test_grows_monotonically(self):+ def test_grows_monotonically(self) -> None: """15 -> 24 -> 30 across successive syncs.""" - def test_fewer_fresh_traces_keeps_cached(self):+ def test_fewer_fresh_traces_keeps_cached(self) -> None: """A partial fetch (27) must not drop cached traces (29).""" - def test_additional_score_unions(self):+ def test_additional_score_unions(self) -> None: """A trace gaining a new score is enriched, not replaced wholesale.""" - def test_fresh_value_wins_on_conflict(self):+ def test_fresh_value_wins_on_conflict(self) -> None: existing = [_trace("1", value=0.5)] - def test_identical_trace_is_reused(self):+ def test_identical_trace_is_reused(self) -> None: existing = [_trace("1", value=0.5)] - def test_category_and_external_id_preserved_across_resync(self):+ def test_category_and_external_id_preserved_across_resync(self) -> None: def _real(trace_id, value=0.5, category="Health"):Apply the same pattern to test methods in
TestComputeSummaryScores(lines 112-136) andTestMergeScoresStepForward(lines 139-170).Also applies to: 28-42, 44-51, 53-69, 71-75, 77-82
🤖 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 `@backend/app/tests/crud/evaluations/test_merge.py` around lines 22 - 26, Add return type annotations `-> None` to every test method in this file; specifically update the signature of test_empty_cache_returns_fresh (and all other test_* functions in the same Test* classes such as TestComputeSummaryScores and TestMergeScoresStepForward) to include `-> None` after the parameter list so each test function has an explicit None return type per the coding guidelines.Source: Coding guidelines
10-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd type hints to the
_tracehelper function.The function parameters and return value lack type annotations. As per coding guidelines, all function parameters and return values in Python code must have type hints.
🔧 Proposed fix
-def _trace(trace_id, value=1.0, name="accuracy", data_type="NUMERIC"):+def _trace(+ trace_id: str,+ value: float | str | None = 1.0,+ name: str = "accuracy",+ data_type: str = "NUMERIC"+) -> dict[str, Any]: return { "trace_id": trace_id,Note: You'll need to add
from typing import Anyat the top of the file if not already present.🤖 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 `@backend/app/tests/crud/evaluations/test_merge.py` around lines 10 - 18, Add explicit type hints to the _trace helper: annotate parameters as trace_id: int | str, value: float, name: str, data_type: str and give the function a return type of Dict[str, Any]; update imports to include from typing import Any, Dict if not already present. Locate the _trace function and change its signature and return annotation accordingly, ensuring the body stays the same and the typing imports are added at the top of the test file.Source: Coding guidelines
🤖 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 @.github/workflows/continuous_integration.yml:
- Line 75: Replace the floating GitHub Action reference uses:
codecov/codecov-action@v7 with a pinned commit SHA for supply-chain security and
to match the v7 release (e.g., uses:
codecov/codecov-action@a99c28d3f0da835de33ff2feb2e15691c7b9641f) and then run CI
to validate that uploads still succeed after the Keybase/account change; update
the workflow line containing "uses: codecov/codecov-action@v7" to the chosen
commit SHA and verify Codecov uploads in the CI logs.
In `@backend/app/crud/evaluations/merge.py`:
- Line 12: The merged result is typed as dict[str, Any] and may omit the
required TraceData.category field, violating the TraceData return contract; fix
by making TraceData.category optional (change TraceData in
backend/app/crud/evaluations/score.py to NotRequired[str]) or alternatively
ensure the merge function always sets a category (populate merged['category']
with a default when both inputs lack it) and update the merged variable typing
to TraceData; reference TraceData, the merged variable, and the merge
function/return to locate the changes.
In `@backend/app/tests/crud/evaluations/test_merge.py`:
- Around line 84-110: The local helper _real in the test lacks type hints and
the suite misses the edge-case where both existing and fresh traces omit the
"category" key: add parameter and return type annotations to the nested function
_real (e.g., annotate trace_id: str, value: float = 0.5, category: str =
"Health" and its return type as Dict[str, Any]) and add a new assertion using
merge_trace_data that provides legacy-shape traces for both existing and fresh
(no "category") and asserts that the merged trace does not contain the
"category" key, thereby exercising the behavior implemented in
_merge_single_trace.
---
Outside diff comments:
In `@backend/app/tests/crud/evaluations/test_merge.py`:
- Around line 22-26: Add return type annotations `-> None` to every test method
in this file; specifically update the signature of
test_empty_cache_returns_fresh (and all other test_* functions in the same Test*
classes such as TestComputeSummaryScores and TestMergeScoresStepForward) to
include `-> None` after the parameter list so each test function has an explicit
None return type per the coding guidelines.
- Around line 10-18: Add explicit type hints to the _trace helper: annotate
parameters as trace_id: int | str, value: float, name: str, data_type: str and
give the function a return type of Dict[str, Any]; update imports to include
from typing import Any, Dict if not already present. Locate the _trace function
and change its signature and return annotation accordingly, ensuring the body
stays the same and the typing imports are added at the top of the test file.
🪄 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: CHILL
Plan: Pro
Run ID: 930dee2f-1392-441d-9310-ec8fc101c48b
📒 Files selected for processing (3)
.github/workflows/continuous_integration.ymlbackend/app/crud/evaluations/merge.pybackend/app/tests/crud/evaluations/test_merge.py
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
🎉 This PR is included in version 1.2.0-main.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
codecov/codecov-actionfromv6tov7which bumps the wrapper submodule to fetch the PGP key from the newcodecovsecopsKeybase account.References:
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.