fix: address CodeRabbit PR feedback

- test_auth_routes.py: Complete incomplete tests for /auth/me endpoint
  with proper assertions and dependency overrides
- test_downloader.py: Remove unused dataclass import
- test_feed_sync.py: Strengthen assertion in test_add_podcast_existing_returns_info
- test_mcp_server.py: Fix empty test to verify module has main entry point,
  remove unused limit variable
- test_podcast_routes.py: Mock httpx.AsyncClient in test_search_limit_clamping
  for deterministic testing
- test_workers.py: Fix test_read_mp3_tags to test actual method behavior
  with a dummy file instead of mocking the method itself

Author: allenhutchison

tests/test_auth_routes.py

@@ -290,9 +290,10 @@ def test_logout_with_cookie_domain(self, app, mock_config):
 class TestMeRoute:
     """Tests for /auth/me endpoint."""
 
-    @patch("src.web.auth_routes.get_current_user")
-    def test_me_returns_user_info(self, mock_get_user, app, mock_repository):
+    def test_me_returns_user_info(self, app, mock_repository):
         """Test /me returns current user info."""
+        from src.web.auth import get_current_user
+
         mock_current_user = {
             "sub": "user-123",
             "email": "test@example.com",
@@ -305,20 +306,41 @@ def test_me_returns_user_info(self, mock_get_user, app, mock_repository):
         mock_repository.get_user.return_value = mock_db_user
 
         # Override dependency
-        app.dependency_overrides[
-            __import__("src.web.auth", fromlist=["get_current_user"]).get_current_user
-        ] = lambda: mock_current_user
+        app.dependency_overrides[get_current_user] = lambda: mock_current_user
 
         client = TestClient(app, raise_server_exceptions=False)
+        response = client.get("/auth/me")
+
+        assert response.status_code == 200
+        data = response.json()
+        assert data["email"] == "test@example.com"
+        assert data["is_admin"] is False
 
-        # Mock the dependency for this specific test
-        with patch("src.web.auth.get_current_user", return_value=mock_current_user):
-            # We need to override at the route level
-            pass
+        # Clean up override
+        app.dependency_overrides.clear()
 
-    @patch("src.web.auth_routes.get_current_user")
-    def test_me_returns_admin_status(self, mock_get_user, app, mock_repository):
+    def test_me_returns_admin_status(self, app, mock_repository):
         """Test /me returns admin status from database."""
+        from src.web.auth import get_current_user
+
+        mock_current_user = {
+            "sub": "admin-123",
+            "email": "admin@example.com",
+            "name": "Admin User",
+        }
+
         mock_db_user = Mock()
         mock_db_user.is_admin = True
         mock_repository.get_user.return_value = mock_db_user
+
+        app.dependency_overrides[get_current_user] = lambda: mock_current_user
+
+        client = TestClient(app, raise_server_exceptions=False)
+        response = client.get("/auth/me")
+
+        assert response.status_code == 200
+        data = response.json()
+        assert data["is_admin"] is True
+
+        # Clean up override
+        app.dependency_overrides.clear()

tests/test_downloader.py

@@ -5,7 +5,6 @@
 from datetime import datetime
 from pathlib import Path
 from unittest.mock import Mock, patch, MagicMock
-from dataclasses import dataclass
 
 from src.podcast.downloader import EpisodeDownloader, DownloadResult
 

tests/test_feed_sync.py

@@ -145,8 +145,10 @@ def test_add_podcast_existing_returns_info(self, sync_service, mock_repository):
 
         result = sync_service.add_podcast_from_url("https://example.com/feed.xml")
 
-        # Should return info about existing podcast
-        assert result.get("error") is not None or result.get("podcast_id") == "existing-id"
+        # Should return info about existing podcast with error message
+        assert result["podcast_id"] == "existing-id"
+        assert result["title"] == "Existing Podcast"
+        assert "already exists" in result["error"]
 
     def test_add_podcast_parse_error(self, sync_service, mock_repository):
         """Test adding a podcast when parsing fails."""

tests/test_mcp_server.py

@@ -56,12 +56,12 @@ def mock_config(self):
         config.GEMINI_FILE_SEARCH_STORE_NAME = "test-store"
         return config
 
-    def test_get_rag_context_decorator_exists(self):
-        """Test that get_rag_context tool is defined."""
-        # Import the module to verify the tool decorator structure
-        with patch("src.mcp_server.MCP"):
-            with patch("src.mcp_server.Config"):
-                pass  # Module structure verified
+    def test_mcp_server_module_has_main(self):
+        """Test that mcp_server module has expected entry point."""
+        import src.mcp_server
+        # Verify main entry point exists and is callable
+        assert hasattr(src.mcp_server, "main")
+        assert callable(src.mcp_server.main)
 
     @patch("src.mcp_server.GeminiSearchManager")
     def test_search_manager_created_with_config(self, mock_search_manager_class, mock_config):
@@ -311,12 +311,11 @@ def test_search_podcasts_no_limit(self, mock_search_manager_class, mock_config):
         mock_search_manager_class.return_value = mock_manager
 
         query = "search"
-        limit = None
 
         search_manager = mock_search_manager_class(config=mock_config, dry_run=False)
         results = search_manager.search_transcriptions(query, print_results=False)
 
-        # No limit applied
+        # No limit applied - all results returned
         response = {
             "query": query,
             "results": results,

tests/test_podcast_routes.py

@@ -771,15 +771,26 @@ def test_search_http_error(self, mock_client_class, client):
 
         assert response.status_code == 502
 
-    def test_search_limit_clamping(self, client):
+    @patch("src.web.podcast_routes.httpx.AsyncClient")
+    def test_search_limit_clamping(self, mock_client_class, client):
         """Test search limit is clamped between 1 and 50."""
         test_client, _ = client
 
-        # This test just verifies the endpoint handles limit param
-        # The actual clamping is tested when the iTunes API is called
+        mock_response = Mock()
+        mock_response.json.return_value = {"results": []}
+        mock_response.raise_for_status = Mock()
+
+        mock_async_client = MagicMock()
+        mock_async_client.__aenter__ = AsyncMock(return_value=mock_async_client)
+        mock_async_client.__aexit__ = AsyncMock(return_value=None)
+        mock_async_client.get = AsyncMock(return_value=mock_response)
+        mock_client_class.return_value = mock_async_client
+
+        # Request with limit > 50 should be clamped
         response = test_client.get("/api/podcasts/search?q=test&limit=100")
-        # Will fail because iTunes API isn't mocked, but request is valid
-        assert response.status_code in [200, 500, 502]
+        assert response.status_code == 200
+        # Verify the request was made (clamping happens internally)
+        mock_async_client.get.assert_called_once()
 
 
 class TestImportOPMLEndpoint:

tests/test_workers.py

@@ -680,15 +680,15 @@ def test_get_pending_count(self, metadata_worker, mock_repository):
         assert count == 2
         mock_repository.get_episodes_pending_metadata.assert_called_with(limit=1000)
 
-    def test_read_mp3_tags_success(self, metadata_worker, tmp_path):
-        """Test reading MP3 tags - mock the mutagen import."""
-        with patch("src.workflow.workers.metadata.MetadataWorker._read_mp3_tags") as mock_read:
-            mock_read.return_value = {"artist": "Test Artist", "album": "Test Album"}
-
-            tags = metadata_worker._read_mp3_tags("/path/to/file.mp3")
-
-            assert tags["artist"] == "Test Artist"
-            assert tags["album"] == "Test Album"
+    def test_read_mp3_tags_returns_dict(self, metadata_worker, tmp_path):
+        """Test that _read_mp3_tags returns a dictionary for valid MP3 files."""
+        # Create a dummy file (not a real MP3, so mutagen will fail gracefully)
+        dummy_file = tmp_path / "test.mp3"
+        dummy_file.write_bytes(b"fake mp3 content")
+
+        # The method should return empty dict for invalid MP3 files
+        tags = metadata_worker._read_mp3_tags(str(dummy_file))
+        assert isinstance(tags, dict)
 
     def test_read_mp3_tags_no_file(self, metadata_worker):
         """Test reading MP3 tags when file doesn't exist."""