🧪 Testing improvement for generate_summary - #270
Conversation
Added comprehensive unit tests for the generate_summary function in src/codeweaver/server/agent_api/search/response.py. Tests cover: - Empty match list handling - Standard summary generation with match count and intent - Top file name extraction and limiting to 3 unique files - Handling of duplicate file names - 1000-character truncation enforcement Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideAdds a new unit test module for generate_summary that exercises edge cases around empty results, result counting, unique filename listing, duplicate filenames, and summary length truncation to solidify the behavior of search result summary generation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The tests use bare
MagicMockobjects with a nestedfile.path.namechain, which can silently drift from the real match type; consider usingspec/spec_setor a simple helper class to better mirror the actual interface and catch attribute mistakes earlier. - The truncation test currently only checks
len(summary) <= 1000and that it ends with either'a'or'...', which is quite loose; tightening this to assert the exact max length and a more specific truncation pattern will make the test less brittle and more clearly tied to the intended behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests use bare `MagicMock` objects with a nested `file.path.name` chain, which can silently drift from the real match type; consider using `spec`/`spec_set` or a simple helper class to better mirror the actual interface and catch attribute mistakes earlier.
- The truncation test currently only checks `len(summary) <= 1000` and that it ends with either `'a'` or `'...'`, which is quite loose; tightening this to assert the exact max length and a more specific truncation pattern will make the test less brittle and more clearly tied to the intended behavior.
## Individual Comments
### Comment 1
<location path="tests/unit/server/agent_api/search/test_response.py" line_range="89-98" />
<code_context>
+ assert summary.count("shared.py") == 1
+
+ @pytest.mark.unit
+ def test_generate_summary_truncation(self) -> None:
+ """Test that summary is truncated to 1000 characters."""
+ # Create a match with an extremely long file name
+ m = MagicMock()
+ long_name = "a" * 2000
+ m.file.path.name = long_name
+
+ summary = generate_summary([m], IntentType.OPTIMIZE, "fast")
+
+ assert len(summary) <= 1000
+ assert summary.endswith(("a", "...")) # Depending on where it cuts
</code_context>
<issue_to_address>
**suggestion (testing):** Clarify and tighten the truncation assertions to better express the expected behavior
`summary.endswith(("a", "..."))` is both loose and unclear, since it allows any summary ending in `a` or `...` without validating the actual truncation behavior. Please assert a clearer contract instead: e.g., that `len(summary) == 1000`, that it begins with the expected prefix from the long filename, and—if the implementation should add an ellipsis on truncation—that it ends with `...`. This will more precisely document and validate the truncation semantics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_generate_summary_truncation(self) -> None: | ||
| """Test that summary is truncated to 1000 characters.""" | ||
| # Create a match with an extremely long file name | ||
| m = MagicMock() | ||
| long_name = "a" * 2000 | ||
| m.file.path.name = long_name | ||
|
|
||
| summary = generate_summary([m], IntentType.OPTIMIZE, "fast") | ||
|
|
||
| assert len(summary) <= 1000 |
There was a problem hiding this comment.
suggestion (testing): Clarify and tighten the truncation assertions to better express the expected behavior
summary.endswith(("a", "...")) is both loose and unclear, since it allows any summary ending in a or ... without validating the actual truncation behavior. Please assert a clearer contract instead: e.g., that len(summary) == 1000, that it begins with the expected prefix from the long filename, and—if the implementation should add an ellipsis on truncation—that it ends with .... This will more precisely document and validate the truncation semantics.
There was a problem hiding this comment.
Pull request overview
Adds focused unit tests to improve confidence in generate_summary behavior within the server-side search response utilities.
Changes:
- Added unit tests for
generate_summarycovering empty results, multiple results, unique file limiting (max 3), filename deduplication, and 1000-char truncation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
I've reviewed the new unit tests for generate_summary and found them to be well-structured and comprehensive. The tests cover all the key behaviors of the function effectively.
✅ Strengths
-
Comprehensive Coverage: The tests cover all important scenarios:
- Empty results
- Non-empty results with correct formatting
- Unique file name limiting (max 3 files from top results)
- Deduplication of file names
- 1000-character truncation
-
Good Test Structure:
- Clear test class organization
- Descriptive test names that explain intent
- Proper use of
@pytest.mark.unitmarkers - Good use of
MagicMockfor creating test fixtures
-
Implementation Alignment: The tests correctly verify the actual implementation behavior in
src/codeweaver/server/agent_api/search/response.py:81-104
📝 Observations
-
Test Assertion in
test_generate_summary_truncation(line 99):- The assertion
assert summary.endswith(("a", "..."))is a bit unusual - Based on the implementation (
return summary[:1000]), the string will be sliced at exactly 1000 characters, so it will end with "a" (not "...") - The comment "# Depending on where it cuts" suggests uncertainty, but the behavior is deterministic
- Consider simplifying to just
assert summary.endswith("a")or testing a specific truncation boundary
- The assertion
-
Intent Type Testing:
- The tests verify that different
IntentTypevalues are used (UNDERSTAND, IMPLEMENT, DEBUG, OPTIMIZE) - This is good practice as it ensures the function works with various intent types
- The implementation uses
intent_type.variablewhich converts to snake_case (e.g., "understand")
- The tests verify that different
-
File Path Mocking:
- The tests properly mock the nested structure
mock_match.file.path.name - This matches the actual code which accesses
m.file.path.name
- The tests properly mock the nested structure
✨ Suggestions for Improvement (Optional)
While not blocking, consider these enhancements:
-
Add a test for edge cases with the 3-file limit:
def test_generate_summary_exactly_three_files(self) -> None: """Test that summary works correctly with exactly 3 unique files.""" matches = [MagicMock(file=MagicMock(path=MagicMock(name=f"file_{i}.py"))) for i in range(3)] summary = generate_summary(matches, IntentType.UNDERSTAND, "test") assert all(f"file_{i}.py" in summary for i in range(3))
-
Test the exact format of the summary string to prevent regressions:
def test_generate_summary_format(self) -> None: """Test the exact format of the generated summary.""" mock_match = MagicMock(file=MagicMock(path=MagicMock(name="test.py"))) summary = generate_summary([mock_match], IntentType.DEBUG, "query") expected = "Found 1 relevant matches for debug query. Top results in: test.py" assert summary == expected
✅ Recommendation
APPROVE - The tests are well-written, provide good coverage, and correctly verify the implementation. The code quality is high and follows the project's testing patterns. The minor observation about the truncation test is not a blocker.
Great work on improving test coverage! 🎉
|
jules -- please address the following: Please address the comments from this code review: Overall Comments
Individual CommentsComment 1+ assert summary.count("shared.py") == 1 + + @pytest.mark.unit + def test_generate_summary_truncation(self) -> None: + """Test that summary is truncated to 1000 characters.""" + # Create a match with an extremely long file name + m = MagicMock() + long_name = "a" * 2000 + m.file.path.name = long_name + + summary = generate_summary([m], IntentType.OPTIMIZE, "fast") + + assert len(summary) <= 1000 + assert summary.endswith(("a", "...")) # Depending on where it cuts **suggestion (testing):** Clarify and tighten the truncation assertions to better express the expected behavior
|
I have addressed the feedback.
|
- Use a helper to create `CodeMatch` mocks with `spec` to mirror the actual interface and avoid attribute drift. - Tighten assertions for the truncation test to verify exact length (1000) and content consistency (prefix/suffix). - Ensure no unintended changes to `uv.lock`. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
…300390026924 Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Unit tests for response summary generation in response.py.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from unittest.mock import MagicMock | ||
|
|
||
| import pytest | ||
|
|
||
| from codeweaver.core import ( | ||
| ChunkKind, | ||
| CodeChunk, | ||
| ConfigLanguage, | ||
| DiscoveredFile, | ||
| ExtCategory, | ||
| SearchStrategy, | ||
| SemanticSearchLanguage, | ||
| Span, | ||
| ) | ||
| from codeweaver.core.utils import uuid7 | ||
| from codeweaver.server.agent_api.search.intent import IntentType | ||
| from codeweaver.server.agent_api.search.response import ( | ||
| build_success_response, | ||
| calculate_token_count, | ||
| extract_languages, | ||
| generate_summary, | ||
| ) | ||
| from codeweaver.server.agent_api.search.types import CodeMatch, CodeMatchType | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_get_indexer_state_info(): | ||
| with patch( | ||
| "codeweaver.server.agent_api.search.response.get_indexer_state_info" | ||
| ) as mock: | ||
| mock.return_value = ("unknown", None) | ||
| yield mock | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| @pytest.mark.parametrize( | ||
| "strategy,expected_mode,expected_status,expected_warning", | ||
| [ | ||
| (SearchStrategy.HYBRID_SEARCH, "hybrid", "success", None), | ||
| ( | ||
| SearchStrategy.DENSE_ONLY, | ||
| "dense_only", | ||
| "success", | ||
| "Sparse embeddings unavailable - using dense search only", | ||
| ), | ||
| ( | ||
| SearchStrategy.SPARSE_ONLY, | ||
| "sparse_only", | ||
| "partial", | ||
| "Dense embeddings unavailable - using sparse search only (degraded mode)", | ||
| ), | ||
| ( | ||
| SearchStrategy.KEYWORD_FALLBACK, | ||
| "sparse_only", | ||
| "partial", | ||
| "Dense embeddings unavailable - using sparse search only (degraded mode)", | ||
| ), | ||
| ], | ||
| ) | ||
| def test_build_success_response_search_modes( | ||
| strategy, expected_mode, expected_status, expected_warning, mock_get_indexer_state_info | ||
| ): | ||
| """Test successful response building with different search strategies and modes.""" | ||
| response = build_success_response( | ||
| code_matches=[], | ||
| query="test", | ||
| intent_type=IntentType.UNDERSTAND, | ||
| total_candidates=0, | ||
| token_limit=1000, | ||
| execution_time_ms=10.0, | ||
| strategies_used=[strategy], | ||
| ) | ||
|
|
||
| assert response.search_mode == expected_mode | ||
| assert response.status == expected_status | ||
| if expected_warning: | ||
| assert expected_warning in response.warnings | ||
| else: | ||
| assert not response.warnings | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def sample_code_matches(): | ||
| chunk1_id = uuid7() | ||
| chunk2_id = uuid7() | ||
| chunk3_id = uuid7() | ||
|
|
||
| return [ | ||
| CodeMatch( | ||
| file=DiscoveredFile( | ||
| path=Path("src/main.py"), | ||
| ext_category=ExtCategory( | ||
| language=SemanticSearchLanguage.PYTHON, kind=ChunkKind.CODE | ||
| ), | ||
| mime_type="text/x-python", | ||
| ), | ||
| content=CodeChunk( | ||
| chunk_id=chunk1_id, | ||
| chunk_name="main.py:hello_world", | ||
| file_path=Path("src/main.py"), | ||
| language="python", | ||
| content="def hello_world():\n print('Hello World!')", | ||
| line_range=Span(start=1, end=2, source_id=chunk1_id), | ||
| ), | ||
| span=Span(start=1, end=2, source_id=chunk1_id), | ||
| relevance_score=0.9, | ||
| match_type=CodeMatchType.SEMANTIC, | ||
| ), | ||
| CodeMatch( | ||
| file=DiscoveredFile( | ||
| path=Path("src/utils.py"), | ||
| ext_category=ExtCategory( | ||
| language=SemanticSearchLanguage.PYTHON, kind=ChunkKind.CODE | ||
| ), | ||
| mime_type="text/x-python", | ||
| ), | ||
| content=CodeChunk( | ||
| chunk_id=chunk2_id, | ||
| chunk_name="utils.py:helper", | ||
| file_path=Path("src/utils.py"), | ||
| language="python", | ||
| content="def helper():\n pass", | ||
| line_range=Span(start=1, end=2, source_id=chunk2_id), | ||
| ), | ||
| span=Span(start=1, end=2, source_id=chunk2_id), | ||
| relevance_score=0.8, | ||
| match_type=CodeMatchType.SEMANTIC, | ||
| ), | ||
| CodeMatch( | ||
| file=DiscoveredFile( | ||
| path=Path("config.json"), | ||
| ext_category=ExtCategory( | ||
| language=ConfigLanguage.JSON, kind=ChunkKind.CONFIG | ||
| ), | ||
| mime_type="application/json", | ||
| ), | ||
| content=CodeChunk( | ||
| chunk_id=chunk3_id, | ||
| chunk_name="config.json", | ||
| file_path=Path("config.json"), | ||
| language="json", | ||
| content='{"key": "value"}', | ||
| line_range=Span(start=1, end=1, source_id=chunk3_id), | ||
| ), | ||
| span=Span(start=1, end=1, source_id=chunk3_id), | ||
| relevance_score=0.5, | ||
| match_type=CodeMatchType.KEYWORD, | ||
| ), | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| def test_calculate_token_count(sample_code_matches): | ||
| """Test token count calculation.""" | ||
| # First item: 4 words * 1.3 ≈ 5.2, rounded/overhead -> 6 tokens | ||
| # Second item: 3 words * 1.3 = 3.9 -> 3 | ||
| # Third item: 2 words * 1.3 = 2.6 -> 2 | ||
| count = calculate_token_count(sample_code_matches, token_limit=1000) | ||
| assert count == 11 | ||
|
|
||
| # Test limit | ||
| count_capped = calculate_token_count(sample_code_matches, token_limit=10) | ||
| assert count_capped == 10 | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| def test_generate_summary_empty(): | ||
| """Test summary generation with empty matches.""" | ||
| summary = generate_summary([], IntentType.UNDERSTAND, "test query") | ||
| assert summary == "No matches found for query: 'test query'" | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| def test_generate_summary_populated(sample_code_matches): | ||
| """Test summary generation with matches.""" | ||
| summary = generate_summary(sample_code_matches, IntentType.DEBUG, "fix bug") | ||
| assert "Found 3 relevant matches for debug query" in summary | ||
| assert "Top results in" in summary | ||
| assert "main.py" in summary | ||
| assert "utils.py" in summary | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| def test_extract_languages(sample_code_matches): | ||
| """Test language extraction filters out config languages.""" | ||
| languages = extract_languages(sample_code_matches) | ||
| assert len(languages) == 1 | ||
| assert languages[0] == SemanticSearchLanguage.PYTHON | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| @patch("codeweaver.server.agent_api.search.response.FindCodeResponseSummary") | ||
| def test_build_success_response_with_matches(mock_response_summary, mock_get_indexer_state_info, sample_code_matches): | ||
| """Test full build_success_response integration with CodeMatch objects.""" | ||
| # We patch FindCodeResponseSummary so we can just verify the values passed to it, bypassing pydantic validation of fake objects | ||
| mock_response_summary.return_value = "MockedResponse" | ||
| response = build_success_response( | ||
| code_matches=sample_code_matches, | ||
| query="test code", | ||
| intent_type=IntentType.IMPLEMENT, | ||
| total_candidates=10, | ||
| token_limit=1000, | ||
| execution_time_ms=15.5, | ||
| strategies_used=[SearchStrategy.HYBRID_SEARCH], | ||
| ) | ||
|
|
||
| assert response == "MockedResponse" | ||
| mock_response_summary.assert_called_once() | ||
| kwargs = mock_response_summary.call_args[1] | ||
|
|
||
| assert kwargs["search_mode"] == "hybrid" | ||
| assert kwargs["status"] == "success" | ||
| assert len(kwargs["matches"]) == 3 | ||
| assert kwargs["total_matches"] == 10 | ||
| assert kwargs["total_results"] == 3 | ||
| assert "Found 3 relevant matches for implement query" in kwargs["summary"] | ||
| assert kwargs["token_count"] == 11 | ||
| assert kwargs["execution_time_ms"] == 15.5 | ||
| assert len(kwargs["languages_found"]) == 1 | ||
| assert kwargs["languages_found"][0] == SemanticSearchLanguage.PYTHON | ||
| from codeweaver.server.agent_api.search.response import generate_summary | ||
| from codeweaver.server.agent_api.search.types import CodeMatch |
There was a problem hiding this comment.
This change rewrites the test module and removes the existing unit coverage for other functions in response.py (e.g., build_success_response, calculate_token_count, extract_languages). A repo-wide search doesn’t show these tests moved elsewhere, so this PR reduces overall coverage rather than only adding generate_summary tests. Please restore/migrate the removed tests (can be in a separate file) and keep the new generate_summary coverage.
| """Unit tests for response summary generation in response.py.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from unittest.mock import MagicMock | ||
|
|
||
| import pytest | ||
|
|
||
| from codeweaver.core import ( | ||
| ChunkKind, | ||
| CodeChunk, | ||
| ConfigLanguage, | ||
| DiscoveredFile, | ||
| ExtCategory, | ||
| SearchStrategy, | ||
| SemanticSearchLanguage, | ||
| Span, | ||
| ) | ||
| from codeweaver.core.utils import uuid7 | ||
| from codeweaver.server.agent_api.search.intent import IntentType | ||
| from codeweaver.server.agent_api.search.response import ( | ||
| build_success_response, | ||
| calculate_token_count, | ||
| extract_languages, | ||
| generate_summary, | ||
| ) | ||
| from codeweaver.server.agent_api.search.types import CodeMatch, CodeMatchType | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_get_indexer_state_info(): | ||
| with patch( | ||
| "codeweaver.server.agent_api.search.response.get_indexer_state_info" | ||
| ) as mock: | ||
| mock.return_value = ("unknown", None) | ||
| yield mock | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| @pytest.mark.parametrize( | ||
| "strategy,expected_mode,expected_status,expected_warning", | ||
| [ | ||
| (SearchStrategy.HYBRID_SEARCH, "hybrid", "success", None), | ||
| ( | ||
| SearchStrategy.DENSE_ONLY, | ||
| "dense_only", | ||
| "success", | ||
| "Sparse embeddings unavailable - using dense search only", | ||
| ), | ||
| ( | ||
| SearchStrategy.SPARSE_ONLY, | ||
| "sparse_only", | ||
| "partial", | ||
| "Dense embeddings unavailable - using sparse search only (degraded mode)", | ||
| ), | ||
| ( | ||
| SearchStrategy.KEYWORD_FALLBACK, | ||
| "sparse_only", | ||
| "partial", | ||
| "Dense embeddings unavailable - using sparse search only (degraded mode)", | ||
| ), | ||
| ], | ||
| ) | ||
| def test_build_success_response_search_modes( | ||
| strategy, expected_mode, expected_status, expected_warning, mock_get_indexer_state_info | ||
| ): | ||
| """Test successful response building with different search strategies and modes.""" | ||
| response = build_success_response( | ||
| code_matches=[], | ||
| query="test", | ||
| intent_type=IntentType.UNDERSTAND, | ||
| total_candidates=0, | ||
| token_limit=1000, | ||
| execution_time_ms=10.0, | ||
| strategies_used=[strategy], | ||
| ) | ||
|
|
||
| assert response.search_mode == expected_mode | ||
| assert response.status == expected_status | ||
| if expected_warning: | ||
| assert expected_warning in response.warnings | ||
| else: | ||
| assert not response.warnings | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def sample_code_matches(): | ||
| chunk1_id = uuid7() | ||
| chunk2_id = uuid7() | ||
| chunk3_id = uuid7() | ||
|
|
||
| return [ | ||
| CodeMatch( | ||
| file=DiscoveredFile( | ||
| path=Path("src/main.py"), | ||
| ext_category=ExtCategory( | ||
| language=SemanticSearchLanguage.PYTHON, kind=ChunkKind.CODE | ||
| ), | ||
| mime_type="text/x-python", | ||
| ), | ||
| content=CodeChunk( | ||
| chunk_id=chunk1_id, | ||
| chunk_name="main.py:hello_world", | ||
| file_path=Path("src/main.py"), | ||
| language="python", | ||
| content="def hello_world():\n print('Hello World!')", | ||
| line_range=Span(start=1, end=2, source_id=chunk1_id), | ||
| ), | ||
| span=Span(start=1, end=2, source_id=chunk1_id), | ||
| relevance_score=0.9, | ||
| match_type=CodeMatchType.SEMANTIC, | ||
| ), | ||
| CodeMatch( | ||
| file=DiscoveredFile( | ||
| path=Path("src/utils.py"), | ||
| ext_category=ExtCategory( | ||
| language=SemanticSearchLanguage.PYTHON, kind=ChunkKind.CODE | ||
| ), | ||
| mime_type="text/x-python", | ||
| ), | ||
| content=CodeChunk( | ||
| chunk_id=chunk2_id, | ||
| chunk_name="utils.py:helper", | ||
| file_path=Path("src/utils.py"), | ||
| language="python", | ||
| content="def helper():\n pass", | ||
| line_range=Span(start=1, end=2, source_id=chunk2_id), | ||
| ), | ||
| span=Span(start=1, end=2, source_id=chunk2_id), | ||
| relevance_score=0.8, | ||
| match_type=CodeMatchType.SEMANTIC, | ||
| ), | ||
| CodeMatch( | ||
| file=DiscoveredFile( | ||
| path=Path("config.json"), | ||
| ext_category=ExtCategory( | ||
| language=ConfigLanguage.JSON, kind=ChunkKind.CONFIG | ||
| ), | ||
| mime_type="application/json", | ||
| ), | ||
| content=CodeChunk( | ||
| chunk_id=chunk3_id, | ||
| chunk_name="config.json", | ||
| file_path=Path("config.json"), | ||
| language="json", | ||
| content='{"key": "value"}', | ||
| line_range=Span(start=1, end=1, source_id=chunk3_id), | ||
| ), | ||
| span=Span(start=1, end=1, source_id=chunk3_id), | ||
| relevance_score=0.5, | ||
| match_type=CodeMatchType.KEYWORD, | ||
| ), | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| def test_calculate_token_count(sample_code_matches): | ||
| """Test token count calculation.""" | ||
| # First item: 4 words * 1.3 ≈ 5.2, rounded/overhead -> 6 tokens | ||
| # Second item: 3 words * 1.3 = 3.9 -> 3 | ||
| # Third item: 2 words * 1.3 = 2.6 -> 2 | ||
| count = calculate_token_count(sample_code_matches, token_limit=1000) | ||
| assert count == 11 | ||
|
|
||
| # Test limit | ||
| count_capped = calculate_token_count(sample_code_matches, token_limit=10) | ||
| assert count_capped == 10 | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| def test_generate_summary_empty(): | ||
| """Test summary generation with empty matches.""" | ||
| summary = generate_summary([], IntentType.UNDERSTAND, "test query") | ||
| assert summary == "No matches found for query: 'test query'" | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| def test_generate_summary_populated(sample_code_matches): | ||
| """Test summary generation with matches.""" | ||
| summary = generate_summary(sample_code_matches, IntentType.DEBUG, "fix bug") | ||
| assert "Found 3 relevant matches for debug query" in summary | ||
| assert "Top results in" in summary | ||
| assert "main.py" in summary | ||
| assert "utils.py" in summary | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| def test_extract_languages(sample_code_matches): | ||
| """Test language extraction filters out config languages.""" | ||
| languages = extract_languages(sample_code_matches) | ||
| assert len(languages) == 1 | ||
| assert languages[0] == SemanticSearchLanguage.PYTHON | ||
|
|
||
|
|
||
| @pytest.mark.unit | ||
| @patch("codeweaver.server.agent_api.search.response.FindCodeResponseSummary") | ||
| def test_build_success_response_with_matches(mock_response_summary, mock_get_indexer_state_info, sample_code_matches): | ||
| """Test full build_success_response integration with CodeMatch objects.""" | ||
| # We patch FindCodeResponseSummary so we can just verify the values passed to it, bypassing pydantic validation of fake objects | ||
| mock_response_summary.return_value = "MockedResponse" | ||
| response = build_success_response( | ||
| code_matches=sample_code_matches, | ||
| query="test code", | ||
| intent_type=IntentType.IMPLEMENT, | ||
| total_candidates=10, | ||
| token_limit=1000, | ||
| execution_time_ms=15.5, | ||
| strategies_used=[SearchStrategy.HYBRID_SEARCH], | ||
| ) | ||
|
|
||
| assert response == "MockedResponse" | ||
| mock_response_summary.assert_called_once() | ||
| kwargs = mock_response_summary.call_args[1] | ||
|
|
||
| assert kwargs["search_mode"] == "hybrid" | ||
| assert kwargs["status"] == "success" | ||
| assert len(kwargs["matches"]) == 3 | ||
| assert kwargs["total_matches"] == 10 | ||
| assert kwargs["total_results"] == 3 | ||
| assert "Found 3 relevant matches for implement query" in kwargs["summary"] | ||
| assert kwargs["token_count"] == 11 | ||
| assert kwargs["execution_time_ms"] == 15.5 | ||
| assert len(kwargs["languages_found"]) == 1 | ||
| assert kwargs["languages_found"][0] == SemanticSearchLanguage.PYTHON | ||
| from codeweaver.server.agent_api.search.response import generate_summary | ||
| from codeweaver.server.agent_api.search.types import CodeMatch |
There was a problem hiding this comment.
PR description says it "added missing unit tests" for generate_summary, but the diff also deletes a large set of pre-existing tests in this file (including response-building/search-mode behavior). If the intent is a focused generate_summary test file, consider moving the removed tests to a different module rather than dropping them, or update the PR description to reflect the test removals.
Code Review - PR #270Critical Issue: Test Coverage Regression
|
There was a problem hiding this comment.
Review Summary
This PR improves test coverage for generate_summary but removes significant existing test coverage for other functions in the same module. This is a blocking issue.
Critical Issue: Loss of Test Coverage
The PR deletes 217 lines of tests while adding 104 new ones. The deleted tests covered:
test_build_success_response_search_modes- Different search strategies (HYBRID_SEARCH, DENSE_ONLY, SPARSE_ONLY, KEYWORD_FALLBACK)test_calculate_token_count- Token counting logictest_extract_languages- Language extraction filteringtest_build_success_response_with_matches- Full response building integration
I verified these tests don't exist elsewhere in the codebase. The functions (build_success_response, calculate_token_count, extract_languages) are exported in the module's __all__, making them part of the public API that needs test coverage.
Issues with New Tests
-
Mocks vs Real Objects: The new tests use
MagicMockobjects instead of realCodeMatchinstances. The deleted tests used actual objects (DiscoveredFile,CodeChunk,Span), which better test real interfaces and catch integration issues. -
CI Failures: Tests are failing on Python 3.12, 3.13, 3.13t, and 3.14.
Positive Aspects
- Good coverage of
generate_summaryedge cases (empty results, file limiting, deduplication, truncation) - Tighter assertions for truncation test with exact length checks
- Clear test documentation with descriptive docstrings
- Good use of pytest class organization
Recommendations
Option 1 (Recommended): Keep the new generate_summary tests AND restore the deleted tests. It's normal for a test file to cover multiple functions from the same module.
Option 2: Split into multiple test files if you want focused test files:
test_response_summary.py→generate_summarytest_response_builder.py→build_success_responseand relatedtest_response_utils.py→calculate_token_count,extract_languages
Option 3: If intentionally removing tests, update the PR description to explain which tests are being removed and why (rather than "added missing unit tests").
Verdict
Requesting changes - Please restore the deleted test coverage or provide equivalent tests elsewhere before merging.
🎯 What: Added missing unit tests for the
generate_summaryfunction insrc/codeweaver/server/agent_api/search/response.py.📊 Coverage: Scenarios covered include empty results, successful results with multiple matches, unique file name limiting (max 3), file name deduplication, and summary length truncation (1000 chars).
✨ Result: Improved test coverage and reliability for search result summary generation.
PR created automatically by Jules for task 9454634300390026924 started by Adam Poulemanos (@bashandbone)
Summary by Sourcery
Tests: