.github/workflows/ci.yml (2)

8-8: Remove temporary branch trigger before merging.

The refactor-test branch trigger appears to be temporary for testing this PR. Remove it before merging to avoid triggering CI on a feature branch indefinitely.

🔎 Proposed fix

       - main
       - develop
-      - refactor-test 

---

24-27: Update actions/setup-python to v5.

The static analysis tool correctly flags that actions/setup-python@v3 is outdated. Version 5 is the current stable release and includes important bug fixes and performance improvements.

🔎 Proposed fix

       - name: Set up Python
-        uses: actions/setup-python@v3
+        uses: actions/setup-python@v5
         with:
           python-version: '3.11'

Apply the same change at line 63 for the integration-test job.

tests/unit_tests/test_version.py (2)

13-41: Consider splitting into focused test methods for better test isolation.

While the single test covers all functionality, splitting into smaller methods (e.g., test_comparison_operators, test_equality, test_normalization, test_string_representation) would improve test isolation and make failures easier to diagnose.

---

39-39: Remove extraneous f-prefix from string literal.

This string has no placeholders, so the f prefix is unnecessary.

🔎 Proposed fix

-        print(f"\n✅ Version comparison tests passed")
+        print("\n✅ Version comparison tests passed")

tests/integration_tests/test_collection_dml.py (2)

16-191: Test is comprehensive but lacks explicit cleanup.

The test relies on fixture cleanup, but doesn't explicitly delete the created collection. Consider adding cleanup in a finally block or using pytest.fixture with cleanup to ensure test isolation, especially if the fixture doesn't drop collections.

🔎 Suggested cleanup pattern

def test_collection_dml(self, db_client):
    collection_name = f"test_dml_{int(time.time() * 1000)}"
    try:
        # ... existing test code ...
    finally:
        # Cleanup: drop the test collection
        try:
            db_client.delete_collection(name=collection_name)
        except Exception:
            pass

---

42-42: Multiple f-strings without placeholders throughout the file.

Lines 42, 60, 79, 95, 111, 127, 144, 156, 159, 166, 169, 185, and 188 use f-strings without any placeholders. Remove the f prefix from these string literals.

🔎 Example fixes

-        print(f"\n✅ Testing collection.add() - single item")
+        print("\n✅ Testing collection.add() - single item")

Apply similar changes to all flagged lines.

tests/integration_tests/test_detect_db_type_and_version.py (1)

32-32: Remove extraneous f-prefix from string literals.

Lines 32, 53, 72, and 93 use f-strings without placeholders.

🔎 Proposed fix for line 32

-        print(f"\n✅ Successfully detected seekdb Server")
+        print("\n✅ Successfully detected seekdb Server")

Apply similar changes to lines 53, 72, and 93.

tests/integration_tests/test_collection_query.py (1)

121-121: Multiple f-strings without placeholders throughout the file.

Lines 121, 136, 147, 158, 169, 185, 201, 214, 225, and 236 use f-strings without placeholders.

🔎 Example fix

-        print(f"\n✅ Testing basic query")
+        print("\n✅ Testing basic query")

tests/integration_tests/test_offical_case.py (2)

1-4: Filename typo: "offical" should be "official"

The filename test_offical_case.py contains a typo. Consider renaming to test_official_case.py for consistency and discoverability.

---

81-87: Collection cleanup may be missing

The comment states cleanup is handled by the db_client fixture, but the fixture only closes the client connection—it doesn't delete created collections. This could leave test artifacts in the database across runs.

Consider explicitly deleting the collection in a finally block:

🔎 Proposed fix

     def test_official_example(self, db_client):
         """
         Official example using client (automatic mode selection).
         
         Automatically runs for: embedded, server, oceanbase
         """
         collection_name = f"official_example_{int(time.time() * 1000)}"
         collection = db_client.get_or_create_collection(name=collection_name)
         
-        # Run the official example workflow
-        _run_official_example(collection)
-        
-        # Note: cleanup is handled automatically by the db_client fixture
+        try:
+            # Run the official example workflow
+            _run_official_example(collection)
+        finally:
+            # Cleanup: delete the test collection
+            try:
+                db_client.delete_collection(name=collection_name)
+            except Exception:
+                pass

tests/integration_tests/test_security_sql_injection.py (1)

98-98: Remove extraneous f-string prefixes

Multiple print statements use f-strings without any placeholders. While functionally harmless, removing the f prefix improves consistency.

Affected lines: 98, 120, 141, 156, 160, 192, 196, 231, 235, 271-272, 276, 308-309, 313, 364.

🔎 Example fix

-        print(f"\n🔒 Running security SQL injection tests")
+        print("\n🔒 Running security SQL injection tests")

Also applies to: 120-120, 141-141, 156-156

tests/integration_tests/test_client_creation.py (2)

17-246: Consider splitting this monolithic test

This single test method spans ~230 lines and covers 16 distinct test scenarios. While comprehensive, this makes failures harder to diagnose and increases test coupling.

Consider splitting into focused test methods (e.g., test_create_collection, test_get_collection, test_count_and_peek) that can fail independently while sharing setup via class-level fixtures.

---

71-118: Direct SQL execution couples test to internal implementation

Using db_client._server._execute() to verify table structure bypasses the public API and creates tight coupling to internal implementation details. If the internal structure changes, this test will break.

Consider using public collection APIs for verification where possible, or documenting this as an intentional implementation detail check.

tests/integration_tests/test_default_embedding_function.py (2)

83-84: Consider using a more robust wait mechanism than time.sleep(1).

A fixed 1-second sleep may be insufficient in some environments or excessive in others. Consider implementing a polling-based approach or making the wait time configurable.

---

158-170: Unit test test_dimension_of should be in unit tests directory.

This test doesn't require the db_client fixture and is purely a unit test for the dimension_of function. Consider moving it to a unit test module for clearer separation.

tests/integration_tests/test_fulltext_parser_config.py (1)

178-196: Single test method iterating through parsers reduces parameterization benefits.

While this approach works, running all parser tests within a single test_fulltext_parser method means a failure in one parser test will prevent subsequent parsers from being tested. Consider using @pytest.mark.parametrize for the parsers list to get independent test runs per parser.

🔎 Example of parametrized approach

@pytest.mark.parametrize("parser_name", ['ik', 'space', 'ngram', 'ngram2', 'beng'])
def test_fulltext_parser_config(self, db_client, parser_name):
    """Test each fulltext parser configuration."""
    self._test_fulltext_parser_config(db_client, parser_name)

def test_fulltext_parser_with_params(self, db_client):
    """Test parser with parameters."""
    self._test_fulltext_parser_config(db_client, 'ngram', params={'ngram_token_size': 3})

def test_default_parser_fallback(self, db_client):
    """Test default parser."""
    self._test_default_parser(db_client)

def test_backward_compatibility_hnsw_only(self, db_client):
    """Test backward compatibility."""
    self._test_backward_compatibility(db_client)

tests/integration_tests/test_collection_hybrid_search.py (1)

17-29: Use explicit Optional type annotation for nullable parameters.

The dimension parameter can be None but lacks explicit type annotation.

🔎 Proposed fix

+from typing import List, Optional
+
 class TestCollectionHybridSearchRefactored:
     """Test collection.hybrid_search() interface using parameterized db_client fixture"""
     
-    def _create_test_collection(self, client, collection_name: str, dimension: int = None):
+    def _create_test_collection(self, client, collection_name: str, dimension: Optional[int] = None):
         """Helper method to create a test collection"""

tests/integration_tests/conftest.py (2)

10-12: Consider using relative imports or proper package installation instead of sys.path manipulation.

Directly modifying sys.path can lead to import issues. If the tests are run via pytest from the project root with the package installed in development mode (pip install -e .), this shouldn't be necessary.

---

177-181: Bare except in cleanup could mask important errors during teardown.

While silently ignoring cleanup failures is often acceptable, consider catching Exception explicitly and optionally logging for debugging.

🔎 Proposed fix

     try:
         if hasattr(client, 'close'):
             client.close()
-    except:
+    except Exception:
         pass

tests/integration_tests/test_collection_embedding_function.py (2)

14-37: Duplicate Simple3DEmbeddingFunction class across test files.

This class is duplicated in tests/integration_tests/test_collection_get.py (per relevant code snippets). Consider extracting it to a shared test utilities module or the conftest.py.

🔎 Example: Move to conftest.py

# In tests/integration_tests/conftest.py

class Simple3DEmbeddingFunction:
    """Simple embedding function that returns 3-dimensional vectors for testing"""
    
    def __init__(self):
        self.dimension = 3
    
    def __call__(self, input: Union[str, List[str]]) -> List[List[float]]:
        """Convert documents to 3D embeddings (simple hash-based)"""
        if isinstance(input, str):
            input = [input]
        
        embeddings = []
        for doc in input:
            hash_val = hash(doc) % 1000
            embedding = [
                float((hash_val % 10) / 10.0),
                float(((hash_val // 10) % 10) / 10.0),
                float(((hash_val // 100) % 10) / 10.0)
            ]
            embeddings.append(embedding)
        
        return embeddings

---

218-218: Unused variable created_collection is intentional.

The variable is assigned for side effects (creating the collection) and the actual test retrieves it separately. Consider using _ prefix or removing the assignment entirely.

🔎 Proposed fix

-        created_collection = db_client.create_collection(
+        db_client.create_collection(
             name=collection_name,
             configuration=config,
             embedding_function=None
         )

tests/integration_tests/test_collection_hybrid_search_builder_integration.py (3)

34-46: Fix implicit Optional type hint.

Per PEP 484, the type annotation should explicitly use Optional or | None when defaulting to None. As flagged by static analysis.

🔎 Proposed fix

-    def _create_test_collection(self, client, collection_name: str, dimension: int = None):
+    def _create_test_collection(self, client, collection_name: str, dimension: int | None = None):

---

48-52: Avoid mutable default argument.

Using a mutable list as a default argument is a Python anti-pattern. While safe here since it's only read, it's better to use None and initialize inside the function. As flagged by static analysis.

🔎 Proposed fix

-    def _generate_query_vector(self, dimension: int, base_vector: List[float] = [1.0, 2.0, 3.0]) -> List[float]:
+    def _generate_query_vector(self, dimension: int, base_vector: List[float] | None = None) -> List[float]:
+        if base_vector is None:
+            base_vector = [1.0, 2.0, 3.0]
         if dimension <= len(base_vector):
             return base_vector[:dimension]

---

146-146: Remove extraneous f prefix from strings without placeholders.

Multiple print statements use f-strings without any placeholders. This applies to lines 146, 164, 192, 226, 255, 296, 335, 349, 363, 374. As flagged by static analysis.

🔎 Example fix

-        print(f"\n✅ Testing hybrid_search with full-text search only")
+        print("\n✅ Testing hybrid_search with full-text search only")

Configuration used: defaults

Review profile: CHILL

Plan: Pro

tests/integration_tests/test_admin_database_management.py (1)

43-43: Remove unnecessary f-string prefixes.

Lines 43, 74, and 83 use f-strings without any placeholders. Removing the f prefix improves consistency and avoids unnecessary string processing.

🔎 Proposed fix

-            print(f"\n📋 Step 1: List all databases")
+            print("\n📋 Step 1: List all databases")

-            print(f"\n📋 Step 5: List all databases to verify deletion")
+            print("\n📋 Step 5: List all databases to verify deletion")

-            print(f"\n🎉 All database management operations completed successfully!")
+            print("\n🎉 All database management operations completed successfully!")

Also applies to: 74-74, 83-83

tests/integration_tests/test_collection_hybrid_search.py (2)

17-17: Consider explicit Optional type hints per PEP 484.

The dimension and base_vector parameters use implicit Optional (defaulting to None). While functionally correct, PEP 484 recommends explicit Optional or union with None.

🔎 Proposed fix

+from typing import List, Optional
+
 class TestCollectionHybridSearchRefactored:
     """Test collection.hybrid_search() interface using parameterized db_client fixture"""
     
-    def _create_test_collection(self, client, collection_name: str, dimension: int = None):
+    def _create_test_collection(self, client, collection_name: str, dimension: int | None = None):
         """Helper method to create a test collection"""

-    def _generate_query_vector(self, dimension: int, base_vector: List[float] = None) -> List[float]:
+    def _generate_query_vector(self, dimension: int, base_vector: List[float] | None = None) -> List[float]:
         """Generate a query vector with the correct dimension"""

Also applies to: 31-31

---

137-137: Optional: Remove unnecessary f-string prefixes in print statements.

Several print statements use f-string syntax without any placeholders. While harmless, removing the f prefix would be slightly more accurate.

Example fix

-        print(f"\n✅ Testing hybrid_search with full-text search only")
+        print("\n✅ Testing hybrid_search with full-text search only")

Apply similar changes to lines 162, 194, 232, 275, 327, 379, 397, 415.

Also applies to: 162-162, 194-194, 232-232, 275-275, 327-327, 379-379, 397-397, 415-415

tests/integration_tests/test_collection_hybrid_search_builder_integration.py (2)

34-34: Consider explicit Optional type hints per PEP 484.

The dimension and base_vector parameters use implicit Optional (defaulting to None). While functionally correct, PEP 484 recommends explicit Optional or union with None.

🔎 Proposed fix

+from typing import List, Optional
+
 class TestCollectionHybridSearchWithBuilderRefactored:
     """Test collection.hybrid_search() using HybridSearch builder with parameterized db_client fixture"""
 
-    def _create_test_collection(self, client, collection_name: str, dimension: int = None):
+    def _create_test_collection(self, client, collection_name: str, dimension: int | None = None):
         """Helper method to create a test collection"""

-    def _generate_query_vector(self, dimension: int, base_vector: List[float] = None) -> List[float]:
+    def _generate_query_vector(self, dimension: int, base_vector: List[float] | None = None) -> List[float]:
+        if base_vector is None:
+            base_vector = [1.0, 2.0, 3.0]

Also applies to: 48-48

---

149-149: Optional: Remove unnecessary f-string prefixes in print statements.

Several print statements use f-string syntax without any placeholders. While harmless, removing the f prefix would be slightly more accurate.

Example fix

-        print(f"\n✅ Testing hybrid_search with full-text search only")
+        print("\n✅ Testing hybrid_search with full-text search only")

Apply similar changes to lines 167, 195, 229, 258, 299, 338, 352, 366, 377.

Also applies to: 167-167, 195-195, 229-229, 258-258, 299-299, 338-338, 352-352, 366-366, 377-377

tests/integration_tests/test_client_creation.py (2)

59-62: Consider logging cleanup exceptions during test execution.

The silent exception handling here occurs mid-test rather than in final cleanup. If delete_collection fails for this test artifact, it could indicate an issue worth investigating.

🔎 Suggested improvement

         # Clean up
         try:
             db_client.delete_collection(test_collection_name_config)
-        except Exception:
-            pass
+        except Exception as e:
+            print(f"   Warning: Failed to cleanup {test_collection_name_config}: {e}")

---

132-132: Remove unnecessary f prefix from strings without placeholders.

Several print statements use f-strings but don't contain any placeholders.

🔎 Suggested fix

-        print(f"\n✅ has_collection correctly returns False for non-existent collection")
+        print("\n✅ has_collection correctly returns False for non-existent collection")
         
         # Test 4: has_collection - should return True for existing collection
         assert db_client.has_collection(test_collection_name)
-        print(f"\n✅ has_collection correctly returns True for existing collection")
+        print("\n✅ has_collection correctly returns True for existing collection")
         
         # ... later in the file ...
         
-        print(f"\n✅ get_or_create_collection successfully retrieved existing collection")
+        print("\n✅ get_or_create_collection successfully retrieved existing collection")
         
         # ... later in the file ...
         
-            print(f"\n✅ delete_collection correctly raises ValueError for non-existent collection")
+            print("\n✅ delete_collection correctly raises ValueError for non-existent collection")
         
         # ... later in the file ...
         
-        print(f"\n✅ get_or_create_collection successfully created collection with default configuration")
+        print("\n✅ get_or_create_collection successfully created collection with default configuration")

Also applies to: 136-136, 147-147, 181-181, 190-190

tests/integration_tests/test_collection_get.py (1)

200-204: Consider logging cleanup exceptions.

While except Exception: pass ensures robust cleanup, consider logging exceptions for debugging (similar to line 210). This helps diagnose cleanup failures during test troubleshooting.

🔎 Suggested improvement

             finally:
                 try:
                     db_client.delete_collection(name=collection_name_2)
-                except Exception:
-                    pass
+                except Exception as e:
+                    print(f"Warning: cleanup failed for {collection_name_2}: {e}")

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: longbingljw
Repo: oceanbase/pyseekdb PR: 91
File: tests/integration_tests/test_collection_dml.py:36-36
Timestamp: 2025-12-22T12:44:52.456Z
Learning: In integration tests under tests/integration_tests, it is acceptable to use the internal db_client._server._execute() to run raw SQL (e.g., CREATE TABLE) when needing custom table setup with specific vector index configurations. Prefer using public/test harness APIs for common setup, and clearly document and limit the use of internal calls to ensure test reliability. Ensure proper cleanup and isolation for each test, and avoid leaking internal APIs into production or non-test code.

Learnt from: longbingljw
Repo: oceanbase/pyseekdb PR: 91
File: tests/integration_tests/test_collection_hybrid_search_builder_integration.py:127-132
Timestamp: 2025-12-22T12:45:51.412Z
Learning: In integration test Python files under tests/integration_tests, it is acceptable to use string interpolation to construct SQL INSERT statements for test data when the data is controlled and internal to the test. This pattern should only be used for trusted, test-only data and not with untrusted input; ensure the injection risk is mitigated by keeping inputs deterministic, non-user-supplied, and isolated to the test environment. If possible, prefer parameterized queries in non-test code, but in these tests, interpolate values selectively when you can guarantee safety.

Learnt from: longbingljw
Repo: oceanbase/pyseekdb PR: 91
File: tests/integration_tests/test_admin_database_management.py:26-39
Timestamp: 2025-12-22T12:33:44.844Z
Learning: In tests/integration_tests/test_admin_database_management.py, it is acceptable to detect the client type with isinstance(admin_client._server, pyseekdb.SeekdbEmbeddedClient). Do not expose a public AdminClient 'mode' property; keep it private (e.g., _mode) or implement it without a public accessor. If needed, document the private attribute and ensure tests rely only on internal type checks rather than public configuration.

Learnt from: longbingljw
Repo: oceanbase/pyseekdb PR: 91
File: tests/integration_tests/test_admin_database_management.py:26-39
Timestamp: 2025-12-22T12:33:44.844Z
Learning: In tests/integration_tests/test_admin_database_management.py, using `isinstance(admin_client._server, pyseekdb.SeekdbEmbeddedClient)` to detect client type is acceptable. AdminClient should not expose a public `mode` property.