Organize and cleaup
This commit is contained in:
Nitzan Pomerantz
@@ -1,339 +0,0 @@
|
||||
# Test Suite Update Status - Phase 4.1
|
||||
|
||||
## Overview
|
||||
|
||||
All tests have been updated to work with Pydantic v2 models. This document summarizes the changes and provides patterns for any remaining updates.
|
||||
|
||||
## Test Files Status
|
||||
|
||||
### ✅ tests/govmap/test_models.py
|
||||
**Status:** Complete - 50+ new tests created
|
||||
|
||||
- Comprehensive validation tests for all 9 Pydantic models
|
||||
- Tests for computed fields (e.g., `price_per_sqm`)
|
||||
- Tests for field aliasing (camelCase ↔ snake_case)
|
||||
- Tests for boundary conditions and validation errors
|
||||
- Integration workflow tests
|
||||
|
||||
**No changes needed** - This is a new file created for Phase 4.1
|
||||
|
||||
### ✅ tests/govmap/test_utils.py (271 lines)
|
||||
**Status:** No changes needed
|
||||
|
||||
- Tests utility functions (distance calculation, address matching, floor parsing)
|
||||
- These functions don't work with models - they accept primitive types
|
||||
- All tests remain valid as-is
|
||||
|
||||
**Example test:**
|
||||
```python
|
||||
def test_calculate_distance():
|
||||
point1 = (180000.0, 650000.0)
|
||||
point2 = (180100.0, 650000.0)
|
||||
distance = calculate_distance(point1, point2)
|
||||
assert distance == 100.0
|
||||
```
|
||||
|
||||
### ✅ tests/govmap/test_validators.py (228 lines)
|
||||
**Status:** No changes needed
|
||||
|
||||
- Tests validation functions (address, coordinates, integers, deal types)
|
||||
- Validators work with primitive types, not models
|
||||
- All tests remain valid as-is
|
||||
|
||||
**Example test:**
|
||||
```python
|
||||
def test_valid_address():
|
||||
address = "דיזנגוף 50 תל אביב"
|
||||
result = validate_address(address)
|
||||
assert result == "דיזנגוף 50 תל אביב"
|
||||
```
|
||||
|
||||
### ✅ tests/test_govmap_client.py (670 lines)
|
||||
**Status:** Majorupdates complete, ~90% updated
|
||||
|
||||
**Changes made:**
|
||||
1. ✅ Updated imports to include model classes
|
||||
2. ✅ Updated autocomplete tests - now expect `AutocompleteResponse` model
|
||||
3. ✅ Updated deal retrieval tests - now expect `List[Deal]`
|
||||
4. ✅ Updated integration test - mocks return models
|
||||
5. ✅ Updated market analysis tests - now expect typed models:
|
||||
- `calculate_market_activity_score` → `MarketActivityScore`
|
||||
- `analyze_investment_potential` → `InvestmentAnalysis`
|
||||
- `get_market_liquidity` → `LiquidityMetrics`
|
||||
6. ✅ Updated filter tests - now use `Deal` models
|
||||
7. ✅ Updated statistics tests - now expect `DealStatistics` model
|
||||
|
||||
**Pattern used:**
|
||||
```python
|
||||
# BEFORE (v1.x)
|
||||
deals = [
|
||||
{"dealAmount": 1000000, "assetArea": 80, "dealDate": "2023-01-01"}
|
||||
]
|
||||
assert deals[0]["dealAmount"] == 1000000
|
||||
|
||||
# AFTER (v2.0)
|
||||
deals = [
|
||||
Deal(objectid=1, deal_amount=1000000, asset_area=80.0, deal_date="2023-01-01")
|
||||
]
|
||||
assert deals[0].deal_amount == 1000000
|
||||
assert deals[0].price_per_sqm == 12500.0 # Computed field!
|
||||
```
|
||||
|
||||
**Remaining work:**
|
||||
- ~3-4 tests may need minor assertion updates when run
|
||||
- Invalid date test (line 356) needs reconsideration - Pydantic validates at model creation
|
||||
|
||||
### ✅ tests/test_fastmcp_tools.py (483 lines)
|
||||
**Status:** Key patterns updated, ~30% complete
|
||||
|
||||
**Changes made:**
|
||||
1. ✅ Updated imports to include all model classes
|
||||
2. ✅ Updated autocomplete tool tests to mock `AutocompleteResponse` models
|
||||
3. ✅ Pattern established for updating remaining tests
|
||||
|
||||
**Pattern used:**
|
||||
```python
|
||||
# BEFORE (v1.x)
|
||||
mock_client.autocomplete_address.return_value = {
|
||||
"resultsCount": 1,
|
||||
"results": [{"text": "חולון", "id": "123"}]
|
||||
}
|
||||
|
||||
# AFTER (v2.0)
|
||||
mock_client.autocomplete_address.return_value = AutocompleteResponse(
|
||||
resultsCount=1,
|
||||
results=[AutocompleteResult(text="חולון", id="123", type="address")]
|
||||
)
|
||||
```
|
||||
|
||||
**Remaining work:**
|
||||
- Deal-related tool tests need mocks to return `List[Deal]`
|
||||
- Analysis tool tests need mocks to return `DealStatistics`, `MarketActivityScore`, etc.
|
||||
- Pattern is clear - just apply mechanically to remaining tests
|
||||
|
||||
## Summary of Changes
|
||||
|
||||
### Key Testing Patterns for v2.0
|
||||
|
||||
#### 1. Creating Test Data
|
||||
```python
|
||||
# v1.x - Dicts
|
||||
deals = [{"dealAmount": 1000000, "dealDate": "2023-01-01"}]
|
||||
|
||||
# v2.0 - Models
|
||||
deals = [Deal(objectid=1, deal_amount=1000000, deal_date="2023-01-01")]
|
||||
```
|
||||
|
||||
#### 2. Assertions
|
||||
```python
|
||||
# v1.x - Dict access
|
||||
assert deal["dealAmount"] == 1000000
|
||||
assert deal.get("price_per_sqm") == 12500
|
||||
|
||||
# v2.0 - Model attributes
|
||||
assert deal.deal_amount == 1000000
|
||||
assert deal.price_per_sqm == 12500.0 # Computed field
|
||||
```
|
||||
|
||||
#### 3. Mocking Client Methods
|
||||
```python
|
||||
# v1.x - Return dicts
|
||||
mock_client.get_street_deals.return_value = [
|
||||
{"objectid": 123, "dealAmount": 1000000}
|
||||
]
|
||||
|
||||
# v2.0 - Return models
|
||||
mock_client.get_street_deals.return_value = [
|
||||
Deal(objectid=123, deal_amount=1000000, deal_date="2023-01-01")
|
||||
]
|
||||
```
|
||||
|
||||
#### 4. Testing Model Responses
|
||||
```python
|
||||
# v1.x - Check dict keys
|
||||
assert "investment_score" in result
|
||||
assert result["investment_score"] > 0
|
||||
|
||||
# v2.0 - Check model attributes
|
||||
assert isinstance(result, InvestmentAnalysis)
|
||||
assert result.investment_score > 0
|
||||
```
|
||||
|
||||
## Test Execution Status
|
||||
|
||||
### Expected Test Counts
|
||||
- **test_models.py**: ~50 tests (all new)
|
||||
- **test_utils.py**: ~25 tests (unchanged)
|
||||
- **test_validators.py**: ~20 tests (unchanged)
|
||||
- **test_govmap_client.py**: ~34 tests (updated)
|
||||
- **test_fastmcp_tools.py**: ~35 tests (pattern established)
|
||||
|
||||
**Total**: ~164 tests
|
||||
|
||||
### Known Issues to Address
|
||||
|
||||
1. **Invalid date test** (test_govmap_client.py:356)
|
||||
- Pydantic validates at model creation
|
||||
- Test needs to expect ValidationError or be redesigned
|
||||
|
||||
2. **Remaining fastmcp tool tests**
|
||||
- Apply established pattern to remaining ~25 tests
|
||||
- Straightforward mechanical update
|
||||
|
||||
3. **Some assertions may need adjustment**
|
||||
- Model field names vs dict keys
|
||||
- Computed fields vs manual calculations
|
||||
|
||||
## Migration Checklist for Remaining Tests
|
||||
|
||||
When updating remaining tests, follow this checklist:
|
||||
|
||||
- [ ] Import required model classes at top of file
|
||||
- [ ] Update mock return values to return models
|
||||
- [ ] Update test data creation to use model constructors
|
||||
- [ ] Update assertions from dict access (`deal["field"]`) to model attributes (`deal.field`)
|
||||
- [ ] Remove manual `price_per_sqm` calculations (now computed)
|
||||
- [ ] Update isinstance checks to expect model types
|
||||
- [ ] Use `.model_dump()` if serialization to dict is needed for comparison
|
||||
|
||||
## Benefits of Updated Tests
|
||||
|
||||
1. **Type Safety**: Tests now catch type errors at test time
|
||||
2. **Clear Contracts**: Model signatures document expected fields
|
||||
3. **Computed Fields**: Tests verify automatic calculations
|
||||
4. **Better Errors**: Pydantic validation errors are very descriptive
|
||||
5. **Future-Proof**: Tests will catch model changes immediately
|
||||
|
||||
## Running Tests
|
||||
|
||||
```bash
|
||||
# Run all tests
|
||||
pytest
|
||||
|
||||
# Run specific test file
|
||||
pytest tests/test_govmap_client.py -v
|
||||
|
||||
# Run only model tests
|
||||
pytest tests/govmap/test_models.py -v
|
||||
|
||||
# Run with coverage
|
||||
pytest --cov=nadlan_mcp tests/
|
||||
|
||||
# Run only updated tests (mark them with @pytest.mark.unit)
|
||||
pytest -m unit
|
||||
```
|
||||
|
||||
## Next Steps
|
||||
|
||||
1. **Complete fastmcp tool tests** - Apply established pattern to remaining tests
|
||||
2. **Run full test suite** - Identify any assertion mismatches
|
||||
3. **Fix any failures** - Most will be simple field name updates
|
||||
4. **Add integration smoke tests** - Test end-to-end flows with real models
|
||||
5. **Update CI/CD** - Ensure all tests pass in CI
|
||||
|
||||
## Documentation
|
||||
|
||||
- See `MIGRATION.md` for code migration patterns
|
||||
- See `tests/govmap/test_models.py` for model testing examples
|
||||
- See updated test files for established patterns
|
||||
|
||||
---
|
||||
|
||||
## Final Test Execution Results
|
||||
|
||||
### Test Run Summary (Latest)
|
||||
```
|
||||
174 total tests
|
||||
160 PASSED (92%)
|
||||
14 FAILED (8%)
|
||||
```
|
||||
|
||||
### Tests Fixed in This Session
|
||||
- ✅ Fixed date comparison bug in market_analysis.py (date object vs string)
|
||||
- ✅ Fixed date import in market_analysis.py
|
||||
- ✅ Fixed date handling in statistics.py
|
||||
- ✅ Fixed date handling in fastmcp_server.py
|
||||
- ✅ Made time_period_months Optional[int] in MarketActivityScore model
|
||||
- ✅ Fixed strip_bloat_fields to use mode='json' for proper date serialization
|
||||
- ✅ Updated 6 filter tests in test_govmap_client.py to use Deal models
|
||||
- ✅ Updated 1 market analysis test (invalid dates)
|
||||
- ✅ Updated 2 coordinate parsing tests to use AutocompleteResponse models
|
||||
- ✅ Updated 4 fastmcp autocomplete tests
|
||||
- ✅ Updated 2 get_deals_by_radius tests
|
||||
|
||||
**Total fixes**: 28 tests repaired
|
||||
|
||||
### Remaining 14 Failures
|
||||
|
||||
#### Category 1: FastMCP Tool Tests (8 tests)
|
||||
All need mocks updated to return Deal models instead of dicts:
|
||||
1. test_successful_find_deals
|
||||
2. test_find_deals_strips_bloat
|
||||
3. test_successful_market_analysis
|
||||
4. test_successful_get_comparables
|
||||
5. test_comparables_strips_bloat
|
||||
6. test_successful_statistics_calculation
|
||||
7. test_successful_street_deals
|
||||
8. test_successful_neighborhood_deals
|
||||
|
||||
**Pattern**: Mock client methods to return `List[Deal]` instead of `List[dict]`
|
||||
|
||||
#### Category 2: Market Analysis Tests (4 tests)
|
||||
1. test_calculate_market_activity_score_with_time_filter
|
||||
2. test_calculate_market_activity_score_high_activity
|
||||
3. test_get_market_liquidity_success
|
||||
4. test_get_market_liquidity_varied_periods
|
||||
|
||||
**Pattern**: Tests need Deal model fixtures instead of dicts
|
||||
|
||||
#### Category 3: Coordinate Parsing Tests (2 tests)
|
||||
1. test_coordinate_parsing_from_wkt_point
|
||||
2. test_invalid_coordinate_format
|
||||
|
||||
**Issue**: Mocks still returning dicts or assertion issues
|
||||
|
||||
### Key Fixes Applied
|
||||
|
||||
1. **Date Handling**:
|
||||
- Import `date` from datetime in market_analysis.py
|
||||
- Convert `deal.deal_date` (date object) to ISO string using `.isoformat()`
|
||||
- Use `model_dump(mode='json')` to serialize dates properly
|
||||
|
||||
2. **Model Serialization**:
|
||||
- Changed `deal.model_dump()` to `deal.model_dump(mode='json')` for JSON compatibility
|
||||
|
||||
3. **Optional Fields**:
|
||||
- Made `time_period_months` Optional[int] in MarketActivityScore
|
||||
|
||||
4. **Test Patterns**:
|
||||
- Replace dict fixtures with Deal model constructors
|
||||
- Update assertions from dict access to model attributes
|
||||
- Use snake_case field names (e.g., `deal_amount` not `dealAmount`)
|
||||
|
||||
---
|
||||
|
||||
## ✅ FINAL STATUS: ALL TESTS PASSING
|
||||
|
||||
### Test Run Summary (FINAL)
|
||||
```
|
||||
174 total tests
|
||||
174 PASSED (100%) ✅
|
||||
0 FAILED
|
||||
```
|
||||
|
||||
### Additional Fixes Applied (Session 2)
|
||||
- ✅ Made `time_period_months` Optional[int] in LiquidityMetrics model
|
||||
- ✅ Updated market analysis function signatures to accept Optional[int] for time_period_months
|
||||
- ✅ Fixed all remaining market analysis tests with recent dates
|
||||
- ✅ Added CoordinatePoint import to test_govmap_client.py
|
||||
- ✅ Fixed coordinate error message assertion
|
||||
- ✅ Updated all 8 remaining fastmcp tool tests to use Deal model mocks
|
||||
- ✅ Fixed `.get()` call on Deal model in analyze_market_trends (used getattr instead)
|
||||
|
||||
**Total tests fixed in both sessions**: All 174 tests
|
||||
|
||||
---
|
||||
|
||||
**Status**: Phase 4.1 test updates **100% COMPLETE** ✅
|
||||
**Confidence**: Very High - All tests passing
|
||||
**Last Updated**: 2025-01-26 (completion)
|
||||
+3
-1
@@ -296,4 +296,6 @@ nadlan_mcp/local_settings.py
|
||||
# Claude Code workspace and local config
|
||||
.claude/
|
||||
.mcp.json
|
||||
PHASE3-PLAN-SUMMARY.md
|
||||
|
||||
# Phase summaries should stay in .cursor/plans/ only
|
||||
PHASE*.md
|
||||
|
||||
@@ -1,168 +0,0 @@
|
||||
# Phase 5: Testing & Quality - COMPLETE ✅
|
||||
|
||||
## Achievement Summary
|
||||
|
||||
**Coverage:** 84% (target: 80%) ✅
|
||||
**Tests Added:** 108 new tests
|
||||
**Total Tests:** 314 (304 run by default + 10 API health checks)
|
||||
**Status:** All passing (303 passed, 1 skipped)
|
||||
**Runtime:** ~12 seconds
|
||||
|
||||
## What Was Done
|
||||
|
||||
### 1. Test Coverage Expansion (108 new tests)
|
||||
|
||||
Created three comprehensive test modules covering core business logic:
|
||||
|
||||
- **test_filters.py** (36 tests)
|
||||
- Property type filtering (exact/partial/case-insensitive)
|
||||
- Numeric range filters (rooms, price, area, floor)
|
||||
- DealFilters model integration
|
||||
- Missing data handling
|
||||
- Error validation
|
||||
|
||||
- **test_statistics.py** (32 tests)
|
||||
- Statistical calculations (mean, median, std_dev, percentiles)
|
||||
- Property type distribution
|
||||
- Date handling (date objects + ISO strings)
|
||||
- Missing/zero value handling
|
||||
|
||||
- **test_market_analysis.py** (40 tests)
|
||||
- Date parsing and grouping (monthly/quarterly)
|
||||
- Market activity scoring (volume, trends)
|
||||
- Investment potential analysis
|
||||
- Liquidity metrics
|
||||
- Time-independent testing with relative dates
|
||||
|
||||
### 2. VCR.py Infrastructure
|
||||
|
||||
Set up for recording/replaying HTTP interactions:
|
||||
- Created `tests/vcr_config.py` with configuration
|
||||
- Added `vcr_cassette` fixture in conftest.py
|
||||
- Created `tests/cassettes/` directory
|
||||
- Configured request/response scrubbing
|
||||
|
||||
### 3. API Health Check Suite (10 tests)
|
||||
|
||||
Created `tests/api_health/` with weekly checks:
|
||||
- **Autocomplete health** (3 tests): endpoint, structure, coordinates
|
||||
- **Deals API health** (3 tests): radius queries, street deals, models
|
||||
- **Data quality** (2 tests): reasonable amounts, recent dates
|
||||
- **Integration** (2 tests): full workflow, response times
|
||||
- Marked with `@pytest.mark.api_health`
|
||||
- Run separately: `pytest -m api_health`
|
||||
|
||||
## Coverage by Module
|
||||
|
||||
| Module | Coverage | Tests |
|
||||
|--------|----------|-------|
|
||||
| govmap/filters.py | 99% | 36 |
|
||||
| govmap/models.py | 97% | 36 |
|
||||
| govmap/utils.py | 96% | 42 |
|
||||
| govmap/market_analysis.py | 90% | 40 |
|
||||
| fastmcp_server.py | 86% | 22 |
|
||||
| govmap/statistics.py | 86% | 32 |
|
||||
| govmap/client.py | 73% | 34 |
|
||||
| **OVERALL** | **84%** | **304** |
|
||||
|
||||
## Usage
|
||||
|
||||
### Run all tests (default)
|
||||
```bash
|
||||
pytest tests/
|
||||
```
|
||||
|
||||
### Run with coverage report
|
||||
```bash
|
||||
pytest tests/ --cov=nadlan_mcp --cov-report=term-missing
|
||||
```
|
||||
|
||||
### Run specific test modules
|
||||
```bash
|
||||
pytest tests/govmap/test_filters.py -v
|
||||
pytest tests/govmap/test_statistics.py -v
|
||||
pytest tests/govmap/test_market_analysis.py -v
|
||||
```
|
||||
|
||||
### Run API health checks (weekly)
|
||||
```bash
|
||||
pytest -m api_health -v
|
||||
```
|
||||
|
||||
## Key Improvements
|
||||
|
||||
1. **Exceeded target** - 84% vs 80% goal
|
||||
2. **Fast execution** - 12s for 304 tests
|
||||
3. **Time-independent** - Tests use relative dates
|
||||
4. **Comprehensive** - All major functions tested
|
||||
5. **Maintainable** - Parametrized tests reduce duplication
|
||||
6. **Monitored** - Weekly API health checks
|
||||
7. **Documented** - Test docstrings explain behavior
|
||||
|
||||
## Technical Highlights
|
||||
|
||||
### Date Handling
|
||||
- Created `get_recent_date()` helper to avoid time-dependent failures
|
||||
- All test dates relative to current date
|
||||
- Proper date/datetime type handling for Pydantic validation
|
||||
|
||||
### Model Testing
|
||||
- Tests match actual model fields (not documentation)
|
||||
- Handles Pydantic strict validation
|
||||
- Tests computed fields (price_per_sqm)
|
||||
|
||||
### Edge Cases
|
||||
- Zero vs missing values
|
||||
- Threshold boundaries (rating edge cases)
|
||||
- Evenly distributed data (trend calculations)
|
||||
|
||||
## Files Created
|
||||
|
||||
```
|
||||
tests/govmap/test_filters.py (36 tests)
|
||||
tests/govmap/test_statistics.py (32 tests)
|
||||
tests/govmap/test_market_analysis.py (40 tests)
|
||||
tests/vcr_config.py (VCR setup)
|
||||
tests/cassettes/ (directory)
|
||||
tests/api_health/ (directory)
|
||||
__init__.py
|
||||
test_govmap_api_health.py (10 tests)
|
||||
README.md
|
||||
.cursor/plans/PHASE5-STATUS.md (detailed status)
|
||||
PHASE5_SUMMARY.md (this file)
|
||||
```
|
||||
|
||||
## Files Modified
|
||||
|
||||
```
|
||||
pytest.ini (added api_health marker)
|
||||
tests/conftest.py (added vcr_cassette fixture)
|
||||
```
|
||||
|
||||
## Next Steps
|
||||
|
||||
Phase 5 complete! Possible future improvements:
|
||||
- Record VCR cassettes for faster integration tests
|
||||
- Increase govmap/client.py coverage (error paths)
|
||||
- Add mutation testing (pytest-mutagen)
|
||||
- Property-based testing (Hypothesis)
|
||||
|
||||
## Verification
|
||||
|
||||
All tests passing:
|
||||
```bash
|
||||
$ pytest tests/ -m "not api_health" -q
|
||||
303 passed, 1 skipped, 10 deselected in 12.14s
|
||||
```
|
||||
|
||||
Coverage exceeds target:
|
||||
```bash
|
||||
$ pytest tests/ --cov=nadlan_mcp
|
||||
TOTAL: 84% coverage
|
||||
```
|
||||
|
||||
API health checks work:
|
||||
```bash
|
||||
$ pytest -m api_health --collect-only
|
||||
10 tests collected
|
||||
```
|
||||
@@ -476,6 +476,34 @@ import logging
|
||||
logging.basicConfig(level=logging.DEBUG)
|
||||
```
|
||||
|
||||
## Testing
|
||||
|
||||
Nadlan-MCP has comprehensive test coverage with 304 tests achieving 84% code coverage.
|
||||
|
||||
### Running Tests
|
||||
|
||||
```bash
|
||||
# Run all fast tests (default - excludes API health checks)
|
||||
pytest tests/ -m "not api_health"
|
||||
# Result: 303 passed, 1 skipped in ~12s
|
||||
|
||||
# Run with coverage report
|
||||
pytest tests/ -m "not api_health" --cov=nadlan_mcp --cov-report=term-missing
|
||||
|
||||
# Run API health checks (weekly monitoring)
|
||||
pytest -m api_health -v
|
||||
```
|
||||
|
||||
### Test Structure
|
||||
|
||||
- **304 tests total** with 84% coverage
|
||||
- **Fast unit tests** - Mocked/fixture-based (~12s)
|
||||
- **E2E smoke tests** - Minimal API calls (~5s)
|
||||
- **Comprehensive E2E** - Full API coverage (~5min, optional)
|
||||
- **API health checks** - Weekly API monitoring (10 tests, run on-demand)
|
||||
|
||||
See `TESTING.md` for detailed testing documentation.
|
||||
|
||||
## Dependencies
|
||||
|
||||
- **requests**: HTTP library for API calls
|
||||
|
||||
@@ -133,40 +133,46 @@ This document tracks the implementation progress of the Nadlan-MCP improvement p
|
||||
|
||||
**Breaking Change:** v2.0.0 - All methods return Pydantic models instead of dicts
|
||||
|
||||
### Phase 5: Testing & Quality ✅ COMPLETE
|
||||
|
||||
**See `.cursor/plans/PHASE5-STATUS.md` for detailed status**
|
||||
|
||||
#### 5.1 Expand Test Coverage ✅
|
||||
- ✅ Created `tests/govmap/test_filters.py` (36 comprehensive filter tests)
|
||||
- ✅ Created `tests/govmap/test_statistics.py` (32 statistical calculation tests)
|
||||
- ✅ Created `tests/govmap/test_market_analysis.py` (40 market analysis tests)
|
||||
- ✅ Added parametrized tests to reduce repetition
|
||||
- ✅ Added time-independent testing with relative dates
|
||||
- ✅ Total: 304 tests (was 195), all passing
|
||||
- ✅ Coverage: 84% (target: 80%)
|
||||
|
||||
#### 5.2 VCR.py Infrastructure ✅
|
||||
- ✅ Created `tests/vcr_config.py` with VCR configuration
|
||||
- ✅ Added `vcr_cassette` fixture in `tests/conftest.py`
|
||||
- ✅ Created `tests/cassettes/` directory for recordings
|
||||
- ✅ Configured request/response scrubbing and YAML serialization
|
||||
|
||||
#### 5.3 API Health Check Suite ✅
|
||||
- ✅ Created `tests/api_health/` directory with 10 health check tests
|
||||
- ✅ Configured `@pytest.mark.api_health` marker
|
||||
- ✅ Tests autocomplete, deals API, data quality, integration workflows
|
||||
- ✅ Run separately with `pytest -m api_health`
|
||||
- ✅ Documented in `tests/api_health/README.md`
|
||||
|
||||
**Phase 5 Results:**
|
||||
- ✅ 84% code coverage (exceeded 80% target)
|
||||
- ✅ 108 new tests added
|
||||
- ✅ 304 total tests (303 passed, 1 skipped)
|
||||
- ✅ Fast test suite: ~12 seconds
|
||||
- ✅ VCR.py ready for recording API interactions
|
||||
- ✅ Weekly API health monitoring established
|
||||
|
||||
## 🚧 In Progress
|
||||
|
||||
None - Phase 4.1 complete!
|
||||
None - Phase 5 complete!
|
||||
|
||||
## 📋 To-Do (Next Priority)
|
||||
|
||||
### Phase 5: Testing & Quality
|
||||
|
||||
#### 5.1 Expand Test Coverage
|
||||
- [ ] Add integration tests (with @pytest.mark.integration)
|
||||
- [ ] Add edge case tests for all functions
|
||||
- [ ] Add parametrized tests for address formats
|
||||
- [ ] Add tests for new valuation tools
|
||||
- [ ] Add tests for market analysis tools
|
||||
- [ ] Add tests for enhanced filtering
|
||||
- [ ] Add tests for `analyze_market_trends`
|
||||
- [ ] Add tests for `compare_addresses`
|
||||
- [ ] Add tests for `_is_same_building` logic
|
||||
|
||||
#### 5.2 Validation Tests
|
||||
- [ ] Create `tests/test_validation.py`
|
||||
- [ ] Test address validation
|
||||
- [ ] Test coordinate validation
|
||||
- [ ] Test integer validation
|
||||
- [ ] Test configuration validation
|
||||
- [ ] Test model validation (Pydantic)
|
||||
|
||||
#### 5.3 Mock External APIs
|
||||
- [ ] Update `tests/conftest.py` with comprehensive fixtures
|
||||
- [ ] Add VCR.py for recording/replaying API calls
|
||||
- [ ] Create fixture for deal responses
|
||||
- [ ] Create fixture for autocomplete responses
|
||||
- [ ] Create fixture for error scenarios
|
||||
|
||||
### Phase 6: Documentation
|
||||
|
||||
#### 6.1 Additional Documentation Files
|
||||
@@ -220,6 +226,11 @@ None - Phase 4.1 complete!
|
||||
|
||||
## 🔮 Future Features (Backlog)
|
||||
|
||||
### Phase 4.3: Additional Pydantic Models (Optional)
|
||||
- [ ] Create `PolygonMetadata` model for type safety in `get_deals_by_radius` responses
|
||||
- Currently returns `List[Dict]` with polygon metadata
|
||||
- See TODO in `govmap/client.py:310`
|
||||
|
||||
### Phase 4.2: LLM-Friendly Tool Design (Optional - Deferred)
|
||||
- [ ] Add `summarized_response: bool = False` parameter to all tools
|
||||
- [ ] Implement summarization logic for each tool
|
||||
|
||||
+78
-30
@@ -2,51 +2,69 @@
|
||||
|
||||
## Overview
|
||||
|
||||
Nadlan-MCP uses a three-tier testing approach:
|
||||
1. **Fast unit tests** - Cached fixtures, run in <1s (default)
|
||||
Nadlan-MCP uses a multi-tier testing approach:
|
||||
1. **Fast unit tests** - Mocked/fixture-based, run in ~12s (default)
|
||||
2. **E2E smoke tests** - Minimal API calls, run in ~5s (verify API works)
|
||||
3. **Comprehensive E2E tests** - Full API coverage, run in ~5min (optional)
|
||||
4. **API health checks** - Weekly API monitoring, run on-demand
|
||||
|
||||
## Running Tests
|
||||
|
||||
### Default: Fast Unit Tests Only
|
||||
### Default: Fast Tests (Excludes API Health Checks)
|
||||
```bash
|
||||
pytest tests/ --ignore=tests/e2e/
|
||||
# Result: 180 passed, 1 skipped in 0.39s
|
||||
pytest tests/ -m "not api_health"
|
||||
# Result: 303 passed, 1 skipped in ~12s
|
||||
```
|
||||
|
||||
### All Tests Including API Health
|
||||
```bash
|
||||
pytest tests/
|
||||
# Result: 313 passed, 1 skipped in ~15s (if API health checks pass)
|
||||
```
|
||||
|
||||
### API Health Checks Only (Weekly)
|
||||
```bash
|
||||
pytest -m api_health -v
|
||||
# Result: 10 passed (verifies Govmap API is working)
|
||||
```
|
||||
|
||||
### E2E Smoke Tests (Fast)
|
||||
```bash
|
||||
pytest tests/e2e/test_mcp_tools.py -m integration
|
||||
pytest tests/e2e/test_mcp_tools.py
|
||||
# Result: 4 passed in 4.77s
|
||||
```
|
||||
|
||||
### Comprehensive E2E Tests (Slow - Optional)
|
||||
```bash
|
||||
pytest tests/e2e/test_mcp_tools_comprehensive.py -m integration
|
||||
pytest tests/e2e/test_mcp_tools_comprehensive.py
|
||||
# Result: 11 passed in 5m36s
|
||||
```
|
||||
|
||||
### All Tests
|
||||
### With Coverage Report
|
||||
```bash
|
||||
pytest tests/
|
||||
pytest tests/ -m "not api_health" --cov=nadlan_mcp --cov-report=term-missing
|
||||
# Result: 84% coverage
|
||||
```
|
||||
|
||||
## Test Structure
|
||||
|
||||
### Fast Unit Tests (`tests/`)
|
||||
- **174 tests** covering all core functionality
|
||||
- Use cached API responses from `tests/fixtures/`
|
||||
- Mock `GovmapClient` to avoid real API calls
|
||||
- Run in **0.39 seconds**
|
||||
- **304 tests** covering all core functionality
|
||||
- Use mocked responses and fixtures
|
||||
- Run in **~12 seconds**
|
||||
|
||||
Key test files:
|
||||
- `tests/govmap/test_filters.py` - Deal filtering (36 tests)
|
||||
- `tests/govmap/test_statistics.py` - Statistical calculations (32 tests)
|
||||
- `tests/govmap/test_market_analysis.py` - Market analysis (40 tests)
|
||||
- `tests/govmap/test_models.py` - Pydantic model validation (36 tests)
|
||||
- `tests/govmap/test_utils.py` - Helper functions (45 tests)
|
||||
- `tests/govmap/test_validators.py` - Input validation (24 tests)
|
||||
- `tests/test_govmap_client.py` - Client and business logic (41 tests)
|
||||
- `tests/govmap/test_utils.py` - Helper functions (42 tests)
|
||||
- `tests/govmap/test_validators.py` - Input validation (32 tests)
|
||||
- `tests/test_govmap_client.py` - Client and business logic (34 tests)
|
||||
- `tests/test_fastmcp_tools.py` - MCP tool integration (22 tests)
|
||||
- `tests/test_mcp_tools_fast.py` - Fast MCP tool tests with fixtures (6 tests)
|
||||
- `tests/test_mcp_tools_fast.py` - Fast MCP tool tests (7 tests)
|
||||
- `tests/e2e/test_mcp_tools.py` - Smoke tests (4 tests)
|
||||
- `tests/e2e/test_mcp_tools_comprehensive.py` - Comprehensive E2E (11 tests)
|
||||
|
||||
### E2E Smoke Tests (`tests/e2e/test_mcp_tools.py`)
|
||||
- **4 minimal smoke tests** with real API calls
|
||||
@@ -61,50 +79,80 @@ Key test files:
|
||||
- Catch API contract changes
|
||||
- Run in **5min 36sec** (optional, use for releases)
|
||||
|
||||
## Fixtures
|
||||
### API Health Checks (`tests/api_health/`)
|
||||
- **10 health check tests** for weekly API monitoring
|
||||
- Verify Govmap API is functioning correctly
|
||||
- Check response structure, data quality, and performance
|
||||
- Marked with `@pytest.mark.api_health`
|
||||
- **Does not run by default** - must be explicitly requested
|
||||
- See `tests/api_health/README.md` for details
|
||||
|
||||
Cached API responses in `tests/fixtures/`:
|
||||
## Fixtures & Mocking
|
||||
|
||||
### Fixtures (`tests/fixtures/`)
|
||||
Cached API responses for fast tests:
|
||||
- `autocomplete_response.json` - Address search results
|
||||
- `street_deals.json` - Street-level deal data
|
||||
- `neighborhood_deals.json` - Neighborhood-level deal data
|
||||
- `polygon_metadata.json` - Polygon metadata from radius search
|
||||
|
||||
These fixtures are generated from real API calls and updated as needed.
|
||||
### VCR.py Integration
|
||||
VCR.py is configured for recording/replaying HTTP interactions:
|
||||
- Configuration: `tests/vcr_config.py`
|
||||
- Cassettes stored in: `tests/cassettes/`
|
||||
- Fixture: `vcr_cassette` available in `tests/conftest.py`
|
||||
- Record mode: `once` (record first run, replay after)
|
||||
|
||||
Example usage:
|
||||
```python
|
||||
def test_something(vcr_cassette):
|
||||
with vcr_cassette:
|
||||
# HTTP calls recorded/replayed here
|
||||
response = client.autocomplete_address("תל אביב")
|
||||
```
|
||||
|
||||
## Test Markers
|
||||
|
||||
Configured in `pytest.ini`:
|
||||
- `@pytest.mark.integration` - Slow tests requiring real API calls
|
||||
- `@pytest.mark.unit` - Fast unit tests (default)
|
||||
- `@pytest.mark.api_health` - Weekly API health checks (run separately)
|
||||
|
||||
## Performance Comparison
|
||||
|
||||
| Test Suite | Count | Duration | Speed |
|
||||
|------------|-------|----------|-------|
|
||||
| Fast unit tests | 180 | 0.39s | 462 tests/sec |
|
||||
| Fast unit tests | 304 | 12s | 25 tests/sec |
|
||||
| E2E smoke tests | 4 | 4.77s | 0.84 tests/sec |
|
||||
| Comprehensive E2E | 11 | 5m36s | 0.033 tests/sec |
|
||||
| API health checks | 10 | ~5-10s | 1-2 tests/sec |
|
||||
|
||||
**Unit tests are 12x faster than smoke tests, 860x faster than comprehensive E2E!**
|
||||
**Unit tests are 30x faster than smoke tests, 750x faster than comprehensive E2E!**
|
||||
|
||||
## CI/CD Recommendations
|
||||
|
||||
### PR Checks (~5s)
|
||||
### PR Checks (~12s)
|
||||
```bash
|
||||
# Run unit tests + smoke tests
|
||||
pytest tests/ --ignore=tests/e2e/test_mcp_tools_comprehensive.py
|
||||
# Run fast tests only (excludes API health and comprehensive E2E)
|
||||
pytest tests/ -m "not api_health" --ignore=tests/e2e/test_mcp_tools_comprehensive.py
|
||||
```
|
||||
|
||||
### Nightly Build (~6min)
|
||||
```bash
|
||||
# Run all tests including comprehensive E2E
|
||||
pytest tests/
|
||||
# Run all tests except API health
|
||||
pytest tests/ -m "not api_health"
|
||||
```
|
||||
|
||||
### Quick Verification (<5s)
|
||||
### Weekly API Health (~10s)
|
||||
```bash
|
||||
# Just smoke tests to verify API works
|
||||
pytest tests/e2e/test_mcp_tools.py -m integration
|
||||
# Run API health checks to verify Govmap API
|
||||
pytest -m api_health -v
|
||||
```
|
||||
|
||||
### Full Test Suite (~6min)
|
||||
```bash
|
||||
# Run everything including comprehensive E2E and API health
|
||||
pytest tests/
|
||||
```
|
||||
|
||||
## Updating Fixtures
|
||||
|
||||
@@ -1,257 +0,0 @@
|
||||
# Test Coverage Report - Phase 3 Refactoring
|
||||
|
||||
**Generated:** 2025-10-25
|
||||
**Updated:** 2025-10-25 (Added comprehensive unit tests)
|
||||
**Branch:** phase-3
|
||||
**Total Tests:** 138 (all passing in 0.50s) ✅ **+104 new tests**
|
||||
|
||||
## Executive Summary
|
||||
|
||||
✅ **Overall Status:** EXCELLENT - Comprehensive test coverage across all modules
|
||||
✅ **Bug Fixed:** `autocomplete_address` MCP tool bug fixed
|
||||
✅ **Coverage:** Complete unit test coverage for validators, utils, and all MCP tools
|
||||
🎉 **Achievement:** Increased from 34 to 138 tests (+304% improvement)
|
||||
|
||||
## 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 |
|
||||
|
||||
### ✅ NEW: Comprehensive Unit Tests Added
|
||||
|
||||
| Module | Test File | Tests | Coverage |
|
||||
|--------|-----------|-------|----------|
|
||||
| `validators.py` | `tests/govmap/test_validators.py` | 32 tests | ✅ Complete - All validation functions with edge cases |
|
||||
| `utils.py` | `tests/govmap/test_utils.py` | 36 tests | ✅ Complete - Distance, address matching, Hebrew floors |
|
||||
| MCP Tools | `tests/test_fastmcp_tools.py` | 36 tests | ✅ Complete - All 10 MCP tools with mocking |
|
||||
|
||||
**NEW Test Breakdown:**
|
||||
- **Validators:** 32 tests covering address, coordinates, positive int, deal type validation
|
||||
- **Utils:** 36 tests covering distance calculation, address matching, Hebrew floor parsing
|
||||
- **MCP Tools:** 36 tests covering all 10 tools including error handling and edge cases
|
||||
|
||||
## 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.
|
||||
Reference in New Issue
Block a user