From ece12f951a2e57534d792b8990974a99a669d676 Mon Sep 17 00:00:00 2001 From: Yunxiao Xu Date: Tue, 17 Feb 2026 02:11:04 -0800 Subject: [PATCH] fix: Address second code review findings (test isolation, frontend type safety) --- backend/tests/api/test_api_auth.py | 18 ++++++++++-------- backend/tests/api/test_api_auth_cookie.py | 16 +++++++++------- backend/tests/api/test_theme.py | 10 ++++++---- frontend/src/App.tsx | 4 ++-- frontend/src/components/theme-provider.tsx | 2 +- frontend/src/services/auth.ts | 5 +++-- 6 files changed, 31 insertions(+), 24 deletions(-) diff --git a/backend/tests/api/test_api_auth.py b/backend/tests/api/test_api_auth.py index eca4977..13d7850 100644 --- a/backend/tests/api/test_api_auth.py +++ b/backend/tests/api/test_api_auth.py @@ -7,7 +7,9 @@ from ea_chatbot.history.models import User # We will need to mock HistoryManager and get_db dependencies later # For now, we define the expected behavior of the auth endpoints. -client = TestClient(app) +@pytest.fixture +def client(): + return TestClient(app) @pytest.fixture def mock_user(): @@ -19,7 +21,7 @@ def mock_user(): theme_preference="light" ) -def test_register_user_success(): +def test_register_user_success(client): """Test successful user registration.""" # We mock it where it is used in the router with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm: @@ -34,7 +36,7 @@ def test_register_user_success(): assert response.status_code == 201 assert response.json()["email"] == "new@example.com" -def test_login_success(): +def test_login_success(client): """Test successful login and JWT return.""" with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm: mock_hm.authenticate_user.return_value = User(id="1", username="test@example.com") @@ -48,7 +50,7 @@ def test_login_success(): assert "access_token" in response.json() assert response.json()["token_type"] == "bearer" -def test_login_invalid_credentials(): +def test_login_invalid_credentials(client): """Test login with wrong password.""" with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm: mock_hm.authenticate_user.return_value = None @@ -61,12 +63,12 @@ def test_login_invalid_credentials(): assert response.status_code == 401 assert "detail" in response.json() -def test_protected_route_without_token(): +def test_protected_route_without_token(client): """Test that protected routes require a token.""" response = client.get("/api/v1/auth/me") assert response.status_code == 401 -def test_oidc_login_redirect(): +def test_oidc_login_redirect(client): """Test that OIDC login returns a redirect URL and sets session cookie.""" with patch("ea_chatbot.api.routers.auth.oidc_client") as mock_oidc: mock_oidc.get_auth_data.return_value = { @@ -81,7 +83,7 @@ def test_oidc_login_redirect(): assert response.json()["url"] == "https://oidc-provider.com/auth" assert "oidc_session" in response.cookies -def test_oidc_callback_success(): +def test_oidc_callback_success(client): """Test successful OIDC callback and JWT issuance.""" with patch("ea_chatbot.api.routers.auth.oidc_client") as mock_oidc, \ patch("ea_chatbot.api.routers.auth.OIDCSession.decrypt") as mock_decrypt, \ @@ -105,7 +107,7 @@ def test_oidc_callback_success(): assert response.status_code == 302 assert "access_token" in response.cookies -def test_get_me_success(): +def test_get_me_success(client): """Test getting current user with a valid token.""" from ea_chatbot.api.utils import create_access_token token = create_access_token(data={"sub": "123"}) diff --git a/backend/tests/api/test_api_auth_cookie.py b/backend/tests/api/test_api_auth_cookie.py index 78b2393..088b723 100644 --- a/backend/tests/api/test_api_auth_cookie.py +++ b/backend/tests/api/test_api_auth_cookie.py @@ -5,7 +5,9 @@ from ea_chatbot.api.main import app from ea_chatbot.history.models import User from ea_chatbot.api.utils import create_access_token -client = TestClient(app) +@pytest.fixture +def client(): + return TestClient(app) @pytest.fixture def mock_user(): @@ -17,7 +19,7 @@ def mock_user(): theme_preference="light" ) -def test_v1_prefix(): +def test_v1_prefix(client): """Test that routes are prefixed with /api/v1.""" # This should now be 404 response = client.get("/auth/me") @@ -27,7 +29,7 @@ def test_v1_prefix(): response = client.get("/api/v1/auth/me") assert response.status_code == 401 -def test_login_sets_cookie(): +def test_login_sets_cookie(client): """Test that login sets the access_token cookie.""" with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm: mock_hm.authenticate_user.return_value = User(id="1", username="test@example.com") @@ -45,7 +47,7 @@ def test_login_sets_cookie(): assert "access_token" in set_cookie assert "HttpOnly" in set_cookie -def test_register_sets_cookie(): +def test_register_sets_cookie(client): """Test that register sets the access_token cookie.""" with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm: mock_hm.get_user.return_value = None @@ -59,7 +61,7 @@ def test_register_sets_cookie(): assert response.status_code == 201 assert "access_token" in response.cookies -def test_auth_via_cookie(): +def test_auth_via_cookie(client): """Test that protected routes work with the access_token cookie.""" token = create_access_token(data={"sub": "123"}) @@ -73,7 +75,7 @@ def test_auth_via_cookie(): assert response.status_code == 200 assert response.json()["email"] == "test@example.com" -def test_logout_clears_cookie(): +def test_logout_clears_cookie(client): """Test that logout endpoint clears the cookie.""" response = client.post("/api/v1/auth/logout") assert response.status_code == 200 @@ -81,7 +83,7 @@ def test_logout_clears_cookie(): cookie = response.cookies.get("access_token") assert not cookie or cookie == "" -def test_oidc_callback_redirects_with_cookie(): +def test_oidc_callback_redirects_with_cookie(client): """Test that OIDC callback sets cookie and redirects.""" with patch("ea_chatbot.api.routers.auth.oidc_client") as mock_oidc, \ patch("ea_chatbot.api.routers.auth.OIDCSession.decrypt") as mock_decrypt, \ diff --git a/backend/tests/api/test_theme.py b/backend/tests/api/test_theme.py index 0e9d88a..f7a3c9a 100644 --- a/backend/tests/api/test_theme.py +++ b/backend/tests/api/test_theme.py @@ -5,7 +5,9 @@ from ea_chatbot.api.main import app from ea_chatbot.history.models import User from ea_chatbot.api.utils import create_access_token -client = TestClient(app) +@pytest.fixture +def client(): + return TestClient(app) @pytest.fixture def test_user(): @@ -20,7 +22,7 @@ def test_user(): def auth_token(): return create_access_token(data={"sub": "user-123"}) -def test_get_me_includes_theme(test_user, auth_token): +def test_get_me_includes_theme(client, test_user, auth_token): """Test that /auth/me returns the theme_preference.""" with patch("ea_chatbot.api.dependencies.history_manager") as mock_hm: mock_hm.get_user_by_id.return_value = test_user @@ -33,7 +35,7 @@ def test_get_me_includes_theme(test_user, auth_token): assert "theme_preference" in data assert data["theme_preference"] == "light" -def test_update_theme_success(test_user, auth_token): +def test_update_theme_success(client, test_user, auth_token): """Test successful theme update via PATCH /auth/theme.""" updated_user = User( id="user-123", @@ -62,7 +64,7 @@ def test_update_theme_success(test_user, auth_token): assert data["theme_preference"] == "dark" mock_hm_router.update_user_theme.assert_called_once_with("user-123", "dark") -def test_update_theme_unauthorized(): +def test_update_theme_unauthorized(client): """Test that theme update requires authentication.""" response = client.patch( "/api/v1/auth/theme", diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index c34bdcc..ad45108 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -37,7 +37,7 @@ function App() { setUser(userData) setIsAuthenticated(true) if (userData.theme_preference) { - setThemeLocal(userData.theme_preference as "light" | "dark") + setThemeLocal(userData.theme_preference) } // Load history after successful auth loadHistory() @@ -67,7 +67,7 @@ function App() { setUser(userData) setIsAuthenticated(true) if (userData.theme_preference) { - setThemeLocal(userData.theme_preference as "light" | "dark") + setThemeLocal(userData.theme_preference) } loadHistory() } catch (err: unknown) { diff --git a/frontend/src/components/theme-provider.tsx b/frontend/src/components/theme-provider.tsx index 5b8fcce..3207bce 100644 --- a/frontend/src/components/theme-provider.tsx +++ b/frontend/src/components/theme-provider.tsx @@ -1,7 +1,7 @@ import { createContext, useContext, useEffect, useState } from "react" import { AuthService } from "@/services/auth" -type Theme = "light" | "dark" +export type Theme = "light" | "dark" interface ThemeContextType { theme: Theme diff --git a/frontend/src/services/auth.ts b/frontend/src/services/auth.ts index e838c99..e49dffb 100644 --- a/frontend/src/services/auth.ts +++ b/frontend/src/services/auth.ts @@ -1,4 +1,5 @@ import api from "./api" +import type { Theme } from "@/components/theme-provider" export interface AuthResponse { access_token: string @@ -9,7 +10,7 @@ export interface UserResponse { id: string email: string display_name?: string - theme_preference: string + theme_preference: Theme } export const AuthService = { @@ -51,7 +52,7 @@ export const AuthService = { await api.post("/auth/logout") }, - async updateTheme(theme: string): Promise { + async updateTheme(theme: Theme): Promise { const response = await api.patch("/auth/theme", { theme }) return response.data },