Conversation
📝 WalkthroughWalkthroughThis PR introduces automated mocking infrastructure for ChromaDB and OpenAI embedding services across the test suite. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_skill_lab.py (1)
173-189:⚠️ Potential issue | 🟠 MajorUnit tests are asserting the fake helper, not the retrieval path.
At Line 185 / 213 / 262 / 298 / 314 / 326, tests call
_fake_query_drills()directly, so the patch at Line 147 is never actually exercised. This can hide regressions in the real retrieval wiring.Suggested fix
+from src.services.rag.chroma_db import chroma_manager ... - results = _fake_query_drills( + results = chroma_manager.query_drills( query_texts=[query_text], n_results=10, where={"category": "dribble"}, ) + mock_drills.assert_called()Apply the same pattern to the other retrieval tests currently invoking
_fake_query_drillsdirectly.Also applies to: 203-215, 253-264, 288-302, 310-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_skill_lab.py` around lines 173 - 189, Tests currently call the helper _fake_query_drills directly (e.g., in test_tc01_normal_drill_retrieval and the other listed tests), so the real retrieval patch is never exercised; update those tests to call the actual retrieval function introduced in the patch (the real query/retrieval function that replaces _fake_query_drills) with the same parameters (query_texts, n_results, where) while keeping the mock_drills fixture or mocks for external dependencies intact, and then assert on the returned results as before so the true retrieval wiring is exercised instead of the fake helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_gear_advisor.py`:
- Around line 124-135: The mock _fake_query_shoes currently ignores the
n_results parameter and returns all matches from
_FAKE_SHOE_DOCS/_FAKE_SHOE_METAS; update _fake_query_shoes to respect n_results
by applying the filter logic first and then truncating the matched docs and
metas to at most n_results before returning, keeping the same return shape
{"documents": [docs], "metadatas": [metas]}; ensure edge cases (n_results is
None or <=0) are handled sensibly (e.g., treat None as no limit, <=0 returns
empty lists).
- Line 127: The two zip usages silently truncate if the paired lists differ in
length; update the for-loop and the paired creation to use zip(..., strict=True)
so mismatched lengths raise an error: change the for d, m in
zip(_FAKE_SHOE_DOCS, _FAKE_SHOE_METAS) loop to use strict=True and change the
paired = list(zip(_FAKE_PLAYER_DOCS, _FAKE_PLAYER_METAS)) call to use
strict=True as well so the linter B905 is satisfied and length mismatches are
caught.
In `@tests/test_skill_lab.py`:
- Line 130: The zip call iterating over _FAKE_DRILL_DOCS and _FAKE_DRILL_METAS
should use strict=True to satisfy B905 and catch length mismatches; update the
loop that currently does for d, m in zip(_FAKE_DRILL_DOCS, _FAKE_DRILL_METAS):
to call zip with strict=True (i.e., zip(_FAKE_DRILL_DOCS, _FAKE_DRILL_METAS,
strict=True)) so the test fails if the fixtures differ in length.
In `@tests/test_whistle.py`:
- Line 103: The two zip usages iterating test fixtures (e.g., the loop using for
d, m in zip(_FAKE_RULE_DOCS, _FAKE_RULE_METAS) and the other zip at the later
test) should be made strict to avoid silent truncation; update both zip(...)
calls to pass strict=True so the test will raise if the sequences differ in
length, leaving the rest of the loop logic unchanged.
---
Outside diff comments:
In `@tests/test_skill_lab.py`:
- Around line 173-189: Tests currently call the helper _fake_query_drills
directly (e.g., in test_tc01_normal_drill_retrieval and the other listed tests),
so the real retrieval patch is never exercised; update those tests to call the
actual retrieval function introduced in the patch (the real query/retrieval
function that replaces _fake_query_drills) with the same parameters
(query_texts, n_results, where) while keeping the mock_drills fixture or mocks
for external dependencies intact, and then assert on the returned results as
before so the true retrieval wiring is exercised instead of the fake helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07e0b407-e398-41b0-9a99-00cc768aed68
📒 Files selected for processing (4)
tests/conftest.pytests/test_gear_advisor.pytests/test_skill_lab.pytests/test_whistle.py
| def _fake_query_shoes(query_texts, n_results=10, where=None): | ||
| """Return filtered shoe results based on where clause.""" | ||
| docs, metas = [], [] | ||
| for d, m in zip(_FAKE_SHOE_DOCS, _FAKE_SHOE_METAS): | ||
| if where and "price_krw" in str(where): | ||
| # Extract budget limit from where clause | ||
| budget = where.get("price_krw", {}).get("$lte") | ||
| if budget is not None and m["price_krw"] > budget: | ||
| continue | ||
| docs.append(d) | ||
| metas.append(m) | ||
| return {"documents": [docs], "metadatas": [metas]} |
There was a problem hiding this comment.
Mock query_shoes should honor n_results to match real query behavior.
Right now the fake returns all matches regardless of requested limit, which can hide pagination/limit regressions.
Suggested fix
def _fake_query_shoes(query_texts, n_results=10, where=None):
"""Return filtered shoe results based on where clause."""
docs, metas = [], []
for d, m in zip(_FAKE_SHOE_DOCS, _FAKE_SHOE_METAS):
if where and "price_krw" in str(where):
# Extract budget limit from where clause
budget = where.get("price_krw", {}).get("$lte")
if budget is not None and m["price_krw"] > budget:
continue
docs.append(d)
metas.append(m)
+ docs = docs[:n_results]
+ metas = metas[:n_results]
return {"documents": [docs], "metadatas": [metas]}🧰 Tools
🪛 Ruff (0.15.6)
[warning] 127-127: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_gear_advisor.py` around lines 124 - 135, The mock
_fake_query_shoes currently ignores the n_results parameter and returns all
matches from _FAKE_SHOE_DOCS/_FAKE_SHOE_METAS; update _fake_query_shoes to
respect n_results by applying the filter logic first and then truncating the
matched docs and metas to at most n_results before returning, keeping the same
return shape {"documents": [docs], "metadatas": [metas]}; ensure edge cases
(n_results is None or <=0) are handled sensibly (e.g., treat None as no limit,
<=0 returns empty lists).
| def _fake_query_shoes(query_texts, n_results=10, where=None): | ||
| """Return filtered shoe results based on where clause.""" | ||
| docs, metas = [], [] | ||
| for d, m in zip(_FAKE_SHOE_DOCS, _FAKE_SHOE_METAS): |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm target Python/lint config for zip(strict=...)
fd -H -a 'pyproject.toml|setup.cfg|ruff.toml|.ruff.toml' .
rg -n --no-heading 'requires-python|target-version|B905|select|ignore' \
pyproject.toml setup.cfg ruff.toml .ruff.toml 2>/dev/null || trueRepository: zweadfx/assist
Length of output: 453
🏁 Script executed:
# Check the full ruff.toml configuration
head -30 /home/jailuser/git/ruff.tomlRepository: zweadfx/assist
Length of output: 1079
🏁 Script executed:
# View the zip() calls at the specified lines in the test file
sed -n '125,145p' /home/jailuser/git/tests/test_gear_advisor.py | cat -nRepository: zweadfx/assist
Length of output: 1157
Add strict=True to both zip() calls to comply with B905 linting rule.
This is enforced by the project's ruff configuration (flake8-bugbear is selected) and prevents silent truncation if paired lists have mismatched lengths.
# Line 127:
for d, m in zip(_FAKE_SHOE_DOCS, _FAKE_SHOE_METAS, strict=True):
# Line 142:
paired = list(zip(_FAKE_PLAYER_DOCS, _FAKE_PLAYER_METAS, strict=True))Also applies to: 142-142
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 127-127: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_gear_advisor.py` at line 127, The two zip usages silently truncate
if the paired lists differ in length; update the for-loop and the paired
creation to use zip(..., strict=True) so mismatched lengths raise an error:
change the for d, m in zip(_FAKE_SHOE_DOCS, _FAKE_SHOE_METAS) loop to use
strict=True and change the paired = list(zip(_FAKE_PLAYER_DOCS,
_FAKE_PLAYER_METAS)) call to use strict=True as well so the linter B905 is
satisfied and length mismatches are caught.
| def _fake_query_drills(query_texts, n_results=3, where=None): | ||
| """Return filtered drill results based on where clause.""" | ||
| docs, metas = [], [] | ||
| for d, m in zip(_FAKE_DRILL_DOCS, _FAKE_DRILL_METAS): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Python target and Ruff rule configuration for B905/zip(strict=...)
fd -H -a 'pyproject.toml|setup.cfg|ruff.toml|.ruff.toml' .
rg -n --no-heading 'requires-python|target-version|B905|select|ignore' \
pyproject.toml setup.cfg ruff.toml .ruff.toml 2>/dev/null || trueRepository: zweadfx/assist
Length of output: 453
🏁 Script executed:
cat ruff.tomlRepository: zweadfx/assist
Length of output: 1135
🏁 Script executed:
# Check if B905 is explicitly mentioned anywhere in config files
rg 'B905' .Repository: zweadfx/assist
Length of output: 40
🏁 Script executed:
# Look at the actual code at line 130 and surrounding context
sed -n '125,135p' tests/test_skill_lab.pyRepository: zweadfx/assist
Length of output: 432
🏁 Script executed:
# Check for per-file ignores that might affect tests/test_skill_lab.py
rg 'test_skill_lab|test.*\.py.*B905' ruff.tomlRepository: zweadfx/assist
Length of output: 40
🏁 Script executed:
# Find the definitions of _FAKE_DRILL_DOCS and _FAKE_DRILL_METAS
rg -n '_FAKE_DRILL_DOCS|_FAKE_DRILL_METAS' tests/test_skill_lab.py | head -20Repository: zweadfx/assist
Length of output: 168
🏁 Script executed:
# Get context around these fixture definitions
sed -n '1,100p' tests/test_skill_lab.py | head -50Repository: zweadfx/assist
Length of output: 1832
🏁 Script executed:
# Get the full definitions of both fixtures
sed -n '80,95p' tests/test_skill_lab.pyRepository: zweadfx/assist
Length of output: 647
🏁 Script executed:
# Get the complete _FAKE_DRILL_METAS definition
sed -n '88,120p' tests/test_skill_lab.pyRepository: zweadfx/assist
Length of output: 943
🏁 Script executed:
# Count the items in both fixtures
sed -n '80,86p' tests/test_skill_lab.py | grep -c '"'Repository: zweadfx/assist
Length of output: 59
🏁 Script executed:
# Get the complete _FAKE_DRILL_METAS list
sed -n '88,128p' tests/test_skill_lab.pyRepository: zweadfx/assist
Length of output: 1144
Add strict=True to zip() to satisfy the enabled B905 linting rule.
This zip() call lacks the strict= parameter. Since B905 is enabled in the ruff configuration and your project requires Python 3.10+, use zip(..., strict=True) to satisfy the rule and catch any future fixture-data length mismatches during tests.
Example fix
for d, m in zip(_FAKE_DRILL_DOCS, _FAKE_DRILL_METAS, strict=True):🧰 Tools
🪛 Ruff (0.15.6)
[warning] 130-130: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_skill_lab.py` at line 130, The zip call iterating over
_FAKE_DRILL_DOCS and _FAKE_DRILL_METAS should use strict=True to satisfy B905
and catch length mismatches; update the loop that currently does for d, m in
zip(_FAKE_DRILL_DOCS, _FAKE_DRILL_METAS): to call zip with strict=True (i.e.,
zip(_FAKE_DRILL_DOCS, _FAKE_DRILL_METAS, strict=True)) so the test fails if the
fixtures differ in length.
| def _fake_query_rules(query_texts, n_results=5, where=None): | ||
| """Return filtered rule results based on where clause.""" | ||
| docs, metas, dists = [], [], [] | ||
| for d, m in zip(_FAKE_RULE_DOCS, _FAKE_RULE_METAS): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Python and Ruff settings before enforcing zip(strict=...)
fd -H -a 'pyproject.toml|setup.cfg|ruff.toml|.ruff.toml' .
rg -n --no-heading 'requires-python|target-version|B905|select|ignore' \
pyproject.toml setup.cfg ruff.toml .ruff.toml 2>/dev/null || trueRepository: zweadfx/assist
Length of output: 453
🏁 Script executed:
# Check if B905 is in the enabled rules
cat ruff.toml | head -50Repository: zweadfx/assist
Length of output: 1135
🏁 Script executed:
# Examine the specific lines in tests/test_whistle.py
sed -n '100,125p' tests/test_whistle.pyRepository: zweadfx/assist
Length of output: 989
🏁 Script executed:
# Search for all zip() calls in the test file to understand context
rg -n 'zip\(' tests/test_whistle.py -A 1 -B 1Repository: zweadfx/assist
Length of output: 312
Add strict=True to both zip() calls (B905 rule violation).
The Ruff configuration explicitly enables flake8-bugbear rules ("B" in select), which includes B905. Both zip() calls at lines 103 and 120 lack the strict= parameter and violate this rule. Adding strict=True prevents silent truncation if the zipped sequences have different lengths, which helps catch bugs during testing.
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 103-103: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_whistle.py` at line 103, The two zip usages iterating test
fixtures (e.g., the loop using for d, m in zip(_FAKE_RULE_DOCS,
_FAKE_RULE_METAS) and the other zip at the later test) should be made strict to
avoid silent truncation; update both zip(...) calls to pass strict=True so the
test will raise if the sequences differ in length, leaving the rest of the loop
logic unchanged.
[Chore] ChatGPT 코드 리뷰 워크플로우 및 pytest CI 제거
1 participant
어떤 변경사항인가요?
CI 환경에서 pytest 실행 시 ChromaDB, OpenAI API 등 외부 의존성으로 인해 전체 테스트가 실패하던 문제를 mock 처리하여 해결했습니다.
작업 상세 내용
tests/conftest.py—generate_embeddings공유 mock (lifespan 초기화 시 OpenAI 호출 차단)tests/test_gear_advisor.py— ChromaDB 조회 및 OpenAI API 호출 mock 처리tests/test_skill_lab.py— ChromaDB 드릴 조회 mock 처리tests/test_weekly_coach.py— 기존 mock 유지, conftest로 lifespan 문제 해결tests/test_whistle.py— ChromaDB 규칙/용어 조회 mock 처리체크리스트
관련 이슈
리뷰 포인트
tests/conftest.py의autouse=Truefixture가 모든 테스트에 자동 적용되어 lifespan의 OpenAI 임베딩 호출을 차단하는 구조side_effect함수가 입력에 따라 필터링된 결과를 반환하여 실제 DB 로직을 적절히 시뮬레이션하는지참고사항 및 스크린샷(선택)
ValueError: Failed to retrieve from database,openai.RateLimitError: insufficient_quotaSummary by CodeRabbit