feat: ディレクトリ構成の見直し#11
Conversation
There was a problem hiding this comment.
Pull request overview
バックエンドのディレクトリ構成を見直し、従来の service 層を infra クライアント中心へ整理しつつ、DB 依存のテストを廃止してユースケースのユニットテスト(モックリポジトリ)へ移行するPRです。
Changes:
app.service.*を廃止し、app.infra.*Client(K8s/S3/LogStream)へ名称・参照を統一session_usecaseにリポジトリ/クライアントのDI(Protocol)を導入し、モック実装を追加- 既存の
backend/testsを削除し、pytest/mypy 設定とテスト配置をapp/配下へ移動
Reviewed changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/tests/test_repository.py | リポジトリのDB統合テストを削除 |
| backend/tests/test_api.py | FastAPI APIテストを削除 |
| backend/tests/conftest.py | テストDB/fixture を削除 |
| backend/pyproject.toml | pytest の収集パス変更、mypy override をテスト移動に合わせて更新 |
| backend/app/workflow/session_implementation_graph.py | K8s/S3 を *Client に切替、LogStream 参照更新 |
| backend/app/workflow/session_create_pr_graph.py | K8s/S3 を *Client に切替、LogStream 参照更新 |
| backend/app/workflow/session_analyzer_graph.py | K8s/S3 を *Client に切替、LogStream 参照更新 |
| backend/app/usecase/test_session_usecase.py | DB不要のユースケースユニットテストを新規追加 |
| backend/app/usecase/session_usecase.py | リポジトリProtocol対応・S3Client注入・旧S3Service参照除去 |
| backend/app/service/artifact_service.py | service 層の ArtifactService を削除 |
| backend/app/service/init.py | app.service 公開APIを削除 |
| backend/app/router/sessions.py | DI依存を get_*_client に変更し型も *Client に統一 |
| backend/app/repository/protocols.py | リポジトリProtocolを新規追加(DI/テスト用) |
| backend/app/repository/mock/mock_session_repository.py | モックSessionリポジトリを新規追加 |
| backend/app/repository/mock/mock_proposal_repository.py | モックProposalリポジトリを新規追加 |
| backend/app/repository/mock/mock_iteration_repository.py | モックIterationリポジトリを新規追加 |
| backend/app/repository/mock/init.py | モックリポジトリのexport追加 |
| backend/app/repository/init.py | Protocol群のexport追加 |
| backend/app/infra/s3_client.py | S3Service → S3Client に名称変更 |
| backend/app/infra/log_stream_client.py | LogStreamService → LogStreamClient、K8s参照更新 |
| backend/app/infra/k8s_client.py | K8sService → K8sClient に名称/説明更新 |
| backend/app/infra/init.py | infra package 初期化を追加 |
| backend/app/di/dependencies.py | get_*_service → get_*_client へ変更 |
| backend/app/di/container.py | LogStream のシングルトン管理を *_client に変更 |
| backend/app/core/middleware.py | 例外型を K8sClientError に更新 |
| backend/app/core/exceptions.py | K8sServiceError → K8sClientError に改名 |
| .gitignore | .ruff_cache を追加 |
Comments suppressed due to low confidence (1)
backend/app/core/middleware.py:39
- This log message still says "K8s service error" even though the exception type is now
K8sClientError. Updating the wording keeps logs consistent with the renamed component and makes grepping/alerting less confusing.
except K8sClientError as e:
logger.error("K8s service error: %s", e)
return JSONResponse(
status_code=503,
content={"detail": "Kubernetes service unavailable"},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def create(self, proposal: Proposal) -> Proposal: | ||
| self._store[proposal.id] = proposal | ||
| return proposal |
There was a problem hiding this comment.
MockProposalRepository.create() uses proposal.id as the dict key, but SQLAlchemy defaults (UUID) may not be assigned until flush/commit. To keep unit tests realistic and avoid None keys, have the mock generate/set an ID when it’s missing before storing the proposal.
|
|
||
| [tool.pytest.ini_options] | ||
| asyncio_mode = "auto" | ||
| testpaths = ["app"] |
There was a problem hiding this comment.
testpaths = ["app"] makes pytest only collect tests inside the production package. Since the previous backend/tests suite was removed, this effectively drops the API/repository integration coverage; consider including a dedicated tests directory in testpaths (or adding both paths) so non-package tests can run without being shipped in the wheel.
| testpaths = ["app"] | |
| testpaths = ["tests", "app"] |
| @pytest.mark.asyncio | ||
| @patch("app.usecase.session_usecase.asyncio.create_task") | ||
| async def test_happy_path(self, mock_create_task: MagicMock) -> None: | ||
| sr = MockSessionRepository() | ||
| ir = MockIterationRepository() | ||
| pr = MockProposalRepository() | ||
|
|
There was a problem hiding this comment.
The patched asyncio.create_task in these tests is a plain MagicMock, so the coroutine passed to it is never scheduled or closed. This can trigger "coroutine was never awaited" warnings and leak resources during the test run; set a side_effect that closes the coroutine (or use the earlier lambda coro: coro.close() approach) before asserting the call.
| def create(self, session: Session) -> Session: | ||
| self._store[session.id] = session | ||
| return session |
There was a problem hiding this comment.
MockSessionRepository.create() assumes session.id is already populated. With SQLAlchemy models, mapped_column(default=uuid.uuid4) is typically assigned on flush/commit, so Session(...) may still have id=None in unit tests. To match the real repository contract (and avoid using None as a key), assign a UUID in the mock when session.id is missing.
| def create(self, iteration: Iteration) -> Iteration: | ||
| self._store[iteration.id] = iteration | ||
| return iteration |
There was a problem hiding this comment.
MockIterationRepository.create() assumes iteration.id is already set. In code paths that construct Iteration(...) without flushing (e.g., usecase unit tests), the SQLAlchemy default UUID may not be assigned yet. To emulate the DB-backed repository behavior, generate and set an ID in the mock when it’s missing before storing the object.
No description provided.