test: improve test_sync.py and sync/test_client.py readability and clarity

Author: AlePouroullis

tests/custom/integration/test_sync.py

@@ -17,16 +17,11 @@ class SyncableFile(NamedTuple):
 
 
 @pytest.fixture
-def test_file_structure(
+def syncable_files_fixture(
     get_humanloop_client: GetHumanloopClientFn,
     sdk_test_dir: str,
-    request,  # This gives us access to the test function's parameters
 ) -> Generator[list[SyncableFile], None, None]:
-    """Creates a predefined structure of files in Humanloop for testing sync.
-
-    The fixture will use use_local_files=True if the test function requests it,
-    otherwise it will use use_local_files=False.
-    """
+    """Creates a predefined structure of files in Humanloop for testing sync."""
     files: List[SyncableFile] = [
         SyncableFile(
             path="prompts/gpt-4",
@@ -56,7 +51,6 @@ def test_file_structure(
     ]
 
     humanloop_client = get_humanloop_client()
-    # Create the files in Humanloop
     created_files = []
     for file in files:
         full_path = f"{sdk_test_dir}/{file.path}"
@@ -86,77 +80,71 @@ def test_file_structure(
 def cleanup_local_files():
     """Cleanup any locally synced files after tests"""
     yield
-    # Clean up the local humanloop directory after tests
     local_dir = Path("humanloop")
     if local_dir.exists():
         import shutil
 
         shutil.rmtree(local_dir)
 
 
-def test_client(get_humanloop_client: GetHumanloopClientFn):
-    client = get_humanloop_client(use_local_files=True)
-    assert client.use_local_files
-
-
 def test_pull_basic(
-    test_file_structure: List[SyncableFile],
+    syncable_files_fixture: List[SyncableFile],
     get_humanloop_client: GetHumanloopClientFn,
 ):
     """Test that humanloop.sync() correctly syncs remote files to local filesystem"""
-    # Run the sync
+    # GIVEN a set of files in the remote system (from syncable_files_fixture)
     humanloop_client = get_humanloop_client()
+
+    # WHEN running the sync
     successful_files = humanloop_client.pull()
 
-    # Verify each file was synced correctly
-    for file in test_file_structure:
-        # Get the extension based on file type: .prompt, .agent
+    # THEN our local filesystem should mirror the remote filesystem in the HL Workspace
+    for file in syncable_files_fixture:
         extension = f".{file.type}"
-
-        # The local path should mirror the remote path structure
         local_path = Path("humanloop") / f"{file.path}{extension}"
 
-        # Basic assertions
+        # THEN the file and its directory should exist
         assert local_path.exists(), f"Expected synced file at {local_path}"
         assert local_path.parent.exists(), f"Expected directory at {local_path.parent}"
 
-        # Verify it's not empty
+        # THEN the file should not be empty
         content = local_path.read_text()
         assert content, f"File at {local_path} should not be empty"
 
 
 def test_overload_with_local_files(
     get_humanloop_client: GetHumanloopClientFn,
-    test_file_structure: List[SyncableFile],
+    syncable_files_fixture: List[SyncableFile],
 ):
     """Test that overload_with_local_files correctly handles local files."""
-    # The test_file_structure fixture will automatically use use_local_files=True
+    # GIVEN a client with use_local_files=True and pulled files
     humanloop_client = get_humanloop_client(use_local_files=True)
     humanloop_client.pull()
 
-    # Test using the pulled files
-    test_file = test_file_structure[0]  # Use the first test file
+    # GIVEN a test file from the structure
+    test_file = syncable_files_fixture[0]
     extension = f".{test_file.type}"
     local_path = Path("humanloop") / f"{test_file.path}{extension}"
 
-    # Verify the file was pulled correctly
+    # THEN the file should exist locally
     assert local_path.exists(), f"Expected pulled file at {local_path}"
     assert local_path.parent.exists(), f"Expected directory at {local_path.parent}"
 
-    # Test call with pulled file
+    # WHEN calling the file
     response: Union[AgentResponse, PromptResponse]
     if test_file.type == "prompt":
         response = humanloop_client.prompts.call(  # type: ignore [assignment]
             path=test_file.path, messages=[{"role": "user", "content": "Testing"}]
         )
-        assert response is not None
     elif test_file.type == "agent":
         response = humanloop_client.agents.call(  # type: ignore [assignment]
             path=test_file.path, messages=[{"role": "user", "content": "Testing"}]
         )
-        assert response is not None
+    # THEN the response should not be None
+    assert response is not None
 
-    # Test with invalid path
+    # WHEN calling with an invalid path
+    # THEN it should raise HumanloopRuntimeError
     with pytest.raises(HumanloopRuntimeError):
         sub_client: Union[PromptsClient, AgentsClient]
         match test_file.type:
@@ -171,36 +159,37 @@ def test_overload_with_local_files(
 
 def test_overload_log_with_local_files(
     get_humanloop_client: GetHumanloopClientFn,
-    test_file_structure: List[SyncableFile],
+    syncable_files_fixture: List[SyncableFile],
     sdk_test_dir: str,
 ):
     """Test that overload_with_local_files correctly handles local files for log operations."""
-    # The test_file_structure fixture will automatically use use_local_files=True
+    # GIVEN a client with use_local_files=True and pulled files
     humanloop_client = get_humanloop_client(use_local_files=True)
     humanloop_client.pull()
 
-    # Test using the pulled files
-    test_file = test_file_structure[0]  # Use the first test file
+    # GIVEN a test file from the structure
+    test_file = syncable_files_fixture[0]
     extension = f".{test_file.type}"
     local_path = Path("humanloop") / f"{test_file.path}{extension}"
 
-    # Verify the file was pulled correctly
+    # THEN the file should exist locally
     assert local_path.exists(), f"Expected pulled file at {local_path}"
     assert local_path.parent.exists(), f"Expected directory at {local_path.parent}"
 
-    # Test log with pulled file
+    # WHEN logging with the pulled file
     if test_file.type == "prompt":
         response = humanloop_client.prompts.log(  # type: ignore [assignment]
             path=test_file.path, messages=[{"role": "user", "content": "Testing"}], output="Test response"
         )
-        assert response is not None
     elif test_file.type == "agent":
         response = humanloop_client.agents.log(  # type: ignore [assignment]
             path=test_file.path, messages=[{"role": "user", "content": "Testing"}], output="Test response"
         )
-        assert response is not None
+    # THEN the response should not be None
+    assert response is not None
 
-    # Test with invalid path
+    # WHEN logging with an invalid path
+    # THEN it should raise HumanloopRuntimeError
     with pytest.raises(HumanloopRuntimeError):
         if test_file.type == "prompt":
             humanloop_client.prompts.log(
@@ -218,29 +207,24 @@ def test_overload_log_with_local_files(
 
 def test_overload_version_environment_handling(
     get_humanloop_client: GetHumanloopClientFn,
-    test_file_structure: List[SyncableFile],
+    syncable_files_fixture: List[SyncableFile],
 ):
-    """Test that overload_with_local_files correctly handles version_id and environment parameters.
-
-    Flow:
-    1. Create files in remote (via test_file_structure fixture)
-    2. Pull files locally
-    3. Test that specifying path + version_id/environment raises HumanloopRuntimeError
-    """
-    # First pull the files locally
+    """Test that overload_with_local_files correctly handles version_id and environment parameters."""
+    # GIVEN a client with use_local_files=True and pulled files
     humanloop_client = get_humanloop_client(use_local_files=True)
     humanloop_client.pull()
 
-    # Test using the pulled files
-    test_file = test_file_structure[0]  # Use the first test file
+    # GIVEN a test file from the structure
+    test_file = syncable_files_fixture[0]
     extension = f".{test_file.type}"
     local_path = Path("humanloop") / f"{test_file.path}{extension}"
 
-    # Verify the file was pulled correctly
+    # THEN the file should exist locally
     assert local_path.exists(), f"Expected pulled file at {local_path}"
     assert local_path.parent.exists(), f"Expected directory at {local_path.parent}"
 
-    # Test with version_id - should raise HumanloopRuntimeError
+    # WHEN calling with version_id
+    # THEN it should raise HumanloopRuntimeError
     with pytest.raises(HumanloopRuntimeError, match="Cannot use local file.*version_id or environment was specified"):
         if test_file.type == "prompt":
             humanloop_client.prompts.call(
@@ -255,7 +239,8 @@ def test_overload_version_environment_handling(
                 messages=[{"role": "user", "content": "Testing"}],
             )
 
-    # Test with environment - should raise HumanloopRuntimeError
+    # WHEN calling with environment
+    # THEN it should raise HumanloopRuntimeError
     with pytest.raises(HumanloopRuntimeError, match="Cannot use local file.*version_id or environment was specified"):
         if test_file.type == "prompt":
             humanloop_client.prompts.call(
@@ -270,7 +255,8 @@ def test_overload_version_environment_handling(
                 messages=[{"role": "user", "content": "Testing"}],
             )
 
-    # Test with both version_id and environment - should raise HumanloopRuntimeError
+    # WHEN calling with both version_id and environment
+    # THEN it should raise HumanloopRuntimeError
     with pytest.raises(HumanloopRuntimeError, match="Cannot use local file.*version_id or environment was specified"):
         if test_file.type == "prompt":
             humanloop_client.prompts.call(

tests/custom/sync/test_client.py

@@ -24,13 +24,16 @@ def sync_client(mock_client: Mock, tmp_path: Path) -> SyncClient:
 
 def test_init(sync_client: SyncClient, tmp_path: Path):
     """Test basic initialization of SyncClient."""
+    # GIVEN a SyncClient instance
+    # THEN it should be initialized with correct base directory, cache size and file types
     assert sync_client.base_dir == tmp_path
     assert sync_client._cache_size == 10
     assert sync_client.SERIALIZABLE_FILE_TYPES == ["prompt", "agent"]
 
 
 def test_normalize_path(sync_client: SyncClient):
     """Test path normalization functionality."""
+    # GIVEN various file paths with different formats
     test_cases = [
         ("path/to/file.prompt", "path/to/file"),
         ("path\\to\\file.agent", "path/to/file"),
@@ -40,11 +43,17 @@ def test_normalize_path(sync_client: SyncClient):
     ]
 
     for input_path, expected in test_cases:
-        assert sync_client._normalize_path(input_path) == expected
+        # WHEN they are normalized
+        normalized = sync_client._normalize_path(input_path)
+        # THEN they should be converted to the expected format
+        assert normalized == expected
 
 
 def test_is_file(sync_client: SyncClient):
     """Test file type detection."""
+    # GIVEN various file paths
+    # WHEN checking if they are valid file types
+    # THEN only .prompt and .agent files should return True
     assert sync_client.is_file("test.prompt")
     assert sync_client.is_file("test.agent")
     assert not sync_client.is_file("test.txt")
@@ -53,54 +62,68 @@ def test_is_file(sync_client: SyncClient):
 
 def test_save_and_read_file(sync_client: SyncClient):
     """Test saving and reading files."""
+    # GIVEN a file content and path
     content = "test content"
     path = "test/path"
     file_type = "prompt"
 
-    # Test saving
+    # WHEN saving the file
     sync_client._save_serialized_file(content, path, "prompt")
     saved_path = sync_client.base_dir / path
     saved_path = saved_path.parent / f"{saved_path.stem}.{file_type}"
+
+    # THEN the file should exist on disk
     assert saved_path.exists()
 
-    # Test reading
+    # WHEN reading the file
     read_content = sync_client.get_file_content(path, file_type)
+
+    # THEN the content should match
     assert read_content == content
 
 
 def test_error_handling(sync_client: SyncClient):
     """Test error handling in various scenarios."""
-    # Test file not found
+    # GIVEN a nonexistent file
+    # WHEN trying to read it
+    # THEN a HumanloopRuntimeError should be raised
     with pytest.raises(HumanloopRuntimeError, match="Local file not found"):
         sync_client.get_file_content("nonexistent", "prompt")
 
-    # Test invalid file type
+    # GIVEN an invalid file type
+    # WHEN trying to pull the file
+    # THEN a ValueError should be raised
     with pytest.raises(ValueError, match="Unsupported file type"):
         sync_client._pull_file("test.txt")
 
-    # Test API error
+    # GIVEN an API error
+    # WHEN trying to pull a file
+    # THEN it should return False
     with patch.object(sync_client.client.files, "retrieve_by_path", side_effect=Exception("API Error")):
         assert not sync_client._pull_file("test.prompt")
 
 
 def test_cache_functionality(sync_client: SyncClient):
     """Test LRU cache functionality."""
-    # Save a test file
+    # GIVEN a test file
     content = "test content"
     path = "test/path"
     file_type: Literal["prompt", "agent"] = "prompt"
     sync_client._save_serialized_file(content, path, file_type)
 
-    # First read should hit disk
+    # WHEN reading the file for the first time
     sync_client.get_file_content(path, file_type)
+    # THEN it should hit disk (implicitly verified by no cache hit)
 
-    # Modify file on disk
+    # WHEN modifying the file on disk
     saved_path = sync_client.base_dir / f"{path}.{file_type}"
     saved_path.write_text("modified content")
 
-    # Second read should use cache
+    # THEN subsequent reads should use cache
     assert sync_client.get_file_content(path, file_type) == content
 
-    # Clear cache and verify new content is read
+    # WHEN clearing the cache
     sync_client.clear_cache()
+
+    # THEN new content should be read from disk
     assert sync_client.get_file_content(path, file_type) == "modified content"