From b667bca5a2ddf8ac5827b3c9763369c18800ceea Mon Sep 17 00:00:00 2001 From: phernandez Date: Mon, 24 Mar 2025 22:45:56 -0500 Subject: [PATCH] fix test coverage and type checks Signed-off-by: phernandez --- src/basic_memory/cli/commands/project.py | 7 +- src/basic_memory/markdown/entity_parser.py | 23 +++-- src/basic_memory/markdown/schemas.py | 2 +- src/basic_memory/mcp/tools/utils.py | 27 ------ src/basic_memory/mcp/tools/write_note.py | 7 +- src/basic_memory/services/entity_service.py | 11 +-- src/basic_memory/sync/watch_service.py | 2 +- src/basic_memory/utils.py | 28 +++++- tests/cli/test_project_commands.py | 16 +-- tests/mcp/test_tool_notes.py | 37 ++++--- tests/services/test_entity_service.py | 2 +- tests/sync/test_tmp_files.py | 102 ++++++++++---------- 12 files changed, 134 insertions(+), 130 deletions(-) diff --git a/src/basic_memory/cli/commands/project.py b/src/basic_memory/cli/commands/project.py index f90522ce..3dcf1912 100644 --- a/src/basic_memory/cli/commands/project.py +++ b/src/basic_memory/cli/commands/project.py @@ -98,15 +98,16 @@ def set_default_project( try: # Set the default project config_manager.set_default_project(name) - + # Also activate it for the current session by setting the environment variable os.environ["BASIC_MEMORY_PROJECT"] = name - + # Reload configuration to apply the change from importlib import reload from basic_memory import config as config_module + reload(config_module) - + console.print(f"[green]Project '{name}' set as default and activated[/green]") except ValueError as e: # pragma: no cover console.print(f"[red]Error: {e}[/red]") diff --git a/src/basic_memory/markdown/entity_parser.py b/src/basic_memory/markdown/entity_parser.py index da628d22..743af5b8 100644 --- a/src/basic_memory/markdown/entity_parser.py +++ b/src/basic_memory/markdown/entity_parser.py @@ -4,21 +4,22 @@ Uses markdown-it with plugins to parse structured data from markdown content. """ from dataclasses import dataclass, field -from pathlib import Path from datetime import datetime +from pathlib import Path from typing import Any, Optional -import dateparser -from markdown_it import MarkdownIt +import dateparser import frontmatter +from markdown_it import MarkdownIt from basic_memory.markdown.plugins import observation_plugin, relation_plugin from basic_memory.markdown.schemas import ( - EntityMarkdown, EntityFrontmatter, + EntityMarkdown, Observation, Relation, ) +from basic_memory.utils import parse_tags md = MarkdownIt().use(observation_plugin).use(relation_plugin) @@ -56,11 +57,11 @@ def parse(content: str) -> EntityContent: ) -def parse_tags(tags: Any) -> list[str]: - """Parse tags into list of strings.""" - if isinstance(tags, (list, tuple)): - return [str(t).strip() for t in tags if str(t).strip()] - return [t.strip() for t in tags.split(",") if t.strip()] +# def parse_tags(tags: Any) -> list[str]: +# """Parse tags into list of strings.""" +# if isinstance(tags, (list, tuple)): +# return [str(t).strip() for t in tags if str(t).strip()] +# return [t.strip() for t in tags.split(",") if t.strip()] class EntityParser: @@ -101,7 +102,9 @@ class EntityParser: metadata = post.metadata metadata["title"] = post.metadata.get("title", absolute_path.name) metadata["type"] = post.metadata.get("type", "note") - metadata["tags"] = parse_tags(post.metadata.get("tags", [])) + tags = parse_tags(post.metadata.get("tags", [])) # pyright: ignore + if tags: + metadata["tags"] = tags # frontmatter entity_frontmatter = EntityFrontmatter( diff --git a/src/basic_memory/markdown/schemas.py b/src/basic_memory/markdown/schemas.py index 6bceeafe..f7c2d078 100644 --- a/src/basic_memory/markdown/schemas.py +++ b/src/basic_memory/markdown/schemas.py @@ -42,7 +42,7 @@ class EntityFrontmatter(BaseModel): @property def tags(self) -> List[str]: - return self.metadata.get("tags") if self.metadata else [] # pyright: ignore + return self.metadata.get("tags") if self.metadata else None # pyright: ignore @property def title(self) -> str: diff --git a/src/basic_memory/mcp/tools/utils.py b/src/basic_memory/mcp/tools/utils.py index 367b73c9..7f1b45ca 100644 --- a/src/basic_memory/mcp/tools/utils.py +++ b/src/basic_memory/mcp/tools/utils.py @@ -5,7 +5,6 @@ to the Basic Memory API, with improved error handling and logging. """ import typing -from typing import Union, List from httpx import Response, URL, AsyncClient, HTTPStatusError from httpx._client import UseClientDefault, USE_CLIENT_DEFAULT @@ -24,32 +23,6 @@ from loguru import logger from mcp.server.fastmcp.exceptions import ToolError -def parse_tags(tags: Union[List[str], str, None]) -> List[str]: - """Parse tags from various input formats into a consistent list. - - Args: - tags: Can be a list of strings, a comma-separated string, or None - - Returns: - A list of tag strings, or an empty list if no tags - """ - if tags is None: - return [] - - if isinstance(tags, list): - return tags - - if isinstance(tags, str): - return [tag.strip() for tag in tags.split(",") if tag.strip()] - - # For any other type, try to convert to string and parse - try: - return parse_tags(str(tags)) - except (ValueError, TypeError): - logger.warning(f"Couldn't parse tags from input of type {type(tags)}: {tags}") - return [] - - def get_error_message(status_code: int, url: URL | str, method: str) -> str: """Get a friendly error message based on the HTTP status code. diff --git a/src/basic_memory/mcp/tools/write_note.py b/src/basic_memory/mcp/tools/write_note.py index 2a5f638e..96b796b6 100644 --- a/src/basic_memory/mcp/tools/write_note.py +++ b/src/basic_memory/mcp/tools/write_note.py @@ -6,9 +6,10 @@ from loguru import logger from basic_memory.mcp.async_client import client from basic_memory.mcp.server import mcp -from basic_memory.mcp.tools.utils import call_put, parse_tags +from basic_memory.mcp.tools.utils import call_put from basic_memory.schemas import EntityResponse from basic_memory.schemas.base import Entity +from basic_memory.utils import parse_tags # Define TagType as a Union that can accept either a string or a list of strings or None TagType = Union[List[str], str, None] @@ -21,7 +22,7 @@ async def write_note( title: str, content: str, folder: str, - tags = None, # Remove type hint completely to avoid schema issues + tags=None, # Remove type hint completely to avoid schema issues ) -> str: """Write a markdown note to the knowledge base. @@ -64,7 +65,7 @@ async def write_note( # Process tags using the helper function tag_list = parse_tags(tags) - + # Create the entity request metadata = {"tags": [f"#{tag}" for tag in tag_list]} if tag_list else None entity = Entity( diff --git a/src/basic_memory/services/entity_service.py b/src/basic_memory/services/entity_service.py index eb86545f..793552ba 100644 --- a/src/basic_memory/services/entity_service.py +++ b/src/basic_memory/services/entity_service.py @@ -146,15 +146,12 @@ class EntityService(BaseService[EntityModel]): # Create post with new content from schema post = await schema_to_markdown(schema) - + # Merge new metadata with existing metadata existing_markdown.frontmatter.metadata.update(post.metadata) - + # Create a new post with merged metadata - merged_post = frontmatter.Post( - post.content, - **existing_markdown.frontmatter.metadata - ) + merged_post = frontmatter.Post(post.content, **existing_markdown.frontmatter.metadata) # write file final_content = frontmatter.dumps(merged_post, sort_keys=False) @@ -322,4 +319,4 @@ class EntityService(BaseService[EntityModel]): ) continue - return await self.repository.get_by_file_path(path) \ No newline at end of file + return await self.repository.get_by_file_path(path) diff --git a/src/basic_memory/sync/watch_service.py b/src/basic_memory/sync/watch_service.py index edf51395..31bf099b 100644 --- a/src/basic_memory/sync/watch_service.py +++ b/src/basic_memory/sync/watch_service.py @@ -351,4 +351,4 @@ class WatchService: duration_ms=duration_ms, ) - await self.write_status() \ No newline at end of file + await self.write_status() diff --git a/src/basic_memory/utils.py b/src/basic_memory/utils.py index e67dc11f..18caa98a 100644 --- a/src/basic_memory/utils.py +++ b/src/basic_memory/utils.py @@ -6,7 +6,7 @@ import logging import re import sys from pathlib import Path -from typing import Optional, Protocol, Union, runtime_checkable +from typing import Optional, Protocol, Union, runtime_checkable, List from loguru import logger from unidecode import unidecode @@ -128,3 +128,29 @@ def setup_logging( # Set log levels for noisy loggers for logger_name, level in noisy_loggers.items(): logging.getLogger(logger_name).setLevel(level) + + +def parse_tags(tags: Union[List[str], str, None]) -> List[str]: + """Parse tags from various input formats into a consistent list. + + Args: + tags: Can be a list of strings, a comma-separated string, or None + + Returns: + A list of tag strings, or an empty list if no tags + """ + if tags is None: + return [] + + if isinstance(tags, list): + return tags + + if isinstance(tags, str): + return [tag.strip() for tag in tags.split(",") if tag.strip()] + + # For any other type, try to convert to string and parse + try: # pragma: no cover + return parse_tags(str(tags)) + except (ValueError, TypeError): # pragma: no cover + logger.warning(f"Couldn't parse tags from input of type {type(tags)}: {tags}") + return [] diff --git a/tests/cli/test_project_commands.py b/tests/cli/test_project_commands.py index 9fc86c8a..876298a0 100644 --- a/tests/cli/test_project_commands.py +++ b/tests/cli/test_project_commands.py @@ -17,11 +17,11 @@ def temp_home(monkeypatch): """Create a temporary directory for testing.""" # Save the original environment variable if it exists original_env = os.environ.get("BASIC_MEMORY_PROJECT") - + # Clear environment variable for clean test if "BASIC_MEMORY_PROJECT" in os.environ: del os.environ["BASIC_MEMORY_PROJECT"] - + with TemporaryDirectory() as tempdir: temp_home = Path(tempdir) monkeypatch.setattr(Path, "home", lambda: temp_home) @@ -31,7 +31,7 @@ def temp_home(monkeypatch): config_dir.mkdir(parents=True, exist_ok=True) yield temp_home - + # Cleanup: restore original environment variable if it existed if original_env is not None: os.environ["BASIC_MEMORY_PROJECT"] = original_env @@ -141,7 +141,7 @@ def test_project_default(cli_runner, temp_home): # Verify default was set config_manager = ConfigManager() assert config_manager.default_project == "test" - + # Extra verification: check if the environment variable was set assert os.environ.get("BASIC_MEMORY_PROJECT") == "test" @@ -157,7 +157,7 @@ def test_project_current(cli_runner, temp_home): "default_project": "main", } config_file.write_text(json.dumps(config_data)) - + # Create the main project directory main_dir = temp_home / "basic-memory" main_dir.mkdir(parents=True, exist_ok=True) @@ -192,16 +192,16 @@ def test_project_default_activates_project(cli_runner, temp_home, monkeypatch): # Create a test environment env = {} monkeypatch.setattr(os, "environ", env) - + # Create two test projects config_manager = ConfigManager() config_manager.add_project("project1", str(temp_home / "project1")) - + # Set project1 as default using the CLI command result = cli_runner.invoke(app, ["project", "default", "project1"]) assert result.exit_code == 0 assert "Project 'project1' set as default and activated" in result.stdout - + # Verify the environment variable was set # This is the core of our fix - the set_default_project command now also sets # the BASIC_MEMORY_PROJECT environment variable to activate the project diff --git a/tests/mcp/test_tool_notes.py b/tests/mcp/test_tool_notes.py index a2f96a91..6fa021b3 100644 --- a/tests/mcp/test_tool_notes.py +++ b/tests/mcp/test_tool_notes.py @@ -189,7 +189,7 @@ async def test_delete_note_doesnt_exist(app): @pytest.mark.asyncio async def test_write_note_with_tag_array_from_bug_report(app): """Test creating a note with a tag array as reported in issue #38. - + This reproduces the exact payload from the bug report where Cursor was passing an array of tags and getting a type mismatch error. """ @@ -198,12 +198,12 @@ async def test_write_note_with_tag_array_from_bug_report(app): "title": "Title", "folder": "folder", "content": "CONTENT", - "tags": ["hipporag", "search", "fallback", "symfony", "error-handling"] + "tags": ["hipporag", "search", "fallback", "symfony", "error-handling"], } - + # Try to call the function with this data directly result = await write_note(**bug_payload) - + assert result assert "permalink: folder/title" in result assert "Tags" in result @@ -257,10 +257,10 @@ async def test_write_note_verbose(app): @pytest.mark.asyncio async def test_write_note_preserves_custom_metadata(app, test_config): """Test that updating a note preserves custom metadata fields. - + Reproduces issue #36 where custom frontmatter fields like Status were being lost when updating notes with the write_note tool. - + Should: - Create a note with custom frontmatter - Update the note with new content @@ -273,15 +273,14 @@ async def test_write_note_preserves_custom_metadata(app, test_config): content="# Initial content", tags=["test"], ) - + # Read the note to get its permalink content = await read_note("test/custom-metadata-note") - + # Now directly update the file with custom frontmatter # We need to use a direct file update to add custom frontmatter - from pathlib import Path import frontmatter - + file_path = test_config.home / "test" / "Custom Metadata Note.md" post = frontmatter.load(file_path) @@ -289,11 +288,11 @@ async def test_write_note_preserves_custom_metadata(app, test_config): post["Status"] = "In Progress" post["Priority"] = "High" post["Version"] = "1.0" - + # Write the file back with open(file_path, "w") as f: f.write(frontmatter.dumps(post)) - + # Now update the note using write_note result = await write_note( title="Custom Metadata Note", @@ -301,23 +300,23 @@ async def test_write_note_preserves_custom_metadata(app, test_config): content="# Updated content", tags=["test", "updated"], ) - + # Verify the update was successful assert "Updated test/Custom Metadata Note.md" in result - + # Read the note back and check if custom frontmatter is preserved content = await read_note("test/custom-metadata-note") - + # Custom frontmatter should be preserved assert "Status: In Progress" in content assert "Priority: High" in content # Version might be quoted as '1.0' due to YAML serialization assert "Version:" in content # Just check that the field exists - assert "1.0" in content # And that the value exists somewhere - + assert "1.0" in content # And that the value exists somewhere + # And new content should be there assert "# Updated content" in content - + # And tags should be updated assert "'#test'" in content - assert "'#updated'" in content \ No newline at end of file + assert "'#updated'" in content diff --git a/tests/services/test_entity_service.py b/tests/services/test_entity_service.py index cd4adcc0..33c18b16 100644 --- a/tests/services/test_entity_service.py +++ b/tests/services/test_entity_service.py @@ -521,7 +521,7 @@ async def test_update_with_content(entity_service: EntityService, file_service: """ ).strip() - # Create test entity + # update entity entity, created = await entity_service.create_or_update_entity( EntitySchema( title="Git Workflow Guide", diff --git a/tests/sync/test_tmp_files.py b/tests/sync/test_tmp_files.py index 4f4b0c25..82360391 100644 --- a/tests/sync/test_tmp_files.py +++ b/tests/sync/test_tmp_files.py @@ -7,7 +7,6 @@ import pytest from watchfiles import Change - async def create_test_file(path: Path, content: str = "test content") -> None: """Create a test file with given content.""" path.parent.mkdir(parents=True, exist_ok=True) @@ -20,9 +19,9 @@ async def test_temp_file_filter(watch_service): # Test filter_changes method directly tmp_path = str(watch_service.config.home / "test.tmp") assert not watch_service.filter_changes(Change.added, tmp_path) - + # Test with valid file - valid_path = str(watch_service.config.home / "test.md") + valid_path = str(watch_service.config.home / "test.md") assert watch_service.filter_changes(Change.added, valid_path) @@ -30,44 +29,44 @@ async def test_temp_file_filter(watch_service): async def test_handle_tmp_files(watch_service, test_config, monkeypatch): """Test handling of .tmp files during sync process.""" project_dir = test_config.home - + # Create a .tmp file - this simulates a file being written with write_file_atomic tmp_file = project_dir / "test.tmp" await create_test_file(tmp_file, "This is a temporary file") - + # Create the target final file final_file = project_dir / "test.md" await create_test_file(final_file, "This is the final file") - + # Setup changes that include both the .tmp and final file changes = { (Change.added, str(tmp_file)), (Change.added, str(final_file)), } - + # Track sync_file calls sync_calls = [] - + # Mock sync_file to track calls original_sync_file = watch_service.sync_service.sync_file - + async def mock_sync_file(path, new=True): sync_calls.append(path) return await original_sync_file(path, new) - + monkeypatch.setattr(watch_service.sync_service, "sync_file", mock_sync_file) - + # Handle changes await watch_service.handle_changes(project_dir, changes) - + # Verify .tmp file was not processed assert "test.tmp" not in sync_calls assert "test.md" in sync_calls - + # Verify only the final file got an entity tmp_entity = await watch_service.sync_service.entity_repository.get_by_file_path("test.tmp") final_entity = await watch_service.sync_service.entity_repository.get_by_file_path("test.md") - + assert tmp_entity is None, "Temp file should not have an entity" assert final_entity is not None, "Final file should have an entity" @@ -76,44 +75,43 @@ async def test_handle_tmp_files(watch_service, test_config, monkeypatch): async def test_atomic_write_tmp_file_handling(watch_service, test_config, monkeypatch): """Test handling of file changes during atomic write operations.""" project_dir = test_config.home - + # This test simulates the full atomic write process: # 1. First a .tmp file is created - # 2. Then the .tmp file is renamed to the final file + # 2. Then the .tmp file is renamed to the final file # 3. Both events are processed by the watch service - + # Setup file paths tmp_path = project_dir / "document.tmp" final_path = project_dir / "document.md" - + # Create mockup of the atomic write process await create_test_file(tmp_path, "Content for document") - + # First batch of changes - .tmp file created changes1 = {(Change.added, str(tmp_path))} - + # Process first batch await watch_service.handle_changes(project_dir, changes1) - + # Now "replace" the temp file with the final file tmp_path.rename(final_path) - + # Second batch of changes - .tmp file deleted, final file added - changes2 = { - (Change.deleted, str(tmp_path)), - (Change.added, str(final_path)) - } - + changes2 = {(Change.deleted, str(tmp_path)), (Change.added, str(final_path))} + # Process second batch await watch_service.handle_changes(project_dir, changes2) - + # Verify only the final file is in the database tmp_entity = await watch_service.sync_service.entity_repository.get_by_file_path("document.tmp") - final_entity = await watch_service.sync_service.entity_repository.get_by_file_path("document.md") - + final_entity = await watch_service.sync_service.entity_repository.get_by_file_path( + "document.md" + ) + assert tmp_entity is None, "Temp file should not have an entity" assert final_entity is not None, "Final file should have an entity" - + # Check events new_events = [e for e in watch_service.state.recent_events if e.action == "new"] assert len(new_events) == 1 @@ -124,57 +122,63 @@ async def test_atomic_write_tmp_file_handling(watch_service, test_config, monkey async def test_rapid_atomic_writes(watch_service, test_config): """Test handling of rapid atomic writes to the same destination.""" project_dir = test_config.home - + # This test simulates multiple rapid atomic writes to the same file: - # 1. Several .tmp files are created one after another + # 1. Several .tmp files are created one after another # 2. Each is then renamed to the same final file # 3. Events are batched and processed together - + # Setup file paths tmp1_path = project_dir / "document.1.tmp" - tmp2_path = project_dir / "document.2.tmp" + tmp2_path = project_dir / "document.2.tmp" final_path = project_dir / "document.md" - + # Create multiple temp files that will be used in sequence await create_test_file(tmp1_path, "First version") await create_test_file(tmp2_path, "Second version") - + # Simulate the first atomic write tmp1_path.rename(final_path) - + # Brief pause to ensure file system registers the change await asyncio.sleep(0.1) - + # Read content to verify content1 = final_path.read_text() assert content1 == "First version" - + # Simulate the second atomic write tmp2_path.rename(final_path) - + # Verify content was updated content2 = final_path.read_text() assert content2 == "Second version" - + # Create a batch of changes that might arrive in mixed order changes = { (Change.added, str(tmp1_path)), (Change.deleted, str(tmp1_path)), - (Change.added, str(tmp2_path)), + (Change.added, str(tmp2_path)), (Change.deleted, str(tmp2_path)), (Change.added, str(final_path)), (Change.modified, str(final_path)), } - + # Process all changes await watch_service.handle_changes(project_dir, changes) - + # Verify only the final file is in the database - final_entity = await watch_service.sync_service.entity_repository.get_by_file_path("document.md") + final_entity = await watch_service.sync_service.entity_repository.get_by_file_path( + "document.md" + ) assert final_entity is not None - + # Also verify no tmp entities were created - tmp1_entity = await watch_service.sync_service.entity_repository.get_by_file_path("document.1.tmp") - tmp2_entity = await watch_service.sync_service.entity_repository.get_by_file_path("document.2.tmp") + tmp1_entity = await watch_service.sync_service.entity_repository.get_by_file_path( + "document.1.tmp" + ) + tmp2_entity = await watch_service.sync_service.entity_repository.get_by_file_path( + "document.2.tmp" + ) assert tmp1_entity is None assert tmp2_entity is None