[Chore] ChatGPT 코드 리뷰 워크플로우 및 pytest CI 제거#70
Merged
Conversation
3 tasks
📝 WalkthroughWalkthroughRemoved the GitHub Actions test workflow and simplified test infrastructure by eliminating mock fixtures for ChromaDB and embeddings. Tests now use real service instances directly instead of mocked data, reducing test coupling and simplifying test setup across multiple test modules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_gear_advisor.py (1)
244-286:⚠️ Potential issue | 🟠 MajorAPI endpoint tests now make real OpenAI API calls - likely unintended.
The
mock_gear_agentfixture was removed, but unliketest_whistle.py(which retainsmock_judge) andtest_skill_lab.py(which retainsmock_coach), this file no longer mocks the LLM invocation for API tests.Based on the context snippets,
gear_agent_graph.ainvoke()triggersgenerate_recommendations()which callschat_completion_with_retry()with thegpt-4omodel. This will:
- Incur OpenAI API costs on every test run
- Fail without
OPENAI_API_KEY- Be flaky due to non-deterministic LLM responses
Given the PR rationale mentions cost reduction, this appears unintentional.
🔧 Suggested fix: Add mock_gear_agent fixture
`@pytest.fixture` def mock_gear_agent(): """Patch gear_agent_graph.ainvoke to return a canned response.""" fake_response = { "final_response": json.dumps({ "recommendation_title": "테스트 추천", "user_profile_summary": "Test profile", "ai_reasoning": "Test reasoning", "shoes": [{ "shoe_id": "test-1", "brand": "TestBrand", "model_name": "TestModel", "price_krw": 150000, "sensory_tags": "테스트 태그", "match_score": 85, "recommendation_reason": "Test reason", }], }) } with patch( "src.api.v1.endpoints.gear.gear_agent_graph.ainvoke", return_value=fake_response, ) as mock: yield mockThen add
mock_gear_agentparameter totest_api_endpoint_success,test_api_endpoint_minimal_input, andtest_api_endpoint_with_all_parameters.🤖 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 244 - 286, Tests in tests/test_gear_advisor.py unintentionally call the real OpenAI API because the mock for gear_agent_graph.ainvoke was removed; add a pytest fixture named mock_gear_agent that patches src.api.v1.endpoints.gear.gear_agent_graph.ainvoke to return a canned response (a dict with "final_response" containing the JSON string of the expected recommendation payload) and then add mock_gear_agent as a parameter to test_api_endpoint_success, test_api_endpoint_minimal_input, and test_api_endpoint_with_all_parameters so generate_recommendations (which calls chat_completion_with_retry using model "gpt-4o") uses the stubbed response, preventing real API calls, flakiness, and API-key dependency.
🧹 Nitpick comments (1)
tests/test_skill_lab.py (1)
212-232: Silent pass when category returns empty results.The test iterates
results["metadatas"][0]without asserting non-empty results first. If a category returns zero matches, the assertion loop is skipped and the test passes silently.Consider adding an assertion to ensure results are returned:
💡 Suggested improvement
results = chroma_manager.query_drills( query_texts=[query_text], n_results=5, where={"category": category}, ) assert results and results.get("metadatas") + assert len(results["metadatas"][0]) > 0, ( + f"Expected results for category '{category}'" + ) for metadata in results["metadatas"][0]:🤖 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 212 - 232, The test test_tc03_category_filter_accuracy silently passes when a category yields zero matches because it iterates results["metadatas"][0] without asserting it's non-empty; update the test around the chroma_manager.query_drills call to explicitly assert that results is truthy, results.get("metadatas") exists, and that results["metadatas"][0] (or its length) is non-empty before iterating, e.g., assert results and results.get("metadatas") and len(results["metadatas"][0]) > 0 with a clear message including the category so an empty-result case fails the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/test_gear_advisor.py`:
- Around line 244-286: Tests in tests/test_gear_advisor.py unintentionally call
the real OpenAI API because the mock for gear_agent_graph.ainvoke was removed;
add a pytest fixture named mock_gear_agent that patches
src.api.v1.endpoints.gear.gear_agent_graph.ainvoke to return a canned response
(a dict with "final_response" containing the JSON string of the expected
recommendation payload) and then add mock_gear_agent as a parameter to
test_api_endpoint_success, test_api_endpoint_minimal_input, and
test_api_endpoint_with_all_parameters so generate_recommendations (which calls
chat_completion_with_retry using model "gpt-4o") uses the stubbed response,
preventing real API calls, flakiness, and API-key dependency.
---
Nitpick comments:
In `@tests/test_skill_lab.py`:
- Around line 212-232: The test test_tc03_category_filter_accuracy silently
passes when a category yields zero matches because it iterates
results["metadatas"][0] without asserting it's non-empty; update the test around
the chroma_manager.query_drills call to explicitly assert that results is
truthy, results.get("metadatas") exists, and that results["metadatas"][0] (or
its length) is non-empty before iterating, e.g., assert results and
results.get("metadatas") and len(results["metadatas"][0]) > 0 with a clear
message including the category so an empty-result case fails the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0017daf8-54af-4cd4-9090-43e4f7efb9eb
📒 Files selected for processing (5)
.github/workflows/test.ymltests/conftest.pytests/test_gear_advisor.pytests/test_skill_lab.pytests/test_whistle.py
💤 Files with no reviewable changes (2)
- tests/conftest.py
- .github/workflows/test.yml
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.
어떤 변경사항인가요?
작업 상세 내용
.github/workflows/test.yml삭제 (pytest CI 워크플로우)tests/conftest.py삭제 (공유 embedding mock)tests/test_gear_advisor.py원복 ([Chore] CI 테스트 외부 의존성 mock 처리 #67 mock 제거)tests/test_skill_lab.py원복 ([Chore] CI 테스트 외부 의존성 mock 처리 #67 mock 제거)tests/test_whistle.py원복 ([Chore] CI 테스트 외부 의존성 mock 처리 #67 mock 제거)체크리스트
관련 이슈
리뷰 포인트
참고사항 및 스크린샷(선택)
Summary by CodeRabbit