Add comprehensive WITH clause integration tests

[DecisionNerd]

Summary

Adds 17 comprehensive integration tests for the WITH clause, establishing a test baseline and exposing implementation gaps.

Motivation

The WITH clause was implemented in v0.1.1 but has zero integration tests. This PR adds comprehensive test coverage to:

1. Document expected behavior
2. Identify bugs and missing features
3. Establish a baseline for future WITH clause work

Test Coverage

✅ Test Categories (17 tests total)

• Basic projection (3 tests)

  • Simple variable projection
  • Variable renaming with AS
  • Variable scoping

• Filtering (2 tests)

  • WHERE after WITH
  • Filtering on computed values

• Aggregation (2 tests)

  • COUNT with GROUP BY
  • Filtering after aggregation

• Ordering (2 tests)

  • ORDER BY on intermediate results
  • ORDER BY + LIMIT

• Pagination (3 tests)

  • LIMIT only
  • SKIP only
  • SKIP + LIMIT pagination

• Chaining (2 tests)

  • Multiple WITH clauses
  • WITH between MATCH clauses

• Edge cases (3 tests)

  • Empty result sets
  • Null value preservation
  • DISTINCT functionality

Known Issues Revealed

These tests intentionally expose bugs in the current implementation:

🐛 15/17 tests currently fail:

1. Column alias bug - Results use col_0, col_1 instead of actual aliases

   • Expected: {"name": "Alice"}
   • Actual: {"col_0": "Alice"}
   • Same regression that was just fixed for RETURN clause

2. DISTINCT not supported - Parser doesn't recognize DISTINCT in WITH

   WITH DISTINCT p.city AS city  -- Parser error

3. Aggregation functions fail - count(*), sum(), etc. cause errors

   TypeError: Cannot evaluate expression type: FunctionCall
   

4. Parser errors with complex patterns - Multiple MATCH+WITH+CREATE fails

   MATCH (p:Person)
   WITH p
   CREATE (p)-[:KNOWS]->(q)  -- Parser error

Value

This PR provides:

• ✅ Comprehensive test documentation for WITH clause behavior
• ✅ Clear baseline showing what works vs. what doesn't
• ✅ Actionable bug list for future work
• ✅ Safety net for refactoring and fixes

Test Results

17 tests collected
2 passed (12%)
15 failed (88%)

Expected behavior - These failures document real bugs that need fixing.

Next Steps

After this PR merges, follow-up work will:

1. Fix column alias preservation in WITH clause
2. Add DISTINCT support to WITH
3. Fix aggregation function evaluation in WITH
4. Improve parser to handle complex multi-part queries

Checklist

• Tests added for new functionality
• CHANGELOG updated
• Tests run locally (intentionally failing to expose bugs)
• Commit message follows conventions
• Small, focused PR (<300 lines)

---

Note: This PR is intended to document and expose bugs, not fix them. The test failures are expected and valuable.

Summary by CodeRabbit

• Tests
  • Added comprehensive integration test suite for WITH clause functionality, covering projection, filtering, aggregation, ordering, pagination, chaining, and edge cases.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)

Validation error: Unrecognized key(s) in object: 'python', 'ignore', 'tools', 'file_types', 'comments', 'thresholds', 'integrations', 'notifications'

⚙️ Configuration instructions

• Please see the configuration documentation for more information.
• You can also validate your configuration using the online YAML validator.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Walkthrough

Adds a comprehensive integration test suite for the WITH clause in GraphForge, covering basic behavior, filtering, aggregation, ordering, pagination, chaining, and edge cases across multiple test classes and scenarios.

Changes

Cohort / File(s)	Summary
WITH Clause Integration Tests tests/integration/test_with_clause.py	New test module with multiple test classes validating WITH clause functionality including projection, renaming, filtering, aggregation, ordering, pagination, chaining across multiple WITH clauses, and edge cases like empty results and null value preservation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: adding comprehensive integration tests for the WITH clause.
Description check	✅ Passed	The description is comprehensive and well-structured, covering motivation, test categories, known issues, and next steps, though it doesn't follow the exact template format.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feature/with-clause-integration-tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@tests/integration/test_with_clause.py`:
- Around line 18-40: The test creates a GraphForge and calls db.close() manually
which can leak resources if assertions fail; refactor to use a pytest fixture
(e.g., db) that constructs GraphForge(tmp_path / f"test_{uuid.uuid4().hex}.db"),
yields the instance, and calls database.close() after yield, then update
test_with_simple_projection to accept that db fixture and remove the explicit
GraphForge(...) and db.close() calls so cleanup always runs; add the fixture in
conftest.py or the module to centralize tmp_path usage and avoid hardcoded DB
names.

🧹 Nitpick comments (2)

tests/integration/test_with_clause.py (2)

245-246: Avoid f-string interpolation in execute() calls.

While safe here with loop counters, using f-strings in db.execute() demonstrates a pattern that could lead to injection vulnerabilities if used with external input. Consider using parameterized queries or batch operations instead.

♻️ Alternative approach

-for i in range(5):
-    db.execute(f"CREATE (p:Person {{id: {i}}})")
+for i in range(5):
+    db.create_node(labels=["Person"], id=i)

Or if the query syntax must be tested:

# At minimum, add a comment explaining why f-strings are safe here
for i in range(5):
    # Safe: i is an integer from range(), not external input
    db.execute(f"CREATE (p:Person {{id: {i}}})")

---

1-8: Consider adding xfail markers for expected failures.

The PR objectives document that 15 of 17 tests are expected to fail due to known bugs (alias preservation, DISTINCT support, aggregation evaluation, parser issues). Adding @pytest.mark.xfail decorators would:

1. Clearly document which failures are expected vs. new regressions
2. Allow the test suite to pass while still running these tests
3. Automatically detect when bugs are fixed (tests will fail with "XPASS")

📝 Example of documenting expected failures

class TestWithBasics:
    """Test basic WITH clause functionality."""

    `@pytest.mark.xfail`(reason="Known bug: aliases returned as col_0 instead of provided aliases")
    def test_with_simple_projection(self, tmp_path):
        """WITH should pass through selected variables."""
        # ...
    
    `@pytest.mark.xfail`(reason="Known bug: alias preservation")
    def test_with_renaming_variables(self, tmp_path):
        """WITH should allow renaming variables."""
        # ...

class TestWithAggregation:
    """Test WITH clause with aggregation."""

    `@pytest.mark.xfail`(reason="Known bug: Cannot evaluate FunctionCall expressions")
    def test_with_count_aggregation(self, tmp_path):
        """WITH should support count aggregation."""
        # ...

This provides clear documentation without changing test assertions.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Use fixtures or context managers for resource cleanup.

The pattern of calling db.close() at the end of each test has a critical flaw: if any assertion fails, close() won't be executed, potentially leaving resources open or causing state pollution between tests.

♻️ Recommended fix using pytest fixture

Create a fixture in conftest.py or at the module level:

`@pytest.fixture`
def db(tmp_path):
    """Provide a GraphForge instance with automatic cleanup."""
    database = GraphForge(tmp_path / f"test_{uuid.uuid4().hex}.db")
    yield database
    database.close()

Then update tests to use the fixture:

-def test_with_simple_projection(self, tmp_path):
+def test_with_simple_projection(self, db):
     """WITH should pass through selected variables."""
-    db = GraphForge(tmp_path / "test.db")
-
     # Create test data
     db.execute("CREATE (p:Person {name: 'Alice', age: 30})")
     ...
-    
-    db.close()

This ensures cleanup happens even when assertions fail and avoids conflicts from hardcoded database names.

🧰 Tools

🪛 GitHub Actions: Test Suite

[error] 35-35: KeyError: 'name' in test_with_simple_projection results

🤖 Prompt for AI Agents

In `@tests/integration/test_with_clause.py` around lines 18 - 40, The test creates
a GraphForge and calls db.close() manually which can leak resources if
assertions fail; refactor to use a pytest fixture (e.g., db) that constructs
GraphForge(tmp_path / f"test_{uuid.uuid4().hex}.db"), yields the instance, and
calls database.close() after yield, then update test_with_simple_projection to
accept that db fixture and remove the explicit GraphForge(...) and db.close()
calls so cleanup always runs; add the fixture in conftest.py or the module to
centralize tmp_path usage and avoid hardcoded DB names.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Test name doesn't match implementation.

The test is named test_with_where_on_computed_value and the docstring claims it tests "filtering on computed values," but the query doesn't compute anything—it just passes through p.score AS score. A computed value would involve an expression like p.score * 2 or p.score + p.bonus.

🔧 Proposed fix options

Option 1: Rename the test to match current behavior

-def test_with_where_on_computed_value(self, tmp_path):
-    """WITH should allow filtering on computed values."""
+def test_with_where_on_projected_value(self, tmp_path):
+    """WITH should allow filtering on projected property values."""

Option 2: Update the query to actually compute a value

     results = db.execute("""
         MATCH (p:Person)
-        WITH p.name AS name, p.score AS score
-        WHERE score >= 85
-        RETURN name, score
-        ORDER BY score DESC
+        WITH p.name AS name, p.score * 2 AS doubled_score
+        WHERE doubled_score >= 170
+        RETURN name, doubled_score
+        ORDER BY doubled_score DESC
     """)
     
     assert len(results) == 2
     assert results[0]["name"].value == "Alice"
-    assert results[0]["score"].value == 95
+    assert results[0]["doubled_score"].value == 190

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	def test_with_where_on_computed_value(self, tmp_path):
	"""WITH should allow filtering on computed values."""
	db = GraphForge(tmp_path / "test.db")
	db.execute("CREATE (p:Person {name: 'Alice', score: 95})")
	db.execute("CREATE (p:Person {name: 'Bob', score: 75})")
	db.execute("CREATE (p:Person {name: 'Charlie', score: 85})")
	# Compute value, then filter
	results = db.execute("""
	MATCH (p:Person)
	WITH p.name AS name, p.score AS score
	WHERE score >= 85
	RETURN name, score
	ORDER BY score DESC
	""")
	assert len(results) == 2
	assert results[0]["name"].value == "Alice"
	assert results[0]["score"].value == 95
	assert results[1]["name"].value == "Charlie"
	assert results[1]["score"].value == 85
	db.close()
	def test_with_where_on_projected_value(self, tmp_path):
	"""WITH should allow filtering on projected property values."""
	db = GraphForge(tmp_path / "test.db")
	db.execute("CREATE (p:Person {name: 'Alice', score: 95})")
	db.execute("CREATE (p:Person {name: 'Bob', score: 75})")
	db.execute("CREATE (p:Person {name: 'Charlie', score: 85})")
	# Compute value, then filter
	results = db.execute("""
	MATCH (p:Person)
	WITH p.name AS name, p.score AS score
	WHERE score >= 85
	RETURN name, score
	ORDER BY score DESC
	""")
	assert len(results) == 2
	assert results[0]["name"].value == "Alice"
	assert results[0]["score"].value == 95
	assert results[1]["name"].value == "Charlie"
	assert results[1]["score"].value == 85
	db.close()

🧰 Tools

🪛 GitHub Actions: Test Suite

[error] 125-125: KeyError: 'name' in test_with_where_on_computed_value results