-
Notifications
You must be signed in to change notification settings - Fork 2
Fix Test Setup and DI Container Issues in Test Suite #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
bashandbone
merged 10 commits into
feat/di_monorepo
from
fix-test-triage-and-issues-4039835176754165263
Mar 10, 2026
Merged
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
aa787e8
Fix initial test setup, DI initialization, and un-awaited fixtures to…
google-labs-jules[bot] 3b66a67
Fix gemini CI action check failure when secrets are not present
google-labs-jules[bot] 5c71660
Fix mock types, container teardown, and doc accuracy in config valida…
Copilot 80073c4
No changes made in response to copilot comment
google-labs-jules[bot] 3a7b5bf
Fix DI container override callable and private API access in config v…
Copilot 5f80c75
Apply suggestions from code review
bashandbone 44947bd
Sign CLA
google-labs-jules[bot] 7d490b8
Fix gemini dispatch action missing secrets error
google-labs-jules[bot] 16f866a
Fix mock_checkpoint_manager: use Mock(spec=CheckpointManager) with As…
Copilot 1f2d103
Merge branch 'feat/di_monorepo' into fix-test-triage-and-issues-40398…
bashandbone File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| <!-- | ||
| SPDX-FileCopyrightText: 2025 Knitli Inc. | ||
|
|
||
| SPDX-License-Identifier: MIT OR Apache-2.0 | ||
| --> | ||
|
|
||
| # Remaining Test Issues & Proposed Solutions | ||
|
|
||
| This document outlines the current state of the test suite after initial fixes, including specific issues that are blocking tests from passing and proposed solutions for each. | ||
|
|
||
| ## 1. The `AsyncMock` Attribute Issue in `test_config_validation_flow.py` | ||
|
|
||
| **Issue:** | ||
| Tests in `test_config_validation_flow.py` were failing with: | ||
| `AttributeError: 'coroutine' object has no attribute 'is_compatible_with'` or `RuntimeWarning: coroutine 'AsyncMockMixin._execute_mock_call' was never awaited`. | ||
|
|
||
| **Root Cause:** | ||
| `mock_checkpoint_manager` was an `AsyncMock()`. Python's `AsyncMock` recursively returns new `AsyncMock` instances for any unconfigured method calls. | ||
| When `ConfigChangeAnalyzer` calls `self.checkpoint_manager._extract_fingerprint()`, it gets an `AsyncMock` back (which acts as a coroutine), rather than a `CheckpointSettingsFingerprint` or a standard `Mock`. Later, when it attempts to call `.is_compatible_with()` on this returned object, it throws an `AttributeError` because the returned object is a coroutine, not a fingerprint instance. | ||
|
|
||
| **Resolution (applied):** | ||
| Changed `mock_checkpoint_manager` base type from `AsyncMock()` to `Mock()`, with async methods (`load`, `validate_checkpoint_compatibility`) explicitly configured as `AsyncMock`. This ensures synchronous methods like `_extract_fingerprint` return regular `Mock` objects. | ||
|
|
||
| ## 2. DI Container and `actual_vector_store` Warnings | ||
|
|
||
| **Issue:** | ||
| There are several `RuntimeWarning: coroutine 'actual_vector_store' was never awaited` warnings in pytest output. | ||
|
|
||
| **Root Cause:** | ||
| The `actual_vector_store` fixture is defined as an `async def` but is likely being injected into synchronous tests or other synchronous fixtures without being awaited properly, or the test runner isn't fully recognizing the fixture dependencies correctly due to a mix of pytest-asyncio versions or missing `@pytest.mark.asyncio` markers on the tests that consume it indirectly. | ||
|
|
||
| **Proposed Solutions:** | ||
| 1. Ensure that any test or fixture consuming `actual_vector_store` is an `async` function and properly marked with `@pytest.mark.asyncio`. | ||
| 2. Evaluate if `actual_vector_store` genuinely needs to be an async fixture. If it only creates a synchronous object (like an in-memory dictionary), it can be converted to a normal `def` fixture. | ||
|
|
||
| ## 3. General Pytest Version Compatibility | ||
|
|
||
| **Issue:** | ||
| During the run, there were warnings related to `PytestRemovedIn9Warning`, and the environment required explicitly pinning `pytest-asyncio` to avoid crashes. | ||
|
|
||
| **Proposed Solutions:** | ||
| 1. **Standardize Pytest Async Markers:** Ensure all asynchronous tests are consistently decorated with `@pytest.mark.asyncio` (not `@pytest.mark.async_test` which is a custom or typoed mark). | ||
| 2. **Async Fixture Decoration:** This repo sets `asyncio_mode = "auto"` in `pyproject.toml`, which means plain `@pytest.fixture` works for `async def` fixtures — they do **not** need to be re-decorated with `@pytest_asyncio.fixture`. The `asyncio_mode = "auto"` setting handles them automatically. | ||
|
|
||
bashandbone marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ## 4. Unimplemented and Skipped Tests | ||
|
|
||
| **Issue:** | ||
| A large number of tests (around 95) are skipped due to missing infrastructure (e.g., Docker Qdrant instances) or pending DI integration implementations. | ||
|
|
||
| **Proposed Solutions:** | ||
| 1. **CI Infrastructure:** For tests requiring Qdrant or real external APIs, either use mock containers (like testcontainers) during CI or ensure the environment variables/services are spun up correctly before running the suite. | ||
| 2. **DI Updates:** Complete the integration of the new Dependency Injection system into the legacy tests that are marked with "DI integration not yet implemented". Note the distinction between two different settings paths: | ||
| - **DI-resolved services**: use `container.override(CodeWeaverSettingsType, test_settings)` to inject test settings. | ||
| - **Direct `get_settings()` calls**: `codeweaver.core.get_settings()` constructs settings from the installed package environment and does *not* consult the container. Code paths that call it directly (e.g. `CheckpointManager._extract_fingerprint`, `_create_fingerprint`) must be patched at the call site — either via `unittest.mock.patch("codeweaver.engine.managers.checkpoint_manager.get_settings", ...)` or by explicitly mocking the methods themselves (as done in the `mock_checkpoint_manager` fixture). | ||
|
|
||
| ## 5. `CheckpointManager` and `get_settings()` — Important Note | ||
|
|
||
| `CheckpointManager` calls `get_settings()` directly (imported from `codeweaver.core`). This function constructs a new settings instance by detecting the installed package; it does **not** read from the DI container. Therefore, `container.override(CodeWeaverSettingsType, test_settings)` alone will **not** influence what `_extract_fingerprint()` or `_create_fingerprint()` receive when they call `get_settings()` internally. | ||
|
|
||
| **Resolution (applied):** The `mock_checkpoint_manager` fixture now explicitly mocks `_extract_fingerprint` and `_create_fingerprint` with `side_effect` functions that return real `CheckpointSettingsFingerprint` instances built from the mock checkpoint/config data. This avoids the need to either patch `get_settings` or instantiate a real `CheckpointManager` in tests. | ||
|
|
||
| For future tests that need to exercise the *real* fingerprint extraction logic, patch `get_settings` at its call site: | ||
|
|
||
| ```python | ||
| from unittest.mock import patch | ||
|
|
||
| with patch("codeweaver.engine.managers.checkpoint_manager.get_settings", return_value=test_settings): | ||
| result = checkpoint_manager._extract_fingerprint(checkpoint) | ||
| ``` | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.