Fix Test Setup and DI Container Issues in Test Suite#207
Fix Test Setup and DI Container Issues in Test Suite#207bashandbone merged 10 commits intofeat/di_monorepofrom
Conversation
… unblock tests 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 GuideFixes failing integration tests by wiring a real DI container/settings into the config validation flow tests, tightening mocks to return realistic values, and aligning async test markers/fixtures and documentation with current pytest/pytest-asyncio behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
🤖 Hi @bashandbone, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
![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 left some high level feedback:
- The
setup_test_containerfixture mutates the global container returned byget_container()and then callsclear(), which may inadvertently affect other tests that rely on shared registrations; consider using a dedicated test-only container instance or restoring the original container state instead of clearing the global one. - The guidance in
remaining_test_issues.mdabout switchingmock_checkpoint_managerto a standardMockconflicts with the currentAsyncMockimplementation intest_config_validation_flow.py; it would be clearer to either update the fixture to match the proposed approach or adjust the document to describe the pattern now in use.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `setup_test_container` fixture mutates the global container returned by `get_container()` and then calls `clear()`, which may inadvertently affect other tests that rely on shared registrations; consider using a dedicated test-only container instance or restoring the original container state instead of clearing the global one.
- The guidance in `remaining_test_issues.md` about switching `mock_checkpoint_manager` to a standard `Mock` conflicts with the current `AsyncMock` implementation in `test_config_validation_flow.py`; it would be clearer to either update the fixture to match the proposed approach or adjust the document to describe the pattern now in use.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
🤖 I'm sorry @bashandbone, but I was unable to process your request. Please see the logs for more details. |
There was a problem hiding this comment.
Pull request overview
Updates the integration test setup for config-change validation to work with the DI container and modern pytest-asyncio marking, and adds a running document of remaining test-suite blockers.
Changes:
- Introduces an autouse DI-container fixture that overrides global settings for tests and clears container state afterward.
- Fixes async-test configuration in
test_config_validation_flow.py(marker standardization, fixture sync/async adjustments) and expands mocks (manifest_manager.load,vector_store.collection_info). - Adds
remaining_test_issues.mddocumenting current failing areas and proposed next steps.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/integration/test_config_validation_flow.py | Switches DI setup to use global container overrides; updates pytest-asyncio marks; expands mocked manager/vector-store behavior. |
| remaining_test_issues.md | Documents current failing test issues and proposed solutions for follow-up work. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| container = get_container() | ||
| container.override(CodeWeaverSettingsType, test_settings) | ||
| yield container | ||
| container.clear() |
There was a problem hiding this comment.
The autouse fixture calls container.clear() in teardown, which wipes factories/singletons/providers on the global container. This can create cross-test interference and unnecessary provider re-discovery. Prefer reverting only what this fixture changed (e.g., container.clear_overrides() / clear request cache) or using reset_container() if you truly need a clean container instance per test.
| @pytest.fixture | ||
| def mock_checkpoint_manager(test_checkpoint_data: dict) -> AsyncMock: | ||
| """Create mock CheckpointManager with test data.""" | ||
| manager = AsyncMock() | ||
|
|
||
| # Create checkpoint object | ||
| checkpoint = Mock() | ||
| checkpoint.version = test_checkpoint_data["version"] | ||
| checkpoint.collection_name = test_checkpoint_data["collection_name"] | ||
| checkpoint.total_vectors = test_checkpoint_data["total_vectors"] | ||
|
|
||
| # Support dictionary-like access if something tries to subscript it | ||
| checkpoint.__getitem__ = Mock(side_effect=test_checkpoint_data.__getitem__) | ||
| checkpoint.get = Mock(side_effect=test_checkpoint_data.get) | ||
|
|
||
| # Create collection metadata | ||
| metadata = Mock() | ||
| metadata.dense_model = test_checkpoint_data["collection_metadata"]["dense_model"] | ||
| metadata.dense_model_family = test_checkpoint_data["collection_metadata"]["dense_model_family"] | ||
| metadata.query_model = test_checkpoint_data["collection_metadata"]["query_model"] | ||
| metadata.dimension = test_checkpoint_data["collection_metadata"]["dimension"] | ||
| metadata.datatype = test_checkpoint_data["collection_metadata"]["datatype"] | ||
| metadata.vector_count = test_checkpoint_data["collection_metadata"]["vector_count"] | ||
| metadata.get_vector_dimension = Mock(return_value=2048) | ||
| metadata.get_vector_datatype = Mock(return_value="float32") | ||
|
|
||
| checkpoint.collection_metadata = metadata | ||
|
|
||
| manager.load = AsyncMock(return_value=checkpoint) | ||
| manager.validate_checkpoint_compatibility = AsyncMock(return_value=(True, "NONE")) | ||
|
|
||
| # Do not mock _extract_fingerprint or _create_fingerprint | ||
| # The real implementation will be run, but it requires the global settings to exist, | ||
| # which we've solved by using `container.override(CodeWeaverSettingsType, test_settings)` | ||
| # However, CheckpointManager uses `get_settings()` from core_settings, which reads from dependency container. | ||
|
|
There was a problem hiding this comment.
mock_checkpoint_manager is an AsyncMock, but ConfigChangeAnalyzer calls _extract_fingerprint() / _create_fingerprint() synchronously and expects real CheckpointSettingsFingerprint objects. With an unconfigured AsyncMock, those attributes become async mocks/coroutines, so new_fingerprint.is_compatible_with(...) will fail at runtime. Use a non-async base mock (e.g., Mock(spec=CheckpointManager)) or explicitly set _extract_fingerprint and _create_fingerprint to return real fingerprint instances.
| manager.load = AsyncMock(return_value=checkpoint) | ||
| manager.validate_checkpoint_compatibility = AsyncMock(return_value=(True, "NONE")) |
There was a problem hiding this comment.
The fixture configures manager.load = AsyncMock(...), but the tests in this file call mock_checkpoint_manager.load_checkpoint(). Since load_checkpoint is not configured here, it will be an AsyncMock returning another mock/coroutine, which can break the tests. Align the mock API with the production CheckpointManager (load()), or add a load_checkpoint alias that returns the same checkpoint if the analyzer/tests still use that name.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@bashandbone I've opened a new pull request, #208, to work on those changes. Once the pull request is ready, I'll request review from you. |
I've addressed the CI issue with the Gemini Actions. The PR was attempting to invoke Gemini PR Review actions without an API key being present in this fork/environment. I added conditions in |
Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
…tion tests (#208) * Initial plan * Address review comments: fix container teardown, mock types, and doc accuracy Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com> * Fix mock types, container teardown, and doc accuracy in config validation tests Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com> * Add fingerprint mocks, fix container isolation, and update docs Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| container.override(CodeWeaverSettingsType, test_settings) | ||
| yield container |
There was a problem hiding this comment.
Container.override() treats any callable override as a factory (and will call/await it). Since test_settings is a Mock (callable by default), resolving CodeWeaverSettingsType will invoke the mock and return a different Mock, so the override won’t reliably supply the intended settings instance. Wrap test_settings in a non-callable object (e.g., NonCallableMock) or override with a small factory (sync or async) that returns test_settings.
| # Snapshot pre-test overrides so teardown can restore them exactly. | ||
| # NOTE: _overrides is a private attribute; this is acceptable in tests where | ||
| # no public "snapshot overrides" API exists on the container. | ||
| initial_overrides = dict(container._overrides) if hasattr(container, "_overrides") else {} | ||
|
|
There was a problem hiding this comment.
The fixture snapshots/restores overrides by reaching into container._overrides and manually calling clear_overrides(). The container already exposes a public use_overrides({...}) context manager that snapshots/restores overrides safely; using it avoids reliance on private state and reduces the chance of leaking/overwriting other overrides.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@bashandbone I've opened a new pull request, #209, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
…alidation tests (#209) * Initial plan * Fix container override callable issue and remove private API access in test fixtures Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com> --------- Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| manager.load = AsyncMock(return_value=checkpoint) | ||
| manager.validate_checkpoint_compatibility = AsyncMock(return_value=(True, "NONE")) | ||
|
|
||
| # Do not mock _extract_fingerprint or _create_fingerprint | ||
| # The real implementation will be run, but it requires the global settings to exist, | ||
| # which we've solved by using `container.override(CodeWeaverSettingsType, test_settings)` |
There was a problem hiding this comment.
Because manager is an AsyncMock, any unconfigured attribute access (including _extract_fingerprint / _create_fingerprint) will produce another AsyncMock and calling it returns a coroutine. ConfigChangeAnalyzer calls these methods synchronously, so this mock setup will break analysis/validation paths. Prefer a Mock (optionally spec=CheckpointManager) for the checkpoint manager, and only use AsyncMock for truly-async methods like load()/save().
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
|
👋 Hey @bashandbone, Thanks for your contribution to codeweaver! 🧵You need to agree to the CLA first... 🖊️Before we can accept your contribution, you need to agree to our Contributor License Agreement (CLA). To agree to the CLA, please comment:
Those exact words are important1, so please don't change them. 😉 You can read the full CLA here: Contributor License Agreement ✅ @bashandbone has signed the CLA. You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. Footnotes
|
Code Review - PR #207I've reviewed the changes in this PR that address test suite setup issues. Overall, this is a solid step forward in stabilizing the test infrastructure, but there are several important issues that need to be addressed before merging. SummaryThis PR makes three main changes:
Critical Issues1. DI Container Override Implementation (
|
I read the contributors license agreement and I agree to it. |
|
@bashandbone I've opened a new pull request, #210, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
…yncMock only on async methods (#210) * Initial plan * Fix mock_checkpoint_manager to use Mock(spec=CheckpointManager) with AsyncMock only for async methods Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com> * Fix type casting for model configuration Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com> --------- Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…35176754165263 Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
This PR addresses a large number of fundamental setup issues in the test suite that were causing the tests to fail during initialization, or crash before assertions could be made:
async deffixtures to use@pytest_asyncio.fixtureas@pytest.fixturedrops support for it in pytest 9.find_codeoverriding the actual module resulting inmodel_dumperrors).awaitstatements intest_config_validation_flow.pyfor methods likeanalyze_current_config.HealthServiceinitialization inside tests to match the new constructor signature required by the new DI container.setup_test_containerfixture totest_config_validation_flow.pyto ensure theContaineris successfully populated with test settings to preventContainer.resolveandRuntimeWarningissues during testing.PR created automatically by Jules for task 4039835176754165263 started by @bashandbone
Summary by Sourcery
Stabilize the config validation integration tests by fixing DI container setup and updating async test configuration, and document remaining test issues and follow-ups.
Enhancements:
Documentation:
Tests: