Improve test reliability with better mocking strategies

[dougborg]

Problem

We discovered a bug in the supplier directory resource (reports.py:215) where the code called list_suppliers() instead of list_all(). The tests passed because they were mocking list_suppliers() - a method that doesn't exist on the real SupplierService.

This happened because our mocks use MagicMock() and AsyncMock() which accept any attribute access without validation.

Proposed Solutions

Option 1: Use autospec for mocks (Quick Win)

Use create_autospec() to make mocks reject non-existent attributes:

from unittest.mock import create_autospec
from stocktrim_mcp_server.services.suppliers import SupplierService

# In conftest.py or test setup
services.suppliers = create_autospec(SupplierService, instance=True)
services.suppliers.list_all.return_value = suppliers  # ✅ Works
services.suppliers.list_suppliers.return_value = suppliers  # ❌ AttributeError!

Pros:

• Quick to implement
• Low risk - only affects test code
• Immediate feedback on interface mismatches

Cons:

• Still mocking - doesn't test actual service logic
• Requires updating conftest and potentially some tests

Option 2: Real service instances with mocked clients (Higher Value)

Use real service objects with mocked API clients:

@pytest.fixture
def real_services_with_mocked_client(mock_api_client):
    """Use real service instances with mocked StockTrimClient."""
    from stocktrim_mcp_server.context import ServerContext
    from stocktrim_mcp_server.services.products import ProductService
    from stocktrim_mcp_server.services.suppliers import SupplierService
    # ... import other services
    
    return ServerContext(
        client=mock_api_client,
        products=ProductService(mock_api_client),
        suppliers=SupplierService(mock_api_client),
        customers=CustomerService(mock_api_client),
        # ... etc
    )

Pros:

• Tests actual service logic, not mocks
• Catches bugs in service implementations
• Better refactoring safety - can change service internals without breaking tests
• More confidence in integration between layers

Cons:

• More setup required
• Need to ensure all service dependencies are properly mocked
• Slightly slower tests (but probably negligible)

Recommendation

Implement both, in phases:

1. Phase 1 (Quick): Add autospec to existing mocks in conftest.py
2. Phase 2 (Better): Migrate to real service instances with mocked clients

Acceptance Criteria

• Phase 1: Update conftest.py to use create_autospec() for service mocks
• Phase 1: Run full test suite to identify any tests that were masking bugs
• Phase 1: Fix any failing tests
• Phase 2: Create fixture for real services with mocked client
• Phase 2: Migrate resource tests to use real services
• Phase 2: Migrate tool tests to use real services
• Phase 2: Document the testing pattern in contributing guide

Related

• Original bug found while fixing Issue Implement user confirmation/elicitation for data modification operations #80
• This would have caught the list_suppliers() vs list_all() mismatch immediately

[dougborg]

UPDATE: Test Coverage Significantly Improved

Great progress has been made on test reliability since this issue was opened:

✅ Completed Improvements

Comprehensive Test Suite

• 276 MCP server tests (PRs Add unit tests for PurchaseOrderService and SalesOrderService #67, fix: resolve all 40 test failures across service and tool layers #72, test(mcp): add comprehensive tests for structured logging #76, test: add comprehensive test coverage for forecast management tools #77)
• 73 public API client tests
• Full coverage of services and tools
• All tests passing consistently

Better Test Organization

• Service layer tests properly isolated
• Tool tests with service mocks
• Clear test fixtures and reusable components
• Consistent test patterns across codebase

❌ Core Recommendation Still Outstanding

Phase 1: Use for service mocks

• Tests still use MagicMock() without spec validation
• No usage of create_autospec() found in codebase
• This was the original recommendation that would have caught the list_suppliers vs list_all bug

Example of what needs to change:

# Current approach:
mock_client = MagicMock()
mock_client.products = MagicMock()
mock_client.products.get_all = AsyncMock(return_value=[])

# Recommended approach:
from unittest.mock import create_autospec
from stocktrim_public_api_client.helpers.products import Products

mock_products = create_autospec(Products, instance=True)
mock_products.get_all = AsyncMock(return_value=[])
mock_client.products = mock_products

Benefits:

• Catches method name typos at test time
• Validates correct number of arguments
• Ensures interface compatibility
• Would have prevented the original bug

Next Steps

Phase 1 (adding autospec) remains a quick win that would significantly improve test reliability by catching interface mismatches early. The test infrastructure is now solid, making this the perfect time to add this layer of safety.

Phase 2 (real services with mocked clients) can be considered later as an even more comprehensive approach.