From 18a7cab36f785f1f9ba5820be93f3e5b97f2acf2 Mon Sep 17 00:00:00 2001 From: Luis Novo Date: Mon, 6 Apr 2026 07:42:20 -0300 Subject: [PATCH] fix: improve test quality for #627 and #651 - #627: Replace model-construction tests with endpoint-level tests that exercise the real create_source async path via TestClient, capturing the Source instance passed to save() using patch.object(autospec=True) - #651: Use assert_awaited_once() instead of assert_called_once() on AsyncMock methods to catch missing await bugs - Remove redundant class-level @patch for Source.save in title tests --- tests/test_bug_fixes.py | 205 ++++++++++++++++++++++++---------------- 1 file changed, 123 insertions(+), 82 deletions(-) diff --git a/tests/test_bug_fixes.py b/tests/test_bug_fixes.py index 1f7bfc6..3ba13d5 100644 --- a/tests/test_bug_fixes.py +++ b/tests/test_bug_fixes.py @@ -25,83 +25,126 @@ def client(): class TestAsyncSourceAssetPersistence: - """Tests for #627 - asset is persisted before async processing.""" + """Tests for #627 - asset is persisted before async processing. - def test_source_created_with_url_asset(self): - """Source created for link type has asset with url.""" - asset = Asset(url="https://example.com/article") - source = Source( - title="Processing...", - topics=[], - asset=asset, - ) + These tests hit the real create_source endpoint with mocked DB/command + calls, verifying that the Source saved to the database has the correct + asset set *before* async processing begins. + """ + + @pytest.mark.asyncio + @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) + @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) + @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) + async def test_async_link_source_persists_url_asset( + self, mock_nb_get, mock_add_nb, mock_submit, client + ): + """POST /sources with type=link and async_processing=true persists Asset(url=...).""" + mock_nb_get.return_value = MagicMock() # notebook exists + mock_submit.return_value = "command:123" + + # Track the Source instance that save() is called on + saved_sources = [] + + async def capture_save(self_source): + saved_sources.append(self_source) + self_source.id = "source:fake" + self_source.command = None + + with patch.object(Source, "save", autospec=True, side_effect=capture_save): + response = client.post( + "/api/sources", + data={ + "type": "link", + "url": "https://example.com/article", + "notebooks": '["notebook:1"]', + "async_processing": "true", + }, + ) + + assert response.status_code == 200 + assert len(saved_sources) >= 1 + + # The first save should have the asset with URL + source = saved_sources[0] assert source.asset is not None assert source.asset.url == "https://example.com/article" assert source.asset.file_path is None - def test_source_created_with_file_asset(self): - """Source created for upload type has asset with file_path.""" - asset = Asset(file_path="/tmp/uploads/video.mp4") - source = Source( - title="Processing...", - topics=[], - asset=asset, - ) + @pytest.mark.asyncio + @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) + @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) + @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) + @patch("api.routers.sources.save_uploaded_file", new_callable=AsyncMock) + async def test_async_upload_source_persists_file_asset( + self, mock_upload, mock_nb_get, mock_add_nb, mock_submit, client + ): + """POST /sources with type=upload and async_processing=true persists Asset(file_path=...).""" + mock_nb_get.return_value = MagicMock() + mock_upload.return_value = "/tmp/uploads/video.mp4" + mock_submit.return_value = "command:123" + + saved_sources = [] + + async def capture_save(self_source): + saved_sources.append(self_source) + self_source.id = "source:fake" + self_source.command = None + + with patch.object(Source, "save", autospec=True, side_effect=capture_save): + response = client.post( + "/api/sources", + data={ + "type": "upload", + "notebooks": '["notebook:1"]', + "async_processing": "true", + }, + files={"file": ("video.mp4", b"fake content", "video/mp4")}, + ) + + assert response.status_code == 200 + assert len(saved_sources) >= 1 + + source = saved_sources[0] assert source.asset is not None assert source.asset.file_path == "/tmp/uploads/video.mp4" assert source.asset.url is None - def test_source_created_without_asset_for_text(self): - """Source created for text type has no asset.""" - source = Source( - title="Processing...", - topics=[], - asset=None, - ) + @pytest.mark.asyncio + @patch("api.routers.sources.CommandService.submit_command_job", new_callable=AsyncMock) + @patch("api.routers.sources.Source.add_to_notebook", new_callable=AsyncMock) + @patch("api.routers.sources.Notebook.get", new_callable=AsyncMock) + async def test_async_text_source_has_no_asset( + self, mock_nb_get, mock_add_nb, mock_submit, client + ): + """POST /sources with type=text and async_processing=true has asset=None.""" + mock_nb_get.return_value = MagicMock() + mock_submit.return_value = "command:123" + + saved_sources = [] + + async def capture_save(self_source): + saved_sources.append(self_source) + self_source.id = "source:fake" + self_source.command = None + + with patch.object(Source, "save", autospec=True, side_effect=capture_save): + response = client.post( + "/api/sources", + data={ + "type": "text", + "content": "Some text content", + "notebooks": '["notebook:1"]', + "async_processing": "true", + }, + ) + + assert response.status_code == 200 + assert len(saved_sources) >= 1 + + source = saved_sources[0] assert source.asset is None - def test_retry_with_url_asset(self): - """Retry endpoint can reconstruct content_state from url asset.""" - source = Source( - title="Processing...", - topics=[], - asset=Asset(url="https://example.com/video"), - ) - # Simulate what the retry endpoint does - content_state = {} - if source.asset: - if source.asset.file_path: - content_state = { - "file_path": source.asset.file_path, - "delete_source": False, - } - elif source.asset.url: - content_state = {"url": source.asset.url} - - assert content_state == {"url": "https://example.com/video"} - - def test_retry_with_file_asset(self): - """Retry endpoint can reconstruct content_state from file asset.""" - source = Source( - title="Processing...", - topics=[], - asset=Asset(file_path="/tmp/uploads/doc.pdf"), - ) - content_state = {} - if source.asset: - if source.asset.file_path: - content_state = { - "file_path": source.asset.file_path, - "delete_source": False, - } - elif source.asset.url: - content_state = {"url": source.asset.url} - - assert content_state == { - "file_path": "/tmp/uploads/doc.pdf", - "delete_source": False, - } - # ============================================================================ # TEST SUITE 2: #670 - Custom title preservation @@ -113,8 +156,7 @@ class TestTitlePreservation: @pytest.mark.asyncio @patch("open_notebook.graphs.source.Source.get") - @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) - async def test_custom_title_preserved(self, mock_save, mock_get): + async def test_custom_title_preserved(self, mock_get): """User-set title is NOT overwritten by content_state.title.""" from open_notebook.graphs.source import save_source @@ -138,13 +180,12 @@ class TestTitlePreservation: await save_source(state) - # Title should remain as the custom user title assert mock_source.title == "My Custom Research Title" + mock_source.save.assert_awaited_once() @pytest.mark.asyncio @patch("open_notebook.graphs.source.Source.get") - @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) - async def test_placeholder_title_replaced(self, mock_save, mock_get): + async def test_placeholder_title_replaced(self, mock_get): """Placeholder 'Processing...' title IS replaced by extracted title.""" from open_notebook.graphs.source import save_source @@ -169,11 +210,11 @@ class TestTitlePreservation: await save_source(state) assert mock_source.title == "Extracted Article Title" + mock_source.save.assert_awaited_once() @pytest.mark.asyncio @patch("open_notebook.graphs.source.Source.get") - @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) - async def test_none_title_replaced(self, mock_save, mock_get): + async def test_none_title_replaced(self, mock_get): """None title IS replaced by extracted title.""" from open_notebook.graphs.source import save_source @@ -198,11 +239,11 @@ class TestTitlePreservation: await save_source(state) assert mock_source.title == "Extracted Title" + mock_source.save.assert_awaited_once() @pytest.mark.asyncio @patch("open_notebook.graphs.source.Source.get") - @patch("open_notebook.graphs.source.Source.save", new_callable=AsyncMock) - async def test_empty_title_replaced(self, mock_save, mock_get): + async def test_empty_title_replaced(self, mock_get): """Empty string title IS replaced by extracted title.""" from open_notebook.graphs.source import save_source @@ -227,6 +268,7 @@ class TestTitlePreservation: await save_source(state) assert mock_source.title == "Extracted Title" + mock_source.save.assert_awaited_once() # ============================================================================ @@ -265,10 +307,9 @@ class TestCredentialCascadeDelete: assert data["deleted_models"] == 2 assert data["message"] == "Credential deleted successfully" - # Verify models were deleted - mock_model1.delete.assert_called_once() - mock_model2.delete.assert_called_once() - mock_cred.delete.assert_called_once() + mock_model1.delete.assert_awaited_once() + mock_model2.delete.assert_awaited_once() + mock_cred.delete.assert_awaited_once() @pytest.mark.asyncio @patch("api.routers.credentials.Credential.get") @@ -284,7 +325,7 @@ class TestCredentialCascadeDelete: assert response.status_code == 200 data = response.json() assert data["deleted_models"] == 0 - mock_cred.delete.assert_called_once() + mock_cred.delete.assert_awaited_once() @pytest.mark.asyncio @patch("api.routers.credentials.Credential.get") @@ -312,9 +353,9 @@ class TestCredentialCascadeDelete: assert response.status_code == 200 data = response.json() assert data["deleted_models"] == 0 # Models were migrated, not deleted - mock_model.save.assert_called_once() + mock_model.save.assert_awaited_once() assert mock_model.credential == "cred:456" - mock_cred.delete.assert_called_once() + mock_cred.delete.assert_awaited_once() if __name__ == "__main__":