-
Notifications
You must be signed in to change notification settings - Fork 4
Comprehensive Testing & Critical Bug Fixes #11
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
Open
aaronlippold
wants to merge
10
commits into
ukkit:main
Choose a base branch
from
aaronlippold:fix-storage-health-coroutine-error
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Comprehensive Testing & Critical Bug Fixes #11
aaronlippold
wants to merge
10
commits into
ukkit:main
from
aaronlippold:fix-storage-health-coroutine-error
Conversation
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
- Fix async/await coroutine error in status_monitoring.py - Fix data deletion during install with absolute path resolution - Add absolute path handling in storage.py and archival.py Authored by: Aaron Lippold<[email protected]>
- Add GitHub Actions workflows following MCP Python SDK standards - Create test fixtures, factories, and helpers in conftest.py - Configure gitignore for test artifacts and internal docs - Enable cross-platform testing (Linux, macOS, Windows) Authored by: Aaron Lippold<[email protected]>
- Add status monitoring tests with async health checks - Add metrics collector tests for performance tracking - Add operation logger tests for operation tracking - Add resource monitor tests for system monitoring - Use fixtures and factories for consistent test patterns Authored by: Aaron Lippold<[email protected]>
- Add comprehensive models.py validation tests (93% coverage) - Add storage contract tests for CRUD operations - Add regression tests for storage path bug fix - Test security validation (XSS, SQL injection, path traversal) - Test business logic and state management Authored by: Aaron Lippold<[email protected]>
- Add LRU cache eviction tests - Add disk cache persistence tests - Add cache invalidation tests for external modifications - Add TTL expiration and size calculation tests - Include Windows datetime precision fixes Authored by: Aaron Lippold<[email protected]>
- Test all CRUD operations with real API behavior - Test PATCH operations (add_summary_entry appends) - Test tag management and metadata operations - Test concurrent operations and edge cases - Cover all 35+ public methods with real behavior validation Authored by: Aaron Lippold<[email protected]>
- Test all 23 MCP tool handlers - Test server infrastructure and integration workflows - Test error handling and concurrent operations - Add optimized server and token usage monitoring tests - Include Windows file handling compatibility fixes Authored by: Aaron Lippold<[email protected]>
- Fix 74 linting errors (UP038, B904, F841, E501, etc.) - Apply modern Python type hints and syntax - Remove unused variables and redundant code - Fix exception handling and line length issues - Apply MCP SDK formatting standards Authored by: Aaron Lippold<[email protected]>
- Merge with upstream v2.3.1 dependencies - Apply linting to protect_data.py utility - Update uv.lock with latest dependency resolution - Maintain compatibility with upstream changes Authored by: Aaron Lippold<[email protected]>
- Merge upstream v2.3.2 async/await fixes - Keep modern type hints (list[T] vs List[T]) - Maintain clean formatting and DRY patterns - Fix duplicate await call in generate_system_report - Update version to v2.3.2 Authored by: Aaron Lippold<[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Comprehensive Testing & Critical Bug Fixes
Overview
This PR adds comprehensive testing coverage and fixes two critical bugs, including one that causes permanent data loss during installation. We independently discovered and fixed the async/await issue addressed in v2.3.2, and we've integrated with that release while maintaining modern Python conventions.
Summary:
Critical Bug Fixes
1. Data Loss During Package Installation
Issue: Running
uv pip install -e .
caused permanent deletion of ALL memory slots without warning, resulting in complete loss of user data.Root Cause:
When
uv pip install -e .
changes the working directory during installation, the relative path resolves to a different location, causing data to be written to (and potentially deleted from) the wrong directory.Solution (storage.py:82-83, archival.py:82-83):
Testing: 4 comprehensive regression tests in
test_storage_path_bug.py
validate the fix and prevent regression.Note: v2.3.1 added valuable data protection utilities and documentation. Our fix complements that by addressing the root cause in the code, preventing the issue from occurring in the first place.
2. Async/Await Coroutine Error (v2.3.2 Compatibility)
Issue:
TypeError: object of type 'coroutine' has no len()
in health monitoring.Discovery: We independently discovered and fixed this bug during our testing work. When we merged with upstream v2.3.2, we found you had released the same fix.
Our Solution:
DiagnosticTool._check_storage_health()
properly asyncawait
calls inrun_health_checks()
andgenerate_system_report()
await self.run_health_checks()
call (line 685)Merge Resolution: We kept the modern Python type hints (
list[T]
vsList[T]
) and clean formatting from our version while incorporating your v2.3.2 changes.Testing: 12 comprehensive async health monitoring tests ensure this works correctly.
Testing Coverage
Module Coverage Summary
Overall: 43% coverage focused on critical user-facing functionality
Testing Methodology
Bottom-up approach:
Focus areas:
Example: We tested the actual behavior to discover that
save_memory()
replaces content whileadd_summary_entry()
appends, then wrote tests accordingly.Test Infrastructure
New test files:
test_cache.py
(22 tests) - Multi-level caching with Windows compatibilitytest_storage_operations.py
(44 tests) - Complete storage API coveragetest_server_mcp_interface.py
(69 tests) - All MCP tool handlerstest_optimized_server.py
(18 tests) - Token usage optimizationtest_storage_path_bug.py
(4 tests) - Data loss bug regression testsEnhanced infrastructure:
conftest.py
- Fixtures, factories, helpers for consistent testingtest_models_contracts.py
- +17 tests for validation and securityCode Quality & Modernization
Ruff Compliance (100%)
list[T]
,dict[K,V]
instead ofList[T]
,Dict[K,V]
from e
Code Improvements
CI/CD
Cross-Platform Compatibility
All 250 tests pass on:
Integration with v2.3.2
We merged with upstream v2.3.2 and resolved conflicts by:
await self.run_health_checks()
callThe async/await fix we made matches yours, discovered independently during our testing work.
Files Changed
Source code (31 files):
storage.py
,archival.py
,status_monitoring.py
Tests (13 files):
conftest.py
with fixtures/factoriesConfiguration (4 files):
.github/workflows/
- CI/CD following MCP SDK standards.gitignore
- Test artifacts and internal docspyproject.toml
- Dev dependencies (merged with v2.3.2)Acknowledgments
Thank you for creating and maintaining memcord! We use it extensively in our work and wanted to contribute back with testing coverage and bug fixes. Your v2.3.1 data protection utilities and v2.3.2 async fixes show great attention to quality and user safety.
These changes aim to build on memcord's solid foundation by adding comprehensive testing and modernizing the codebase with current Python best practices.
Please let us know if you'd like any adjustments or have suggestions for improvements!