diff --git a/TEST_COVERAGE_REPORT.md b/TEST_COVERAGE_REPORT.md new file mode 100644 index 0000000..1a7af93 --- /dev/null +++ b/TEST_COVERAGE_REPORT.md @@ -0,0 +1,250 @@ +# Test Coverage Report - Phase 3 Refactoring + +**Generated:** 2025-10-25 +**Branch:** phase-3 +**Total Tests:** 34 (all passing in 0.14s) + +## Executive Summary + +✅ **Overall Status:** GOOD - Most functionality is well-tested +⚠️ **Concerns:** 1 bug found in `autocomplete_address` MCP tool, some modules lack direct unit tests +📊 **Coverage:** Indirect coverage is good through integration tests, but direct unit tests for new modules would improve testability + +## Test Coverage by Module + +### ✅ Well Tested (Indirect Coverage via Integration Tests) + +| Module | Functions | Test Coverage | Notes | +|--------|-----------|---------------|-------| +| `client.py` | 9 API methods | ✅ High | Tested through GovmapClient integration tests | +| `filters.py` | `filter_deals_by_criteria` | ✅ High | 8 dedicated tests covering all filter types | +| `statistics.py` | `calculate_deal_statistics`, `calculate_std_dev` | ✅ Good | 1 dedicated test, used in other tests | +| `market_analysis.py` | 4 functions | ✅ High | 6 dedicated tests for market analysis | +| `utils.py` | `is_same_building` | ✅ Good | 1 dedicated test | + +### ⚠️ Missing Direct Unit Tests + +| Module | Functions | Test Coverage | Impact | Recommendation | +|--------|-----------|---------------|--------|----------------| +| `validators.py` | 4 validation functions | ⚠️ Indirect only | Low | Add unit tests for edge cases | +| `utils.py` | `calculate_distance`, `extract_floor_number` | ⚠️ Indirect only | Low | Add unit tests for Hebrew floor parsing | +| `market_analysis.py` | `parse_deal_dates` (helper) | ⚠️ Indirect only | Low | Already tested via parent functions | + +## E2E Test Results (MCP Tools) + +Tested the following MCP tools with real data: + +### ✅ Passing E2E Tests + +1. **`find_recent_deals_for_address`** + - Input: `"הרצל 1 תל אביב"`, 1 year, 100m radius, max 5 deals + - Result: ✅ SUCCESS - Returned 5 deals with complete statistics + - Data Quality: Excellent - all fields populated correctly + +2. **`analyze_market_trends`** + - Input: `"דיזנגוף 50 תל אביב"`, 2 years + - Result: ✅ SUCCESS - Returned 82 deals analyzed + - Output: Comprehensive yearly trends, property type breakdown, neighborhoods + - Data Quality: Excellent + +3. **`get_valuation_comparables`** + - Input: `"רוטשילד 1 תל אביב"`, property_type="דירה", rooms 2-4, max 5 + - Result: ✅ SUCCESS - Returned 1 comparable with statistics + - Filtering: ✅ Working correctly (rooms, property type) + - Token Usage: ✅ Optimized (no bloat fields) + +### ❌ Bug Found: `autocomplete_address` + +**Status:** 🐛 BUG - Incorrect field mapping + +**Problem:** +The `autocomplete_address` tool in `fastmcp_server.py` uses incorrect field names when parsing the API response. + +**Current Code (WRONG):** +```python +formatted_results.append({ + "address": result.get("addressLabel", ""), # ❌ Wrong field + "settlement": result.get("settlementNameHeb", ""), # ❌ Wrong field + "coordinates": result.get("coordinates", {}), # ❌ Wrong field + "polygon_id": result.get("polygon_id") # ❌ Wrong field +}) +``` + +**Actual API Response Fields:** +```python +{ + "id": "address|ADDR|123|test", + "text": "תל אביב", # ✅ Use this for address + "type": "address", + "score": 100, + "shape": "POINT(3870000.123 3770000.456)", # ✅ Parse this for coordinates + "data": {} +} +``` + +**Impact:** HIGH - The tool returns empty data for all fields + +**Fix Required:** +```python +# Parse coordinates from WKT POINT format +shape_str = result.get("shape", "") +coordinates = {} +if shape_str.startswith("POINT("): + coords_str = shape_str[6:-1] # Remove "POINT(" and ")" + coords = coords_str.split() + if len(coords) == 2: + coordinates = { + "longitude": float(coords[0]), + "latitude": float(coords[1]) + } + +formatted_results.append({ + "text": result.get("text", ""), # ✅ Display text + "id": result.get("id", ""), # ✅ Unique ID + "type": result.get("type", ""), # ✅ Result type + "score": result.get("score", 0), # ✅ Match score + "coordinates": coordinates # ✅ Parsed coordinates +}) +``` + +## Test Organization + +### Current Structure ✅ +``` +tests/ +└── test_govmap_client.py (34 tests) + ├── TestGovmapClient (12 tests) + └── TestMarketAnalysisFunctions (22 tests) +``` + +### Recommended Structure 📋 +``` +tests/ +├── test_govmap_client.py (existing integration tests) +├── govmap/ +│ ├── test_validators.py (NEW - 10-15 tests) +│ ├── test_utils.py (NEW - 8-10 tests) +│ ├── test_filters.py (refactor existing) +│ ├── test_statistics.py (refactor existing) +│ └── test_market_analysis.py (refactor existing) +└── test_fastmcp_tools.py (NEW - E2E tool tests) +``` + +## Missing Test Cases + +### High Priority + +1. **Validator Edge Cases** + ```python + # validators.py + - Test validate_address with empty string + - Test validate_address with very long address (>500 chars) + - Test validate_coordinates with out-of-bounds ITM coordinates + - Test validate_positive_int with negative numbers + - Test validate_deal_type with invalid types (3, 0, -1) + ``` + +2. **Utils Hebrew Floor Parsing** + ```python + # utils.py + - Test extract_floor_number with all Hebrew floor names + - Test extract_floor_number with numeric strings + - Test extract_floor_number with invalid input + - Test calculate_distance with same point + - Test calculate_distance with far points + ``` + +3. **MCP Tool E2E Tests** + ```python + # test_fastmcp_tools.py + - Test autocomplete_address with real API + - Test get_deals_by_radius with edge coordinates + - Test error handling for all tools + - Test token limits for large responses + ``` + +### Medium Priority + +4. **Client Error Handling** + ```python + # client.py + - Test retry logic with transient failures + - Test rate limiting behavior + - Test timeout handling + - Test invalid API responses + ``` + +5. **Filter Edge Cases** + ```python + # filters.py + - Test filtering with all null values + - Test filtering with overlapping criteria + - Test filtering with no matches + ``` + +### Low Priority + +6. **Integration Tests** + ```python + # Integration with real API + - Test full workflow: autocomplete → find deals → analyze + - Test with various Israeli cities + - Test with English addresses + ``` + +## Recommendations + +### Immediate Actions (Before Merge) + +1. **🔴 CRITICAL: Fix `autocomplete_address` bug** + - File: `nadlan_mcp/fastmcp_server.py` lines 65-71 + - Estimated time: 10 minutes + - Add test to prevent regression + +### Short-term Actions (Next Sprint) + +2. **🟡 Add validator unit tests** + - Create `tests/govmap/test_validators.py` + - 10-15 tests covering edge cases + - Estimated time: 1-2 hours + +3. **🟡 Add utils unit tests** + - Create `tests/govmap/test_utils.py` + - Focus on Hebrew floor parsing and distance calculation + - Estimated time: 1 hour + +4. **🟡 Add MCP tool E2E tests** + - Create `tests/test_fastmcp_tools.py` + - Test all 10 MCP tools with real data + - Estimated time: 2-3 hours + +### Long-term Actions (Future) + +5. **🟢 Install and configure pytest-cov** + - Get actual coverage percentage + - Set coverage thresholds in CI + +6. **🟢 Reorganize tests into submodules** + - Split test_govmap_client.py into focused test files + - Better test organization and maintainability + +7. **🟢 Add integration test suite** + - Mark with @pytest.mark.integration + - Test full workflows with real API + +## Conclusion + +The refactored code has **good test coverage** overall: +- ✅ 34 tests all passing +- ✅ Core functionality well-tested through integration tests +- ✅ E2E tests show tools working correctly +- ⚠️ One bug found and documented (`autocomplete_address`) +- 📋 Some unit tests missing for edge cases + +**Recommended Next Steps:** +1. Fix the `autocomplete_address` bug (10 min) +2. Add the fix to the PR before merging +3. Create follow-up issues for missing unit tests +4. Consider adding pytest-cov for coverage metrics + +**Overall Assessment:** The Phase 3 refactoring maintains code quality and test coverage. The modular structure will make it easier to add targeted unit tests in the future. diff --git a/nadlan_mcp/fastmcp_server.py b/nadlan_mcp/fastmcp_server.py index a620565..3c242e4 100644 --- a/nadlan_mcp/fastmcp_server.py +++ b/nadlan_mcp/fastmcp_server.py @@ -63,13 +63,29 @@ def autocomplete_address(search_text: str) -> str: # Format results for better readability formatted_results = [] for result in response['results']: + # Parse coordinates from WKT POINT format: "POINT(longitude latitude)" + shape_str = result.get("shape", "") + coordinates = {} + if shape_str and shape_str.startswith("POINT("): + try: + coords_str = shape_str[6:-1] # Remove "POINT(" and ")" + coords = coords_str.split() + if len(coords) == 2: + coordinates = { + "longitude": float(coords[0]), + "latitude": float(coords[1]) + } + except (ValueError, IndexError) as e: + logger.warning(f"Failed to parse coordinates from shape: {shape_str}, error: {e}") + formatted_results.append({ - "address": result.get("addressLabel", ""), - "settlement": result.get("settlementNameHeb", ""), - "coordinates": result.get("coordinates", {}), - "polygon_id": result.get("polygon_id") + "text": result.get("text", ""), + "id": result.get("id", ""), + "type": result.get("type", ""), + "score": result.get("score", 0), + "coordinates": coordinates }) - + return json.dumps(formatted_results, ensure_ascii=False, indent=2) except Exception as e: