tests: Consolidate and simplify Elixir tests #56
Conversation
WalkthroughCentralises test DB cleanup, removes several legacy Elixir tests, adds many new Elixir and Rust test suites (prepared statements, concurrency, streaming, replication, error handling), introduces Rust TestDbGuard RAII and Elixir TestHelpers, and makes minor docs/.gitignore/.beads edits. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @test/smoke_test.exs:
- Around line 76-80: The failing CI is due to formatting in the test block
around the EctoLibSql.connect/1 and EctoLibSql.handle_execute/4 assertions; run
mix format on test/smoke_test.exs (or run mix format for the repo) to apply the
expected formatting so the block containing {:ok, state} =
EctoLibSql.connect(state[:opts]), the Query struct construction
%EctoLibSql.Query{statement: "SELECT * FROM not_existing_table"}, and the assert
line are properly formatted to satisfy mix format --check-formatted.
- Around line 119-126: Run mix format on the test file to fix formatting issues:
format test/smoke_test.exs (or run mix format --check-formatted to see failures
and then mix format to apply fixes). Ensure the block around the
EctoLibSql.handle_execute and EctoLibSql.handle_commit calls follows standard
Elixir formatting (correct indentation, spacing, and line breaks) so that the
test compiles and passes mix format checks.
🧹 Nitpick comments (2)
test/smoke_test.exs (1)
104-126: Consider adding a transaction rollback test.Whilst the smoke tests are intentionally minimal, adding a basic transaction rollback scenario would strengthen confidence in core functionality. This would help catch issues early before running the more comprehensive test suites.
Based on learnings and coding guidelines, tests should cover transaction rollback scenarios.
Example rollback test
Add this test to the "basic transaction" describe block:
test "can rollback transaction", state do {:ok, state} = EctoLibSql.connect(state[:opts]) # Create table first create = %EctoLibSql.Query{ statement: "CREATE TABLE IF NOT EXISTS users (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT, email TEXT)" } {:ok, _query, _result, state} = EctoLibSql.handle_execute(create, [], [], state) # Begin transaction {:ok, _begin_result, state} = EctoLibSql.handle_begin([], state) # Insert data insert = %EctoLibSql.Query{statement: "INSERT INTO users (name, email) VALUES (?, ?)"} {:ok, _query, _result, state} = EctoLibSql.handle_execute(insert, ["Bob", "[email protected]"], [], state) # Rollback instead of commit {:ok, _rollback_result, state} = EctoLibSql.handle_rollback([], state) # Verify data was not persisted select = %EctoLibSql.Query{statement: "SELECT COUNT(*) FROM users"} {:ok, _query, result, _state} = EctoLibSql.handle_execute(select, [], [], state) assert [[0]] = result.rows endtest/prepared_statement_test.exs (1)
1-739: Consider adding concurrent prepared statement tests.Whilst the current test coverage is excellent, adding tests for concurrent usage of prepared statements would strengthen confidence in thread-safety and statement isolation. This could include scenarios where multiple processes use the same or different prepared statements simultaneously.
Based on coding guidelines, tests should cover concurrent operations where applicable.
Example concurrent test structure
@tag :concurrent test "concurrent prepared statement executions", %{state: state} do # Insert test data {:ok, _query, _result, state} = exec_sql(state, "INSERT INTO users (id, name, email) VALUES (?, ?, ?)", [ 1, "Alice", "[email protected]" ]) {:ok, _query, _result, state} = exec_sql(state, "INSERT INTO users (id, name, email) VALUES (?, ?, ?)", [ 2, "Bob", "[email protected]" ]) {:ok, stmt_id} = Native.prepare(state, "SELECT * FROM users WHERE id = ?") # Execute concurrently from multiple tasks tasks = Enum.map(1..10, fn _ -> Task.async(fn -> {:ok, result1} = Native.query_stmt(state, stmt_id, [1]) {:ok, result2} = Native.query_stmt(state, stmt_id, [2]) {result1, result2} end) end) # All tasks should complete successfully results = Enum.map(tasks, &Task.await/1) assert length(results) == 10 Native.close_stmt(stmt_id) end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.beads/last-touchedtest/ecto_libsql_test.exstest/error_demo_test.exstest/explain_simple_test.exstest/prepared_statement_test.exstest/smoke_test.exstest/statement_features_test.exstest/stmt_caching_performance_test.exs
💤 Files with no reviewable changes (4)
- test/error_demo_test.exs
- test/statement_features_test.exs
- test/ecto_libsql_test.exs
- test/explain_simple_test.exs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/prepared_statement_test.exstest/stmt_caching_performance_test.exstest/smoke_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/prepared_statement_test.exstest/stmt_caching_performance_test.exstest/smoke_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/prepared_statement_test.exstest/stmt_caching_performance_test.exstest/smoke_test.exs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
test/prepared_statement_test.exstest/stmt_caching_performance_test.exstest/smoke_test.exs
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries to achieve ~10-15x performance improvement
Applied to files:
test/prepared_statement_test.exstest/stmt_caching_performance_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
test/prepared_statement_test.exstest/smoke_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/prepared_statement_test.exstest/stmt_caching_performance_test.exstest/smoke_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto/adapters/libsql/connection.ex : Add SQLite function support by defining expression handlers for native SQLite functions
Applied to files:
test/prepared_statement_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
Applied to files:
test/prepared_statement_test.exstest/smoke_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/smoke_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
test/smoke_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Use the Repository pattern for data access in EctoLibSql applications
Applied to files:
test/smoke_test.exs
🧬 Code graph analysis (2)
test/prepared_statement_test.exs (3)
test/stmt_caching_performance_test.exs (1)
exec_sql(15-18)test/savepoint_test.exs (1)
exec_sql(14-17)lib/ecto_libsql/native.ex (3)
prepare(951-959)execute_stmt(991-1020)close_stmt(1080-1082)
test/smoke_test.exs (1)
lib/ecto_libsql.ex (3)
disconnect(131-133)handle_execute(148-188)handle_begin(312-317)
🪛 GitHub Actions: CI
test/smoke_test.exs
[error] 76-80: Mix format --check-formatted failed. The following regions in test/smoke_test.exs are not properly formatted.
[error] 119-126: Mix format --check-formatted failed. The following regions in test/smoke_test.exs are not properly formatted.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
.beads/last-touched (1)
1-1: Metadata update looks good.The metadata tracking update is straightforward with no concerns.
test/stmt_caching_performance_test.exs (1)
1-8: LGTM! Clear module rename and documentation.The module rename from
BenchmarkTesttoPerformanceTestbetter reflects the test's purpose, and the added documentation clearly explains that these tests verify performance characteristics without being strict gates. The documentation follows British/Australian English conventions as required.test/prepared_statement_test.exs (3)
1-11: Excellent test module structure and documentation.The module is well-organised with clear documentation linking to Phase 1 roadmap features. The comprehensive test coverage across preparation, execution, introspection, lifecycle, and performance aspects demonstrates thorough validation of prepared statement functionality.
313-366: Well-implemented automatic reset tests.These tests properly verify that statement caching handles reset automatically between executions, ensuring parameter bindings don't leak across calls. The verification of row counts confirms correctness.
588-738: Comprehensive binding behaviour tests with appropriate performance validation.The tests ported from ecto_sql provide excellent coverage of binding behaviour, parameter type handling, and performance characteristics. The performance test is appropriately tagged as
:flakyand includes informative output whilst always passing, which is the correct approach for CI environments where performance can vary significantly.The explicit notes about CI variability and the focus on API functionality rather than strict performance gates show good engineering judgement.
- Adds comprehensive rollback scenario alongside existing commit test - Verifies rollback correctly undoes in-transaction changes - Confirms pre-transaction data is preserved - Strengthens confidence in core transaction functionality - All 8 smoke tests pass
Adds 5 comprehensive concurrent tests to verify thread-safety and statement isolation: 1. Multiple processes using different prepared statements concurrently - Verifies independent statements don't interfere with each other - Tests both ID-based and name-based lookups in parallel 2. Single prepared statement safely used by multiple processes - Confirms statement reuse across concurrent tasks is safe - Validates parameter isolation and correct results - Tests with Process.sleep to simulate real-world timing 3. Concurrent writes with prepared statements maintain consistency - Verifies INSERT operations through prepared statements are atomic - Confirms final count matches expected value (1 initial + 5 new = 6) - Tests concurrent write scenarios 4. Prepared statements handle parameter isolation across concurrent tasks - Verifies parameters from one task don't leak to another - Tests custom parameter values with detailed assertions - Validates both parameter AND data isolation 5. Prepared statements maintain isolation when reset concurrently - Tests explicit reset_stmt() during concurrent access - Verifies statement can be reset while other tasks use it - Confirms isolation is maintained after reset All 44 tests pass including these new concurrent scenarios.
ocean
force-pushed
the
consolidate-tests
branch
from
e57c504 to
cd0209f
Compare
ocean
force-pushed
the
consolidate-tests
branch
from
c8bd634 to
737517d
Compare
…REPORT Created 9 new issues based on TEST_AUDIT_REPORT.md recommendations: **P2 (Medium priority - should do soon)**: - el-doo: Test cursor streaming with large result sets - el-fd8: Test connection pool behavior under load - el-d63: Test connection error recovery - el-crt: Test savepoint + replication interaction - el-wtl: Test JSONB binary format operations - el-d3o: Add Rust tests for error scenarios **P3 (Low priority - nice to have)**: - el-cbv: Add performance benchmark tests - el-1p2: Document test layering strategy - el-v3v: Reduce redundant parameter binding tests These issues capture all missing test coverage areas identified in TEST_AUDIT_REPORT.md
ocean
force-pushed
the
consolidate-tests
branch
from
56e148a to
5b6afe8
Compare
… load - test/savepoint_replication_test.exs: 5 tests for savepoint+replication interaction * Basic savepoint operations with replica sync enabled * Savepoint rollback with remote sync preservation * Nested savepoints with remote sync * Error recovery patterns with savepoints * Constraint violation handling - test/pool_load_test.exs: 8 tests for concurrent connection behavior * Multiple concurrent independent connections * Rapid burst handling * Long-running transaction robustness * Error recovery and isolation under load * Resource cleanup (prepared statements) * Transaction isolation guarantees Both test suites designed to test critical integration points without over-testing. Tests are isolated, skippable (replication tests), and pass 100% (13/13 tests). Closes: el-crt, el-fd8
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @test/savepoint_replication_test.exs:
- Around line 85-92: Run mix format to fix formatting in
test/savepoint_replication_test.exs; specifically format the block that calls
EctoLibSql.handle_execute and the subsequent assertion (the code using "SELECT
COUNT(*) FROM #{table}", the variables state/result, and the assert [[1]] =
result.rows) so it adheres to project formatting rules (e.g., run `mix format`
or apply your editor’s formatter to that file).
🧹 Nitpick comments (1)
test/savepoint_replication_test.exs (1)
1-19: Minor spelling issue: Use British/Australian EnglishPer the coding guidelines, British/Australian English should be used in documentation and comments. Line 7 uses "behavior" which should be "behaviour".
Suggested fix
Focused on critical integration scenarios: 1. Savepoints work correctly in replica mode with sync enabled - 2. Savepoint rollback doesn't interfere with remote sync + 2. Savepoint rollback doesn't interfere with remote synchronisation 3. Error recovery with savepoints in replicated transactions
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.beads/last-touched.gitignoreCLAUDE.mdtest/pool_load_test.exstest/prepared_statement_test.exstest/savepoint_replication_test.exstest/smoke_test.exs
🚧 Files skipped from review as they are similar to previous changes (1)
- .beads/last-touched
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/prepared_statement_test.exstest/pool_load_test.exstest/savepoint_replication_test.exstest/smoke_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/prepared_statement_test.exstest/pool_load_test.exstest/savepoint_replication_test.exstest/smoke_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/prepared_statement_test.exstest/pool_load_test.exstest/savepoint_replication_test.exstest/smoke_test.exs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
test/prepared_statement_test.exstest/pool_load_test.exstest/savepoint_replication_test.exsCLAUDE.mdtest/smoke_test.exs
**/*.{md,markdown}
📄 CodeRabbit inference engine (CLAUDE.md)
Use British/Australian English for all documentation files (except SQL keywords and compatibility requirements)
Files:
CLAUDE.md
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries to achieve ~10-15x performance improvement
Applied to files:
test/prepared_statement_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
test/prepared_statement_test.exstest/pool_load_test.exstest/savepoint_replication_test.exstest/smoke_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/prepared_statement_test.exstest/pool_load_test.exstest/savepoint_replication_test.exstest/smoke_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto/adapters/libsql/connection.ex : Add SQLite function support by defining expression handlers for native SQLite functions
Applied to files:
test/prepared_statement_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
Applied to files:
test/prepared_statement_test.exsCLAUDE.mdtest/smoke_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
test/prepared_statement_test.exstest/pool_load_test.exstest/savepoint_replication_test.exstest/smoke_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/pool_load_test.exstest/smoke_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use savepoints for partial rollback within transactions to enable error recovery patterns in batch operations
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/turso_remote_test.exs : Turso remote tests should be skipped by default if credentials are missing and only run when explicitly needed due to cost implications
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to AGENTS.md : Update AGENTS.md API reference when adding new public NIF functions or Ecto features that users should know about
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to README.md : Update README.md when adding new features or changing user-facing functionality
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Use Beads (`bd` command) for multi-session issue tracking with persistence across sessions and for work with dependencies between tasks
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Add `#[rustler::nif]` attribute to NIF functions—modern Rustler auto-detects them without manual registration
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : Always format Rust code before committing using `cargo fmt`
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs,rs,md} : When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with `git commit`, clean up stashes, and provide context for next session
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Always format Elixir code before committing using `mix format --check-formatted`
Applied to files:
test/smoke_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Use the Repository pattern for data access in EctoLibSql applications
Applied to files:
test/smoke_test.exs
🧬 Code graph analysis (3)
test/pool_load_test.exs (2)
lib/ecto_libsql.ex (2)
handle_execute(148-188)disconnect(131-133)lib/ecto_libsql/native.ex (5)
begin(828-838)commit(853-857)prepare(951-959)execute_stmt(991-1020)close_stmt(1080-1082)
test/savepoint_replication_test.exs (2)
lib/ecto_libsql.ex (1)
handle_execute(148-188)lib/ecto_libsql/native.ex (5)
begin(828-838)create_savepoint(1805-1807)release_savepoint_by_name(1836-1838)commit(853-857)rollback_to_savepoint_by_name(1877-1879)
test/smoke_test.exs (1)
lib/ecto_libsql.ex (4)
disconnect(131-133)handle_execute(148-188)handle_begin(312-317)handle_rollback(350-358)
🪛 GitHub Actions: CI
test/savepoint_replication_test.exs
[error] Mix format check failed. The following files are not formatted.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (28)
.gitignore (1)
41-43: LGTM!Appropriate addition to ignore local beads viewer configuration and caches. This aligns with the project's use of Beads for issue tracking as documented in CLAUDE.md.
test/smoke_test.exs (5)
1-14: LGTM!Well-structured smoke test module with clear documentation referencing specialised test files for comprehensive coverage. The use of
doctest EctoLibSqlensures documentation examples stay valid.
15-50: LGTM!Solid test isolation strategy with unique database files per test and thorough cleanup in both
setupandon_exitcallbacks. The cleanup of WAL and SHM files alongside the main database file is a good practice for SQLite.
52-66: LGTM!Basic connectivity tests cover the essential lifecycle operations (connect, ping, disconnect) with proper pattern matching.
68-104: LGTM!Good coverage of query scenarios including error handling for invalid SQL. The multi-statement test (lines 83-103) verifies compound statement execution which is important for SQLite compatibility.
106-171: LGTM!Excellent transaction test coverage with proper verification of rollback semantics. The rollback test (lines 131-170) correctly validates that:
- Pre-transaction data (Bob) persists
- Transaction data (Charlie) is rolled back
- Final count confirms isolation
This aligns with the coding guidelines for transaction rollback scenarios.
test/savepoint_replication_test.exs (3)
21-62: LGTM!Good setup with proper test isolation via unique database names and conditional Turso connection based on environment variables. The try/rescue in
on_exitis appropriate for handling cleanup of potentially already-closed connections.
64-183: LGTM!Comprehensive savepoint tests covering:
- Basic savepoint operations with replica sync (lines 65-93)
- Rollback preserving outer transaction data (lines 95-132)
- Nested savepoints with partial rollback (lines 134-183)
The tests properly verify data integrity after each operation.
203-214: The test correctly uses the state from the error tuple for savepoint recovery.The
{:error, _reason, trx_state}tuple at line 211 contains the original, unmodified transaction state. Error conditions do not corrupt the state; it remains safe to use for subsequent operations including savepoint rollback. The Rust layer validates transaction ownership (checkingentry.conn_id == conn_id) before executingrollback_to_savepoint_by_name, and the transaction is automatically re-inserted after the operation. This test correctly implements the error recovery pattern using savepoints as documented.CLAUDE.md (3)
49-49: LGTM!Good addition of the Table of Contents entry for easy navigation to the Beads section.
262-262: LGTM!Minor punctuation improvement for readability.
298-304: LGTM!Useful best practices section with actionable guidance for session workflow. The bullet points provide clear, concise instructions for using Beads effectively.
test/pool_load_test.exs (7)
1-18: LGTM!Well-documented test module covering critical concurrent connection scenarios. The moduledoc clearly outlines the test objectives.
19-41: LGTM!Good test isolation with unique database per test run. Each test creates its own connections, and cleanup handles the setup connection appropriately.
43-109: LGTM!Good concurrent connection tests verifying that multiple independent connections can successfully execute operations without interference. The verification of final row counts (lines 80, 107) ensures data integrity.
111-177: LGTM!Good tests for long-running operations with appropriate use of
busy_timeoutoption and verification that concurrent transactions complete successfully. TheProcess.sleepcalls simulate realistic workloads.
179-267: LGTM!Excellent error recovery tests demonstrating connection resilience. The tests verify that:
- A connection recovers after a query error (lines 179-216)
- Multiple connections recover independently (lines 218-267)
This matches the coding guidelines requirement for error case coverage.
270-308: LGTM!Good test for prepared statement lifecycle under concurrent load. Properly exercises prepare, execute, and close operations across multiple concurrent tasks.
311-355: LGTM!Solid transaction isolation test with concurrent transactions and slight delays to increase operation overlap. Verification of final count confirms all transactions committed successfully without interference.
test/prepared_statement_test.exs (9)
313-339: LGTM!Good test for statement reuse with automatic binding reset. The test properly verifies that prepared statements can be executed multiple times without re-preparation, and that all insertions succeed.
341-425: LGTM!Comprehensive reset tests covering:
- Automatic binding reset between executions (lines 341-366)
- Explicit
reset_stmtusage (lines 368-396)- Multiple consecutive resets (lines 398-419)
- Error handling for invalid statements (lines 421-424)
Good coverage of both happy path and error cases as per coding guidelines.
427-507: LGTM!Excellent column metadata tests covering:
- Basic column retrieval with origin names and declared types
- Aliased columns preserving alias names
- Expression columns (COUNT, MAX)
- Error handling for invalid statements
509-586: LGTM!Comprehensive parameter introspection tests covering all SQLite parameter styles:
- Colon-style (
:name)- Dollar-style (
$name)- At-style (
@name)- Positional (
?)- Mixed positional and named
This is valuable for ensuring the adapter correctly handles various SQL authoring patterns.
740-784: LGTM!Good concurrent access test verifying that multiple processes can safely use different prepared statements simultaneously. The verification that both queries return identical data (line 771) is a nice touch.
786-830: LGTM!Important test validating that a single prepared statement can be safely shared across multiple processes. The verification of data integrity after concurrent access (lines 825-826) ensures the statement remains in a consistent state.
832-874: LGTM!Good test for concurrent write operations using prepared statements. Verifies that multiple concurrent INSERT operations maintain data consistency and the final count is correct.
876-916: LGTM!Critical test for parameter isolation across concurrent tasks. The assertion at lines 901-902 explicitly verifies no parameter contamination occurs between concurrent executions.
918-972: LGTM!Excellent edge case test for concurrent reset operations. Testing
reset_stmtduring concurrent access is important for validating thread safety of the reset functionality.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/json_helpers_test.exs (2)
5-25: Addon_exitdisconnect to avoid leaking connections across a large suite.
This suite is big enough that leaving connections to GC can become flaky.Proposed fix
setup do {:ok, state} = EctoLibSql.connect(database: ":memory:") + on_exit(fn -> EctoLibSql.disconnect([], state) end) # Create a test table with JSON columns {:ok, _, _, state} = EctoLibSql.handle_execute( """ CREATE TABLE IF NOT EXISTS json_test ( id INTEGER PRIMARY KEY, data TEXT, data_jsonb BLOB, metadata TEXT ) """, [], [], state ) {:ok, state: state} end
288-305: Relax JSONB “must be smaller” assertions to avoid cross-version flakiness.
JSONB being smaller is common, but not a stable guarantee; strict</<=can fail depending on implementation details.Proposed fix
test "converts to JSONB binary format", %{state: state} do json = ~s({"a":1}) {:ok, result} = JSON.convert(state, json, :jsonb) # Should be binary assert is_binary(result) - # JSONB is smaller/different than text JSON - assert byte_size(result) < byte_size(json) + # JSONB is binary; size characteristics are implementation-dependent + assert byte_size(result) > 0 end- test "JSONB storage is 5-10% smaller than text JSON", %{state: state} do + test "JSONB storage is not pathologically larger than text JSON", %{state: state} do # Create a reasonably sized JSON object json_text = ~s({"user":{"id":1,"name":"Alice","email":"[email protected]","profile":{"bio":"Software engineer","location":"San Francisco","interests":["Elixir","Rust","Go"]},"settings":{"theme":"dark","notifications":true,"language":"en"}}}) # Convert to JSONB {:ok, jsonb} = JSON.convert(state, json_text, :jsonb) text_size = byte_size(json_text) jsonb_size = byte_size(jsonb) - # JSONB should be smaller (5-10% is typical, but may vary) - # We check for general size improvement (not overly strict) - assert jsonb_size <= text_size, - "JSONB (#{jsonb_size} bytes) should be <= text JSON (#{text_size} bytes)" - - # Most of the time JSONB is noticeably smaller - # but we don't enforce a strict percentage due to variation + # Don’t assert “must be smaller”; just ensure it’s within a reasonable bound. + assert jsonb_size <= trunc(text_size * 1.5), + "JSONB (#{jsonb_size} bytes) unexpectedly large vs text JSON (#{text_size} bytes)" endAlso applies to: 786-805
🤖 Fix all issues with AI agents
In @native/ecto_libsql/src/tests/error_handling_tests.rs:
- Around line 485-496: The test test_create_db_invalid_permissions uses a
NUL-containing path ("\0invalid\0path.db") which is not portable to Windows;
guard the test with a platform cfg or use platform-specific invalid-path logic:
wrap the test function (test_create_db_invalid_permissions) with #[cfg(unix)] so
it only runs on Unix, or implement conditional behavior inside the test using
cfg!(windows) to choose a different invalid-path strategy, keeping the assertion
that Builder::new_local(invalid_path).build().await must not panic.
In @test/connection_recovery_test.exs:
- Around line 8-11: Add an on_exit hook in the setup block that calls the
disconnect function for the in-memory connection to avoid leaking resources:
after {:ok, state} = EctoLibSql.connect(database: ":memory:") register on_exit
and call EctoLibSql.disconnect(state) (or the appropriate teardown function for
EctoLibSql) so the connection is cleaned up when the test finishes, then return
{:ok, state: state}.
In @test/cursor_streaming_large_test.exs:
- Around line 33-110: Tests drop updated state from insert_rows/4 and call
helpers that use a hard-coded query instead of the declared cursor, causing
state and query/cursor mismatches; fix by threading the returned state through
every insert_rows/4 call and into subsequent calls to EctoLibSql.handle_declare
and your fetch helpers, and change fetch_all_rows and fetch_remaining_rows (and
any helper that calls EctoLibSql.handle_fetch) to accept and use the declared
Query/cursor returned by handle_declare rather than using a new hard-coded
Query; ensure every test captures the updated state (e.g., state =
insert_rows(state, ...)) and passes that same state and the declared
cursor/query into fetch_* helpers so handle_fetch/4 receives the exact cursor
and state that handle_declare produced.
In @test/pool_load_test.exs:
- Around line 43-109: The concurrency tests are flaky due to SQLite writer
contention and default Task await timeouts; update each concurrent test that
spawns Task.async to pass a busy_timeout when connecting (e.g., call
EctoLibSql.connect(database: test_db, busy_timeout: 30_000) or similar to the
connect API) and replace Task.await_many(tasks) with Task.await_many(tasks,
30_000) so tasks have a longer timeout; apply this pattern in the described
tests that call EctoLibSql.connect and Task.await_many (including the blocks
using EctoLibSql.handle_execute and Task.await_many at the shown ranges).
In @test/prepared_statement_test.exs:
- Around line 918-960: The test allows empty or nil results after resetting the
prepared statement, masking regressions; tighten the assertions in the "prepared
statements maintain isolation when reset concurrently" test so that after
calling Native.reset_stmt(state, stmt_id) a subsequent Native.query_stmt(state,
stmt_id, [task_num + 5]) must return the row inserted earlier (IDs 6..10), not
[] or nil. Replace the lenient case block with a strict assertion that
result2.rows contains a single row whose id equals task_num + 5 and whose
name/email match "User#{task_num + 5}" and "user#{task_num + 5}@example.com",
and remove the unreachable new_id == nil branch.
🧹 Nitpick comments (10)
test/savepoint_replication_test.exs (1)
1-62: Skip + teardown strategy looks sound for remote-cost control.
One small hardening:File.rm/1returns{:error, :enoent}and is currently ignored; if you want quieter teardown, useFile.rm/1+ ignore return (explicit) orFile.rm/1wrapped in a helper.test/json_helpers_test.exs (1)
467-545::skipdoesn’t skip tests; considerExUnitskipping so it’s visible.
Right now those branches “pass” silently. If that’s intended, all good; if not, useskip/1(or tag) so CI output is explicit.Also applies to: 647-686
native/ecto_libsql/src/tests/error_handling_tests.rs (2)
1-28: Module intent + unwrap exception are consistent with test guidance.
One optional tidy-up:setup_test_db/cleanup_test_dbare duplicated withnative/ecto_libsql/src/tests/integration_tests.rs; consider factoring into a shared test util if you keep growing test modules. Based on learnings, unwrap usage in tests is acceptable.
239-288: “No panic” tests should still assert something meaningful where feasible.
For parameter mismatch onexecute/preparedquery, consider assertingis_err()(or checking a stable error class) if libsql behaviour is expected to be consistent in your supported versions—otherwise these tests can pass while masking regressions.Also applies to: 451-479
test/pool_load_test.exs (1)
270-309: Ensure statement/connection cleanup even on failures inside tasks.
If any{:ok, ...} = ...match fails, the task crashes and may skipclose_stmt/disconnect. Wrapping task bodies withtry/afterwould make this more robust under load.test/connection_recovery_test.exs (1)
13-451: Assert the error tuple in “failure” steps to make recovery guarantees explicit.
E.g. replace_result = ...withassert {:error, _reason, _state} = ...(or the exact shape you expect). That keeps the tests honest and aligns with the “handle errors via pattern matching” guidance. Based on learnings, this is worth tightening.test/cursor_streaming_large_test.exs (3)
367-381:insert_rows/4is O(n) round-trips; consider a faster insert path if CI gets slow.
10k individual inserts (plus other tests) can drag. A prepared insert statement (or a single multi-rowINSERT) would keep coverage while reducing runtime. Based on learnings, prepared statements are a good fit for repeated queries.
383-427: Avoid non-tail recursion +++for ID collection (cheap to harden now).
fetch_all_ids/3builds lists withids ++ ..., which is quadratic; and several helpers aren’t tail-recursive. For current sizes it’s probably fine, but a small accumulator-based helper keeps this robust.
398-412:fetch_remaining_rows/3duplicatesfetch_all_rows/3.
Oncefetch_all_rows/4is query-aware, you can dropfetch_remaining_rows/3(or make it a thin wrapper) to reduce maintenance surface.test/prepared_statement_test.exs (1)
740-784: Concurrency suite looks useful; consider tagging/timeouts to reduce CI flake risk.
Task.await_many(..., 5000)can be tight under load; a slightly higher timeout and/or a@tag :concurrency/:slowwould make failures more actionable.Also applies to: 786-830, 832-874, 876-916
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
AGENTS.mdnative/ecto_libsql/src/tests/error_handling_tests.rsnative/ecto_libsql/src/tests/integration_tests.rsnative/ecto_libsql/src/tests/mod.rsnative/ecto_libsql/src/transaction.rstest/connection_recovery_test.exstest/cursor_streaming_large_test.exstest/json_helpers_test.exstest/pool_load_test.exstest/prepared_statement_test.exstest/savepoint_replication_test.exs
✅ Files skipped from review due to trivial changes (2)
- AGENTS.md
- native/ecto_libsql/src/transaction.rs
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Always format Rust code before committing usingcargo fmt
NEVER use.unwrap()in production Rust code—usesafe_lockhelpers and proper error handling instead (tests may use.unwrap()for simplicity)
Files:
native/ecto_libsql/src/tests/mod.rsnative/ecto_libsql/src/tests/error_handling_tests.rsnative/ecto_libsql/src/tests/integration_tests.rs
native/ecto_libsql/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
native/ecto_libsql/src/**/*.rs: Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Validate transaction ownership by checking thatentry.conn_id == conn_idbefore performing transaction operations
Add#[rustler::nif]attribute to NIF functions—modern Rustler auto-detects them without manual registration
Files:
native/ecto_libsql/src/tests/mod.rsnative/ecto_libsql/src/tests/error_handling_tests.rsnative/ecto_libsql/src/tests/integration_tests.rs
native/ecto_libsql/src/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
native/ecto_libsql/src/tests/mod.rsnative/ecto_libsql/src/tests/error_handling_tests.rsnative/ecto_libsql/src/tests/integration_tests.rs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
native/ecto_libsql/src/tests/mod.rsnative/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exstest/json_helpers_test.exstest/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/savepoint_replication_test.exsnative/ecto_libsql/src/tests/integration_tests.rs
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/pool_load_test.exstest/json_helpers_test.exstest/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/savepoint_replication_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/pool_load_test.exstest/json_helpers_test.exstest/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/savepoint_replication_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/pool_load_test.exstest/json_helpers_test.exstest/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/savepoint_replication_test.exs
🧠 Learnings (25)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*.exs : Tests may use `.unwrap()` for simplicity when handling test data and setup
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
native/ecto_libsql/src/tests/mod.rsnative/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exstest/json_helpers_test.exstest/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/savepoint_replication_test.exsnative/ecto_libsql/src/tests/integration_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
native/ecto_libsql/src/tests/mod.rsnative/ecto_libsql/src/tests/error_handling_tests.rsnative/ecto_libsql/src/tests/integration_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Add `#[rustler::nif]` attribute to NIF functions—modern Rustler auto-detects them without manual registration
Applied to files:
native/ecto_libsql/src/tests/mod.rsnative/ecto_libsql/src/tests/error_handling_tests.rsnative/ecto_libsql/src/tests/integration_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
Applied to files:
native/ecto_libsql/src/tests/mod.rsnative/ecto_libsql/src/tests/error_handling_tests.rstest/prepared_statement_test.exsnative/ecto_libsql/src/tests/integration_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Applied to files:
native/ecto_libsql/src/tests/mod.rsnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Validate and document unsupported Ecto features by returning `:unsupported` atom errors in Rust implementations with clear documentation and comprehensive tests
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Lock Arc<Mutex<T>> using `safe_lock_arc(&client, "function_name client")?` instead of `.unwrap()` to prevent panics
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exstest/json_helpers_test.exstest/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/connection.rs : Lock a registry using `safe_lock(®ISTRY, "function_name context")?` instead of `.unwrap()` to prevent panics
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto/adapters/libsql.ex : Implement type loaders for converting SQLite values to Ecto types using `loaders/2` function
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto/adapters/libsql/connection.ex : Add SQLite function support by defining expression handlers for native SQLite functions
Applied to files:
test/pool_load_test.exstest/prepared_statement_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/pool_load_test.exstest/connection_recovery_test.exstest/savepoint_replication_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : JSON functions (extract, type, set, replace, insert, remove, patch) should be used for efficient JSON/JSONB manipulation in LibSQL 3.45.1+
Applied to files:
test/json_helpers_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use cursor streaming for large result sets to avoid loading entire datasets into memory
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*.exs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/connection_recovery_test.exstest/savepoint_replication_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use savepoints for partial rollback within transactions to enable error recovery patterns in batch operations
Applied to files:
test/connection_recovery_test.exstest/savepoint_replication_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Use the Repository pattern for data access in EctoLibSql applications
Applied to files:
test/connection_recovery_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries to achieve ~10-15x performance improvement
Applied to files:
test/prepared_statement_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Always format Elixir code before committing using `mix format --check-formatted`
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-08T04:56:27.393Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.393Z
Learning: Applies to priv/repo/migrations/*.exs : Use STRICT tables in migrations for type safety when database schema stability is critical
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/turso_remote_test.exs : Turso remote tests should be skipped by default if credentials are missing and only run when explicitly needed due to cost implications
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
native/ecto_libsql/src/tests/integration_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
native/ecto_libsql/src/tests/integration_tests.rs
🧬 Code graph analysis (5)
native/ecto_libsql/src/tests/mod.rs (1)
native/ecto_libsql/src/lib.rs (1)
tests(30-30)
native/ecto_libsql/src/tests/error_handling_tests.rs (3)
native/ecto_libsql/src/tests/integration_tests.rs (4)
setup_test_db(14-16)cleanup_test_db(18-20)row(108-108)row(281-281)lib/ecto_libsql/query.ex (1)
to_string(49-51)native/ecto_libsql/src/transaction.rs (1)
drop(141-150)
test/pool_load_test.exs (2)
lib/ecto_libsql.ex (2)
handle_execute(148-188)disconnect(131-133)lib/ecto_libsql/native.ex (5)
begin(828-838)commit(853-857)prepare(951-959)execute_stmt(991-1020)close_stmt(1080-1082)
test/connection_recovery_test.exs (2)
lib/ecto_libsql.ex (2)
handle_begin(312-317)handle_rollback(350-358)lib/ecto_libsql/native.ex (5)
prepare(951-959)batch(1208-1212)create_savepoint(1805-1807)rollback_to_savepoint_by_name(1877-1879)reset(1312-1314)
test/savepoint_replication_test.exs (2)
lib/ecto_libsql.ex (2)
handle_execute(148-188)disconnect(131-133)lib/ecto_libsql/native.ex (4)
begin(828-838)create_savepoint(1805-1807)release_savepoint_by_name(1836-1838)rollback_to_savepoint_by_name(1877-1879)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (7)
native/ecto_libsql/src/tests/integration_tests.rs (1)
241-241: Spelling change aligns with AU/UK docs guideline.
No functional impact; looks good.test/savepoint_replication_test.exs (1)
64-251: Coverage hits the key savepoint/rollback/error-recovery paths.
The assertions are appropriately outcome-focused (rows/counts) rather than brittle error-string matching. Based on learnings, this is in the right direction for savepoint recovery patterns.native/ecto_libsql/src/tests/mod.rs (1)
6-10: Test module wiring looks correct.test/pool_load_test.exs (1)
19-41: Setup/teardown is good (per-test DB + cleanup).
Nice that this isolates state and cleans WAL/SHM artefacts.test/cursor_streaming_large_test.exs (1)
5-8: Guidelines check: en-AU spelling is good here; please also ensure formatting gate passes.
Comments read en-AU already; just make suremix format --check-formattedis clean before merge.Also applies to: 363-366
test/prepared_statement_test.exs (2)
313-586: Nice coverage expansion for reset, metadata, and parameter introspection.
The new scenarios are well-targeted and align with the test coverage learnings (happy path + edge/error cases).
13-17: Repo guideline mismatch: “camelCase function names in Elixir” vs current snake_case helpers.
This file uses idiomatic snake_case (exec_sql/3etc.). If the guideline is intentional, it should be applied consistently; if it’s a typo, it should be corrected to avoid churn.Also applies to: 313-425
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @TEST_AUDIT_REPORT.md:
- Line 3: Update the document header's Date value from "2024-01-08" to the
correct PR creation date "2026-01-08" by editing the Date line in
TEST_AUDIT_REPORT.md so the header matches the actual PR date.
🧹 Nitpick comments (3)
TEST_AUDIT_REPORT.md (2)
199-202: Add language specifier to fenced code block.Per markdownlint, fenced code blocks should have a language specified for proper syntax highlighting.
-``` +```text explain_query_test.exs: 262 lines, uses full Ecto setup explain_simple_test.exs: 115 lines, simpler test setup--- `453-458`: **Add language specifiers to directory tree code blocks.** The directory tree blocks at lines 453 and 462 lack language specifiers. Consider using `text` or `plaintext` for consistency with markdown linting. </blockquote></details> <details> <summary>test/cursor_streaming_large_test.exs (1)</summary><blockquote> `317-416`: **Consider consolidating similar helper functions.** The helper functions `fetch_all_rows`, `fetch_remaining_rows`, `fetch_all_ids`, `fetch_all_binary_rows`, `fetch_all_computed_rows`, `fetch_all_group_rows`, and `fetch_all_distinct_rows` share a similar pattern. Consider extracting a generic fetch function that accepts a row transformer. <details> <summary>♻️ Suggested refactor</summary> ```elixir defp fetch_all_with_transform(state, cursor, query, opts, transform_fn) do case EctoLibSql.handle_fetch(query, cursor, opts, state) do {:cont, result, next_state} -> transformed = transform_fn.(result.rows) transformed ++ fetch_all_with_transform(next_state, cursor, query, opts, transform_fn) {:halt, result, _state} -> transform_fn.(result.rows) end end # Usage examples: defp fetch_all_rows(state, cursor, query, opts) do fetch_all_with_transform(state, cursor, query, opts, &length/1) |> Enum.sum() end defp fetch_all_ids(state, cursor, query, opts) do fetch_all_with_transform(state, cursor, query, opts, fn rows -> Enum.map(rows, fn [id] -> id end) end) end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
TEST_AUDIT_REPORT.mdTEST_COVERAGE_ISSUES_CREATED.mdnative/ecto_libsql/src/tests/error_handling_tests.rstest/connection_recovery_test.exstest/cursor_streaming_large_test.exstest/pool_load_test.exstest/prepared_statement_test.exs
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{md,markdown}
📄 CodeRabbit inference engine (CLAUDE.md)
Use British/Australian English for all documentation files (except SQL keywords and compatibility requirements)
Files:
TEST_AUDIT_REPORT.mdTEST_COVERAGE_ISSUES_CREATED.md
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
TEST_AUDIT_REPORT.mdTEST_COVERAGE_ISSUES_CREATED.mdnative/ecto_libsql/src/tests/error_handling_tests.rstest/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/pool_load_test.exs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Always format Rust code before committing usingcargo fmt
NEVER use.unwrap()in production Rust code—usesafe_lockhelpers and proper error handling instead (tests may use.unwrap()for simplicity)
Files:
native/ecto_libsql/src/tests/error_handling_tests.rs
native/ecto_libsql/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
native/ecto_libsql/src/**/*.rs: Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Validate transaction ownership by checking thatentry.conn_id == conn_idbefore performing transaction operations
Add#[rustler::nif]attribute to NIF functions—modern Rustler auto-detects them without manual registration
Files:
native/ecto_libsql/src/tests/error_handling_tests.rs
native/ecto_libsql/src/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
native/ecto_libsql/src/tests/error_handling_tests.rs
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/pool_load_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/pool_load_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/pool_load_test.exs
🧠 Learnings (20)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*.exs : Tests may use `.unwrap()` for simplicity when handling test data and setup
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
TEST_AUDIT_REPORT.mdTEST_COVERAGE_ISSUES_CREATED.mdnative/ecto_libsql/src/tests/error_handling_tests.rstest/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rstest/cursor_streaming_large_test.exstest/connection_recovery_test.exstest/prepared_statement_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Add `#[rustler::nif]` attribute to NIF functions—modern Rustler auto-detects them without manual registration
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rstest/prepared_statement_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Validate and document unsupported Ecto features by returning `:unsupported` atom errors in Rust implementations with clear documentation and comprehensive tests
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Lock Arc<Mutex<T>> using `safe_lock_arc(&client, "function_name client")?` instead of `.unwrap()` to prevent panics
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rstest/connection_recovery_test.exstest/prepared_statement_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : NEVER use `.unwrap()` in production Rust code—use `safe_lock` helpers and proper error handling instead (tests may use `.unwrap()` for simplicity)
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use cursor streaming for large result sets to avoid loading entire datasets into memory
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/connection_recovery_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*.exs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/connection_recovery_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use savepoints for partial rollback within transactions to enable error recovery patterns in batch operations
Applied to files:
test/connection_recovery_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Use the Repository pattern for data access in EctoLibSql applications
Applied to files:
test/connection_recovery_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/turso_remote_test.exs : Turso remote tests should be skipped by default if credentials are missing and only run when explicitly needed due to cost implications
Applied to files:
test/connection_recovery_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries to achieve ~10-15x performance improvement
Applied to files:
test/prepared_statement_test.exs
📚 Learning: 2026-01-08T04:56:27.393Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.393Z
Learning: Applies to priv/repo/migrations/*.exs : Use RAW ROWID tables with the `random_rowid: true` option to prevent ID enumeration attacks in production
Applied to files:
test/prepared_statement_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto/adapters/libsql/connection.ex : Add SQLite function support by defining expression handlers for native SQLite functions
Applied to files:
test/prepared_statement_test.exs
🧬 Code graph analysis (4)
test/cursor_streaming_large_test.exs (1)
lib/ecto_libsql.ex (3)
handle_execute(148-188)handle_declare(466-484)handle_fetch(415-442)
test/connection_recovery_test.exs (2)
lib/ecto_libsql.ex (4)
disconnect(131-133)handle_execute(148-188)handle_begin(312-317)handle_rollback(350-358)lib/ecto_libsql/native.ex (5)
prepare(951-959)batch(1208-1212)create_savepoint(1805-1807)rollback_to_savepoint_by_name(1877-1879)reset(1312-1314)
test/prepared_statement_test.exs (3)
test/stmt_caching_performance_test.exs (1)
exec_sql(15-18)test/savepoint_test.exs (1)
exec_sql(14-17)lib/ecto_libsql/native.ex (4)
prepare(951-959)execute_stmt(991-1020)close_stmt(1080-1082)query_stmt(1041-1067)
test/pool_load_test.exs (2)
lib/ecto_libsql.ex (2)
handle_execute(148-188)disconnect(131-133)lib/ecto_libsql/native.ex (5)
begin(828-838)commit(853-857)prepare(951-959)execute_stmt(991-1020)close_stmt(1080-1082)
🪛 LanguageTool
TEST_AUDIT_REPORT.md
[typographical] ~362-~362: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...is_err()); } ``` Estimated effort: 1-2 hours Impact: - Verifies Rust lay...
(HYPHEN_TO_EN)
[typographical] ~420-~420: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...ormat operations Estimated effort: 3-4 hours Impact: More robust confiden...
(HYPHEN_TO_EN)
[typographical] ~548-~548: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...ntributors Estimated total effort: 2-3 hours for high-priority items
(HYPHEN_TO_EN)
🪛 markdownlint-cli2 (0.18.1)
TEST_AUDIT_REPORT.md
17-17: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
199-199: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
453-453: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
462-462: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (31)
TEST_AUDIT_REPORT.md (1)
1-549: Comprehensive and well-structured test audit documentation.This document provides excellent guidance for test consolidation efforts. The tiered categorisation, clear recommendations with effort estimates, and implementation checklists are valuable for tracking progress. The analysis of Rust vs Elixir test separation and identification of duplication aligns well with the PR objectives.
TEST_COVERAGE_ISSUES_CREATED.md (1)
1-127: Well-organised issue tracking documentation.This document provides excellent traceability from the TEST_AUDIT_REPORT.md recommendations to actionable issues. The priority breakdown, effort estimates, and mapping table make it easy to track progress on test coverage improvements.
native/ecto_libsql/src/tests/error_handling_tests.rs (7)
1-16: Excellent module documentation and appropriate clippy configuration.The module documentation clearly explains the purpose (BEAM VM stability) and the focus areas for error handling tests. The
#![allow(clippy::unwrap_used)]is appropriate here as per the project guidelines that allow.unwrap()in test code for simplicity.
22-28: Test isolation with unique database files.Good pattern using UUID-based database filenames to ensure test isolation. The cleanup function properly removes the test database file.
34-171: Comprehensive constraint violation tests.These tests cover all major SQLite constraint types (NOT NULL, UNIQUE, PRIMARY KEY, CHECK) and verify that violations result in errors rather than panics. This is critical for BEAM VM stability.
239-314: Parameter binding error tests with pragmatic assertions.The approach of not asserting on specific success/failure for edge cases (parameter count mismatch) is sensible, as libsql behaviour may vary. The key assertion that it doesn't panic is the correct focus for BEAM stability.
485-511: Platform-specific tests for invalid paths.Good use of
#[cfg(unix)]and#[cfg(windows)]to provide platform-appropriate invalid path tests. This ensures the test suite works correctly across platforms.
599-635: Unicode handling and SQL injection prevention tests.Excellent tests for Unicode data preservation and SQL injection prevention via parameterisation. The Unicode test includes characters from multiple scripts (Latin, Chinese, Arabic) providing good coverage.
1-673: Comprehensive error handling test suite.This new test file addresses the recommendations from TEST_AUDIT_REPORT.md for Rust-side error handling tests. It covers constraint violations, syntax errors, parameter binding, transaction errors, prepared statements, file errors, and edge cases. The tests verify graceful error returns rather than panics, which is critical for BEAM VM stability. Based on learnings, the use of
.unwrap()in test setup is appropriate.test/connection_recovery_test.exs (5)
1-16: Clean test module setup with in-memory database.Good use of in-memory database for test isolation and proper cleanup in
on_exitcallback. The alias forEctoLibSqlkeeps the test code concise.
18-91: Thorough connection recovery tests for query and constraint errors.These tests verify that the connection remains usable after failed queries and constraint violations. The pattern of executing an error-causing operation and then verifying subsequent operations succeed is appropriate for testing resilience.
133-176: Transaction error recovery test.Good test verifying that a transaction can still be rolled back after a query error within the transaction. This is important for ensuring data integrity in error scenarios.
324-385: Savepoint error recovery with partial rollback.Excellent test demonstrating savepoint functionality for targeted rollback within a transaction. The test correctly verifies that:
- Data before the savepoint is preserved
- Data after savepoint rollback is lost
- New data can be inserted after rollback
- Final commit works correctly
This covers an important error recovery pattern as per the coding guidelines.
1-457: Comprehensive connection recovery test suite.This test module provides excellent coverage of connection resilience scenarios including failed queries, constraint violations, syntax errors, transaction errors, prepared statement errors, batch failures, savepoint recovery, and connection resets. The tests follow the pattern of causing an error and verifying the connection remains usable, which is critical for production reliability.
test/pool_load_test.exs (5)
1-41: Well-documented test module with proper setup.The moduledoc clearly explains the test scenarios and the rationale for using separate connections. The setup properly creates a unique database file and includes comprehensive cleanup of SQLite WAL mode files (-shm, -wal).
43-109: Concurrent connection tests with appropriate timeouts.Good use of
Task.async/Task.await_manyfor testing concurrent connections. Thebusy_timeout: 30_000setting is appropriate for SQLite concurrent access where write locks may cause contention. The tests verify all concurrent operations succeed.
111-177: Long-running operation tests.These tests verify that transactions held for longer periods don't cause timeout issues. The use of
Process.sleepto simulate work within transactions is appropriate for this scenario.
270-308: Prepared statement cleanup under concurrent load.Good test verifying that prepared statements are properly created, executed, and closed under concurrent load. The explicit
close_stmtcall ensures resources are cleaned up.
1-356: Comprehensive pool/load test suite addressing identified gaps.This test module addresses the "Connection pool behavior under load" gap identified in TEST_AUDIT_REPORT.md. It covers concurrent independent connections, long-running operations, connection recovery, resource cleanup, and transaction isolation. The use of file-based databases with WAL mode and appropriate busy timeouts is well-suited for testing concurrent SQLite access.
test/cursor_streaming_large_test.exs (4)
1-30: Clear test module setup with comprehensive table schema.Good setup with in-memory database and a table schema that supports various test scenarios (batch_id for grouping, sequence for ordering, data BLOB for binary tests).
32-59: Large dataset streaming tests.These tests verify cursor streaming with 1K and 10K rows, demonstrating that large datasets can be processed in batches without loading everything into memory at once. This addresses the "Large result sets with streaming" gap identified in TEST_AUDIT_REPORT.md.
110-155: BLOB data preservation through cursor streaming.Excellent test verifying that binary data is correctly preserved when streamed through cursors. The test creates 1KB binary data blocks and verifies both the content structure and size are maintained.
1-417: Comprehensive cursor streaming test suite.This test module provides excellent coverage of cursor-based streaming scenarios including large datasets, batch sizes, filtering, ordering, BLOB handling, JOINs, computed columns, aggregations, and more. This directly addresses the "Large result set streaming (cursors)" gap identified in TEST_AUDIT_REPORT.md. The tests verify that data can be streamed in batches without loading entire datasets into memory.
test/prepared_statement_test.exs (8)
313-366: Statement reset and caching tests.Good tests verifying that prepared statements can be reused multiple times with automatic binding reset between executions. This demonstrates the ~10-15x performance benefit of prepared statements mentioned in the learnings.
368-425: Explicit reset tests with error handling.These tests verify the explicit
reset_stmtAPI, including error handling for invalid statement IDs. Good coverage of the reset lifecycle.
427-507: Full column metadata introspection tests.Comprehensive tests for
get_stmt_columnscovering basic columns, aliased columns, and expression columns. Good verification of the metadata tuple structure{name, origin_name, decl_type}.
509-586: Parameter naming tests for different styles.Excellent coverage of SQLite's parameter naming conventions including colon-style (
:name), dollar-style ($name), at-style (@name), and mixed positional/named parameters.
740-975: Concurrent prepared statement usage tests.These tests verify thread-safety of prepared statements under concurrent access. Good coverage of:
- Multiple processes using different statements
- Single statement shared across processes
- Concurrent writes maintaining consistency
- Parameter isolation across tasks
- Concurrent resets maintaining isolation
This addresses an important concern for production use where multiple processes may share prepared statements.
734-734: Good use of British English spelling.The comment correctly uses "behaviour" (British spelling) as per the coding guidelines.
1-976: Significantly expanded prepared statement test suite.This test file now provides comprehensive coverage of prepared statement functionality including:
- Basic preparation and execution
- Statement introspection (columns, parameters)
- Statement lifecycle (close, use-after-close)
- Reset and caching behaviour
- Parameter naming conventions
- Binding behaviour (auto-reset, type coercion)
- Concurrent usage across multiple processes
The additions address the consolidation goals from TEST_AUDIT_REPORT.md by merging functionality previously in
statement_features_test.exsinto this canonical file.
7-7: No action required—async: trueis safely implemented.The test module uses
async: truewith proper database isolation: each test creates a unique database file via:erlang.unique_integer([:positive])and cleans it up after execution. This isolation pattern prevents interference between concurrent test runs. The concurrent task spawning within individual tests (lines 740–975) operates against each test's own isolated database, confirming that async execution is safe.
TEST_AUDIT_REPORT.md
Outdated
| @@ -0,0 +1,549 @@ | |||
| # Comprehensive Test Audit: Elixir vs Rust Tests | |||
|
|
|||
| **Date**: 2024-01-08 | |||
There was a problem hiding this comment.
Inconsistent date in document header.
The date shows 2024-01-08 but the PR was created on 2026-01-08. This appears to be a typo.
-**Date**: 2024-01-08
+**Date**: 2026-01-08 📝 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.
| **Date**: 2024-01-08 | |
| **Date**: 2026-01-08 |
🤖 Prompt for AI Agents
In @TEST_AUDIT_REPORT.md at line 3, Update the document header's Date value from
"2024-01-08" to the correct PR creation date "2026-01-08" by editing the Date
line in TEST_AUDIT_REPORT.md so the header matches the actual PR date.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/cursor_streaming_large_test.exs (2)
232-236: Consider tightening batch count assertion.For 1000 rows with a batch size of 100, you should get exactly 10 batches. The assertion
>= 9is conservative and could mask off-by-one errors.Suggested fix
- assert batch_count >= 9, "Should have at least 9 batches for 1000 rows with batch size 100" + assert batch_count == 10, "Should have exactly 10 batches for 1000 rows with batch size 100"
372-420: Consider consolidating duplicate helper functions.
fetch_all_binary_rows,fetch_all_computed_rows,fetch_all_group_rows, andfetch_all_distinct_rowsare functionally identical—they all collectresult.rows. A single generic helper would reduce duplication.Suggested consolidation
- defp fetch_all_binary_rows(state, cursor, query, opts) do - case EctoLibSql.handle_fetch(query, cursor, opts, state) do - {:cont, result, next_state} -> - result.rows ++ fetch_all_binary_rows(next_state, cursor, query, opts) - - {:halt, result, _state} -> - result.rows - end - end - - defp fetch_all_computed_rows(state, cursor, query, opts) do - case EctoLibSql.handle_fetch(query, cursor, opts, state) do - {:cont, result, next_state} -> - result.rows ++ fetch_all_computed_rows(next_state, cursor, query, opts) - - {:halt, result, _state} -> - result.rows - end - end - - defp count_batches(state, cursor, query, opts) do - case EctoLibSql.handle_fetch(query, cursor, opts, state) do - {:cont, _result, next_state} -> - 1 + count_batches(next_state, cursor, query, opts) - - {:halt, _result, _state} -> - 1 - end - end - - defp fetch_all_group_rows(state, cursor, query, opts) do - case EctoLibSql.handle_fetch(query, cursor, opts, state) do - {:cont, result, next_state} -> - result.rows ++ fetch_all_group_rows(next_state, cursor, query, opts) - - {:halt, result, _state} -> - result.rows - end - end - - defp fetch_all_distinct_rows(state, cursor, query, opts) do - case EctoLibSql.handle_fetch(query, cursor, opts, state) do - {:cont, result, next_state} -> - result.rows ++ fetch_all_distinct_rows(next_state, cursor, query, opts) - - {:halt, result, _state} -> - result.rows - end - end + defp fetch_all_raw_rows(state, cursor, query, opts) do + case EctoLibSql.handle_fetch(query, cursor, opts, state) do + {:cont, result, next_state} -> + result.rows ++ fetch_all_raw_rows(next_state, cursor, query, opts) + + {:halt, result, _state} -> + result.rows + end + endThen update callers to use
fetch_all_raw_rows/4.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/cursor_streaming_large_test.exs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/cursor_streaming_large_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/cursor_streaming_large_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/cursor_streaming_large_test.exs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
test/cursor_streaming_large_test.exs
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Always format Elixir code before committing using `mix format --check-formatted`
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use cursor streaming for large result sets to avoid loading entire datasets into memory
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/cursor_streaming_large_test.exs
🧬 Code graph analysis (1)
test/cursor_streaming_large_test.exs (1)
lib/ecto_libsql.ex (4)
handle_execute(148-188)disconnect(131-133)handle_declare(466-484)handle_fetch(415-442)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (10)
test/cursor_streaming_large_test.exs (10)
1-34: Setup block and module structure look good.The test module is well-structured with an in-memory database setup, proper cleanup via
on_exit, and a table schema that supports the various test scenarios (including BLOB data via thedatacolumn).
37-63: Good coverage of basic large dataset streaming.These tests effectively validate that the cursor can stream large datasets (1K and 10K rows) without loading all data into memory at once, with configurable batch sizes.
65-82: Batch size validation is correct.The test properly verifies that the cursor respects the
max_rowsoption by checking the first batch size and then counting remaining rows to confirm the total.
100-112: Order verification test is well-designed.The test correctly verifies that cursor streaming preserves row ordering by comparing fetched IDs against the expected sequence.
114-159: BLOB handling test is thorough.Good test for binary data preservation through the cursor streaming path. The test creates known binary patterns and verifies size integrity.
253-277: GROUP BY aggregation test is well-designed.The test correctly validates grouped aggregation results by verifying both the number of groups and the count per group.
279-295: OFFSET/LIMIT boundary validation is correct.Good test for verifying that cursor streaming respects SQL-level pagination, checking both the count and the exact ID range.
325-339: Helper functioninsert_rowsis correct.The row-by-row insertion is straightforward and works well for test purposes. Whilst batch inserts would be faster for large datasets, the current approach prioritises clarity.
36-319: Consider complementary test coverage for cursor streaming error and edge cases.The test suite provides comprehensive happy-path coverage for cursor streaming with large datasets (various SQL clauses, binary data, computed columns). Per the coding guidelines, tests should cover error cases, edge cases (NULL values, empty result sets), transaction rollback scenarios, and concurrent operations. Verify whether these scenarios are covered in companion test files before addressing this review.
246-250: No action required — the test assumption is correct.The
handle_fetchimplementation returns{:halt, ...}only when the rows list is empty. For an aggregation query returning a single row,rowswill be non-empty and{:cont, ...}will be returned, as expected by the test pattern match.
…nd fix formatting
ocean
force-pushed
the
consolidate-tests
branch
from
fa26f8f to
efbeacb
Compare
- Changed format string from {}, i to {i} syntax
- Resolves clippy::uninlined-format-args warning
- All tests pass, no formatting issues
…cation - Tighten batch count assertion to exactly 11 (10 full batches + 1 final empty batch) - Replace 4 nearly identical fetch_all_* functions with single generic helper - Keep semantic aliases for backwards compatibility - All tests pass
- Add explicit CRITICAL warning in Quick Rules section - Detail exact 5-step pre-commit workflow with strict ordering - Emphasise that check-formatted must PASS before any commit - Clarify Cargo fmt --check verification step - Add warning about never committing with failing checks
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/json_helpers_test.exs (1)
5-29: Consider assertingdisconnect/2succeeds (or intentionally ignoring failures).
on_exitcleanup is a good addition, but right now any disconnect failure will be silent. IfEctoLibSql.disconnect/2returns a tagged tuple, pattern-match it for a cleaner failure mode.Proposed tweak
on_exit(fn -> - EctoLibSql.disconnect([], state) + _ = EctoLibSql.disconnect([], state) end)
🤖 Fix all issues with AI agents
In @native/ecto_libsql/src/tests/error_handling_tests.rs:
- Around line 291-313: The assertion `result.is_ok() || result.is_err()` is a
tautology and tests nothing; replace it with a real check in the
`test_type_coercion_integer_to_text` test: ensure the `conn.execute(...)` call
stored in `result` does not panic by asserting success (e.g., assert that
`result.is_ok()` or unwrap the result) so the insert using `Value::Integer(123)`
for the TEXT column is verified to complete; update the assertion on the
`result` variable (from the `conn.execute` call) accordingly.
- Around line 1-14: The file uses US spelling in Rust comments/docs (e.g.,
“parameterized”); update those to Australian/British variants across the test
file (native/ecto_libsql/src/tests/error_handling_tests.rs) including the header
and other occurrences around the Parameter validation section and lines
~647-670; specifically replace "parameterized" → "parameterised",
"parameterization" → "parameterisation", and any similar US spellings in
comments/docs while leaving code identifiers and string literals unchanged so
function names (tests, modules) and runtime behavior are not modified.
- Around line 22-28: Tests call cleanup_test_db(&db_path) while DB handles
(conn/db) are still in scope and they don't remove SQLite WAL/SHM files, causing
Windows file-lock flakes; change setup_test_db to create the DB path under
std::env::temp_dir(), implement an RAII guard type (e.g., TestDbGuard) declared
before any db/conn so its Drop runs after handles are dropped, and move deletion
logic into its Drop to remove the main .db and the associated .db-wal and
.db-shm files; update tests to instantiate TestDbGuard using the path produced
by setup_test_db and remove direct cleanup_test_db calls so cleanup is reliable
across platforms.
In @test/cursor_streaming_large_test.exs:
- Around line 341-359: Both fetch_all_rows and fetch_remaining_rows call
EctoLibSql.handle_fetch/4 but lack a clause for {:error, reason, state}, causing
a CaseClauseError on failures; add a third match in each case to handle {:error,
reason, _state} and call ExUnit.flunk/1 with a descriptive message (e.g., "fetch
error: #{inspect(reason)}") so tests fail with a clear error instead of a
CaseClauseError.
In @test/pool_load_test.exs:
- Around line 44-109: Tasks open DB connections with EctoLibSql.connect then
call EctoLibSql.handle_execute but call EctoLibSql.disconnect only on the
success path, so if a task crashes the DB stays locked; wrap the task body in a
try ... after (or try do ... rescue ... after ...) so that
EctoLibSql.disconnect([], state) is invoked in the after block regardless of
success/failure and still return the original result/exception, updating all
task bodies that call EctoLibSql.connect, handle_execute, and disconnect (e.g.,
inside the blocks that currently use Task.async in the tests).
🧹 Nitpick comments (5)
native/ecto_libsql/src/tests/error_handling_tests.rs (1)
513-544: Rename or refactortest_readonly_database_insert— it doesn't actually open a read-only connectionThe test reopens the database using
Builder::new_local(), which creates a standard read-write connection, not a read-only one. In libsql v0.9.29, read-only connections requireBuilder::new_local_replica(). Either rename the test to reflect what it actually does (e.g.test_reopen_and_query_database) or refactor it to properly set up read-only mode using the replication feature if that's the intent.test/pool_load_test.exs (2)
19-41:on_exitfile cleanup should tolerate:enoent(and optionally assert unexpected errors).
File.rm/1returns{:error, :enoent}when the file doesn’t exist; ignoring is fine, but if you want stronger hygiene, handle only:enoentand surface other failures.
43-356: Consider tagging these as:slow/:flaky(or reducing counts) to keep CI stable.These tests intentionally stress concurrency and locking; on constrained CI they may intermittently time out. Tagging lets you keep them valuable without making the default suite brittle.
test/cursor_streaming_large_test.exs (2)
325-339: Bulk insert (or wrap inserts in a transaction) to keep runtime under control.
insert_rows/4issues one insert per row; with 10k+ rows across multiple tests this can dominate CI time. Even a single explicit transaction around the loop would help.
361-381: Avoid++in recursive fetch helpers (can go O(n²) and inflate memory).
ids ++ fetch_all_ids(...)andresult.rows ++ ...repeatedly copy lists. Prefer an accumulator (reverse at the end) orStream/Enum.reduce.Sketch: accumulator approach for IDs
- defp fetch_all_ids(state, cursor, query, opts) do - case EctoLibSql.handle_fetch(query, cursor, opts, state) do - {:cont, result, next_state} -> - ids = Enum.map(result.rows, fn [id] -> id end) - ids ++ fetch_all_ids(next_state, cursor, query, opts) - - {:halt, result, _state} -> - Enum.map(result.rows, fn [id] -> id end) - end - end + defp fetch_all_ids(state, cursor, query, opts) do + fetch_all_ids(state, cursor, query, opts, []) + end + + defp fetch_all_ids(state, cursor, query, opts, acc) do + case EctoLibSql.handle_fetch(query, cursor, opts, state) do + {:cont, result, next_state} -> + batch_ids = Enum.map(result.rows, fn [id] -> id end) + fetch_all_ids(next_state, cursor, query, opts, [batch_ids | acc]) + + {:halt, result, _state} -> + batch_ids = Enum.map(result.rows, fn [id] -> id end) + acc = [batch_ids | acc] + acc |> Enum.reverse() |> List.flatten() + end + endAlso applies to: 372-399
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignorenative/ecto_libsql/src/tests/error_handling_tests.rstest/connection_recovery_test.exstest/cursor_streaming_large_test.exstest/json_helpers_test.exstest/pool_load_test.exstest/prepared_statement_test.exs
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- test/connection_recovery_test.exs
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/cursor_streaming_large_test.exstest/json_helpers_test.exstest/prepared_statement_test.exstest/pool_load_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/cursor_streaming_large_test.exstest/json_helpers_test.exstest/prepared_statement_test.exstest/pool_load_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/cursor_streaming_large_test.exstest/json_helpers_test.exstest/prepared_statement_test.exstest/pool_load_test.exs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
test/cursor_streaming_large_test.exstest/json_helpers_test.exsnative/ecto_libsql/src/tests/error_handling_tests.rstest/prepared_statement_test.exstest/pool_load_test.exs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Always format Rust code before committing usingcargo fmt
NEVER use.unwrap()in production Rust code—usesafe_lockhelpers and proper error handling instead (tests may use.unwrap()for simplicity)
Files:
native/ecto_libsql/src/tests/error_handling_tests.rs
native/ecto_libsql/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
native/ecto_libsql/src/**/*.rs: Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Validate transaction ownership by checking thatentry.conn_id == conn_idbefore performing transaction operations
Add#[rustler::nif]attribute to NIF functions—modern Rustler auto-detects them without manual registration
Files:
native/ecto_libsql/src/tests/error_handling_tests.rs
native/ecto_libsql/src/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
native/ecto_libsql/src/tests/error_handling_tests.rs
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Validate and document unsupported Ecto features by returning `:unsupported` atom errors in Rust implementations with clear documentation and comprehensive tests
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Add `#[rustler::nif]` attribute to NIF functions—modern Rustler auto-detects them without manual registration
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use cursor streaming for large result sets to avoid loading entire datasets into memory
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
test/cursor_streaming_large_test.exstest/json_helpers_test.exsnative/ecto_libsql/src/tests/error_handling_tests.rstest/prepared_statement_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/cursor_streaming_large_test.exstest/json_helpers_test.exsnative/ecto_libsql/src/tests/error_handling_tests.rstest/prepared_statement_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : JSON functions (extract, type, set, replace, insert, remove, patch) should be used for efficient JSON/JSONB manipulation in LibSQL 3.45.1+
Applied to files:
test/json_helpers_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
test/json_helpers_test.exsnative/ecto_libsql/src/tests/error_handling_tests.rstest/prepared_statement_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/json_helpers_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
Applied to files:
test/json_helpers_test.exsnative/ecto_libsql/src/tests/error_handling_tests.rstest/prepared_statement_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Add `#[rustler::nif]` attribute to NIF functions—modern Rustler auto-detects them without manual registration
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Validate and document unsupported Ecto features by returning `:unsupported` atom errors in Rust implementations with clear documentation and comprehensive tests
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Lock Arc<Mutex<T>> using `safe_lock_arc(&client, "function_name client")?` instead of `.unwrap()` to prevent panics
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/connection.rs : Lock a registry using `safe_lock(®ISTRY, "function_name context")?` instead of `.unwrap()` to prevent panics
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : NEVER use `.unwrap()` in production Rust code—use `safe_lock` helpers and proper error handling instead (tests may use `.unwrap()` for simplicity)
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries to achieve ~10-15x performance improvement
Applied to files:
test/prepared_statement_test.exs
📚 Learning: 2026-01-08T04:56:27.393Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.393Z
Learning: Applies to priv/repo/migrations/*.exs : Use RAW ROWID tables with the `random_rowid: true` option to prevent ID enumeration attacks in production
Applied to files:
test/prepared_statement_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto/adapters/libsql/connection.ex : Add SQLite function support by defining expression handlers for native SQLite functions
Applied to files:
test/prepared_statement_test.exs
🧬 Code graph analysis (3)
test/cursor_streaming_large_test.exs (1)
lib/ecto_libsql.ex (3)
handle_execute(148-188)disconnect(131-133)handle_fetch(415-442)
test/json_helpers_test.exs (2)
lib/ecto_libsql.ex (2)
disconnect(131-133)handle_execute(148-188)lib/ecto_libsql/query.ex (1)
to_string(49-51)
test/pool_load_test.exs (2)
lib/ecto_libsql.ex (2)
handle_execute(148-188)disconnect(131-133)lib/ecto_libsql/native.ex (5)
begin(828-838)commit(853-857)prepare(951-959)execute_stmt(991-1020)close_stmt(1080-1082)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
native/ecto_libsql/src/tests/error_handling_tests.rs (1)
34-673: Nice breadth of negative-path coverageThe suite hits a good spread of constraint, syntax, transaction-lifecycle, prepared-statement, and edge-case scenarios, and the tests consistently avoid
unwrap()on the operation-under-test (matching the “no panic” goal). Based on learnings, allowingunwrap()for setup innative/ecto_libsql/src/tests/**/*.rsis fine.test/json_helpers_test.exs (1)
292-300: Good move dropping JSONB size assumptions.
assert result != jsonis a much more stable property than comparing sizes across libSQL/SQLite versions.test/prepared_statement_test.exs (3)
313-586: Nice coverage of statement reset + metadata + parameter name variants.This rounds out the lifecycle surface area well (reuse, explicit reset, columns, and mixed parameter styles).
509-586: The test assumptions aboutstmt_parameter_nameindexing are correct.SQLite and libSQL use 1-based indexing for parameter lookup, and anonymous positional parameters (
?) returnnil. The test cases correctly validate this behaviour across colon-style (:name), dollar-style ($name), and at-style (@name) named parameters, as well as mixed parameter types. No changes required.
740-975: Concurrent prepared statement usage is fully supported and thread-safe.The NIF implementation uses thread-safe registries (
Mutex<HashMap>) to manage connections and statements, with each statement stored asArc<Mutex<stmt>>for safe concurrent access. Bothquery_preparedandexecute_preparedvalidate statement ownership (decode::verify_statement_ownership) before operations, and statement bindings are reset before each execution to ensure parameter isolation. These tests are excellent regression tests for verifying the concurrency guarantees and can be kept as-is.
- Changed 'parameterized' to 'parameterised' in comments (3 occurrences) - Applies CLAUDE.md guidance on British/Australian English - No functional changes to code or test identifiers - All tests pass
- test/pool_load_test.exs: Handle :enoent in file cleanup gracefully - Changed on_exit/1 to tolerate missing files with proper error handling - Surfaces unexpected errors with IO.warn for better diagnostics - test/pool_load_test.exs: Tag all stress tests as :slow and :flaky - These concurrency/load tests can be flaky on constrained CI systems - Tagged to keep default test suite fast and stable - Tests still run when explicitly included - test/test_helper.exs: Update exclusion logic for CI - Default: exclude :ci_only, :slow, :flaky tests locally - On CI: only exclude :flaky tests to maintain stability - Reduces CI brittleness while keeping valuable tests available
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
native/ecto_libsql/src/tests/error_handling_tests.rs (1)
309-312: Tautological assertion - always passes.
result.is_ok() || result.is_err()is always true for anyResult. This assertion doesn't verify anything meaningful. For consistency with similar tests in this file (e.g., lines 259, 286), use thelet _ = result;pattern instead.♻️ Suggested fix
- assert!( - result.is_ok() || result.is_err(), - "Should handle type coercion without panic" - ); + // SQLite permits this due to type affinity, but should not panic + let _ = result;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{md,markdown}
📄 CodeRabbit inference engine (CLAUDE.md)
Use British/Australian English for all documentation files (except SQL keywords and compatibility requirements)
Files:
CLAUDE.md
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Always format Rust code before committing usingcargo fmt
NEVER use.unwrap()in production Rust code—usesafe_lockhelpers and proper error handling instead (tests may use.unwrap()for simplicity)
Files:
native/ecto_libsql/src/tests/error_handling_tests.rs
native/ecto_libsql/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
native/ecto_libsql/src/**/*.rs: Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Validate transaction ownership by checking thatentry.conn_id == conn_idbefore performing transaction operations
Add#[rustler::nif]attribute to NIF functions—modern Rustler auto-detects them without manual registration
Files:
native/ecto_libsql/src/tests/error_handling_tests.rs
native/ecto_libsql/src/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
native/ecto_libsql/src/tests/error_handling_tests.rs
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Validate and document unsupported Ecto features by returning `:unsupported` atom errors in Rust implementations with clear documentation and comprehensive tests
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to AGENTS.md : Update AGENTS.md API reference when adding new public NIF functions or Ecto features that users should know about
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to README.md : Update README.md when adding new features or changing user-facing functionality
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : NEVER use `.unwrap()` in production Rust code—use `safe_lock` helpers and proper error handling instead (tests may use `.unwrap()` for simplicity)
Applied to files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : Always format Rust code before committing using `cargo fmt`
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Applied to files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Lock Arc<Mutex<T>> using `safe_lock_arc(&client, "function_name client")?` instead of `.unwrap()` to prevent panics
Applied to files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Always format Elixir code before committing using `mix format --check-formatted`
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Validate and document unsupported Ecto features by returning `:unsupported` atom errors in Rust implementations with clear documentation and comprehensive tests
Applied to files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Use Beads (`bd` command) for multi-session issue tracking with persistence across sessions and for work with dependencies between tasks
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Add `#[rustler::nif]` attribute to NIF functions—modern Rustler auto-detects them without manual registration
Applied to files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs,rs,md} : When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with `git commit`, clean up stashes, and provide context for next session
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
Applied to files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-08T04:56:27.393Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.393Z
Learning: Applies to priv/repo/migrations/*.exs : Use STRICT tables in migrations for type safety when database schema stability is critical
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
CLAUDE.mdnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{md,markdown} : Use British/Australian English for all documentation files (except SQL keywords and compatibility requirements)
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/connection.rs : Lock a registry using `safe_lock(®ISTRY, "function_name context")?` instead of `.unwrap()` to prevent panics
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Rust Fuzz Testing
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (11)
native/ecto_libsql/src/tests/error_handling_tests.rs (7)
1-28: LGTM!The module documentation clearly explains the test purpose and focus areas. The
#![allow(clippy::unwrap_used)]is appropriate here since tests may use.unwrap()for simplicity per project guidelines. The UUID-based test database setup ensures test isolation.
34-171: LGTM!Comprehensive coverage of constraint violation scenarios (NOT NULL, UNIQUE, PRIMARY KEY, CHECK). Each test properly isolates the database, validates that violations return errors rather than panicking, and cleans up afterwards.
177-233: LGTM!Good coverage of SQL syntax and semantic errors. Testing typos, non-existent tables/columns, and malformed SQL ensures the NIF layer handles these gracefully without panicking.
320-430: LGTM!Excellent transaction error coverage including double commit/rollback scenarios and verification that rollback actually works. The
test_query_after_rollbacktest properly validates that data is not persisted after rollback.
436-479: LGTM!Good coverage of prepared statement error scenarios. The parameter mismatch test appropriately uses the
let _ = result;pattern since libsql behaviour may vary.
485-544: LGTM!Good platform-specific handling with
#[cfg(unix)]and#[cfg(windows)]attributes. The invalid path tests (null bytes on Unix, reserved device name on Windows) are reasonable edge cases to verify no panics occur.
550-673: LGTM!Excellent edge case coverage including empty/whitespace SQL, very long queries, Unicode handling (good multi-script example), and SQL injection safety verification. The injection test properly validates that the malicious string is stored literally rather than executed, confirming parameterised query safety.
CLAUDE.md (4)
13-17: LGTM!Good addition of explicit formatting workflow in Quick Rules. The three-step process (format → verify → commit) with emphasis on catching issues early is clear and actionable.
53-53: LGTM!TOC correctly updated to include the Issue Tracking with Beads section.
302-308: LGTM!Practical best practices for Beads workflow that complement the command reference. The emphasis on session hygiene (checking
bd readyat start, syncing before end) aligns well with the mandatory session completion workflow.
565-584: LGTM!The strict ordering and explicit step numbering in the pre-commit checklist provides clear guidance. The critical warning about not proceeding with failing checks reinforces the project's quality standards.
Two key performance improvements: 1. insert_rows/4 - Use prepared statements instead of individual inserts - Was: 1000+ unprepared INSERT statements per test - Now: Reuses single prepared statement with parameter binding - Benefit: Reduces statement compilation overhead, faster inserts - Note: Can't wrap in transaction (cursors require no active transaction) 2. fetch_all_ids/4 and fetch_all_cursor_rows/4 - Fix O(n²) list concatenation - Was: ids ++ recursive_call() and result.rows ++ recursive_call() - Now: Accumulator pattern with single reversal at end - Benefit: Prevents expensive list copies, scales to 10k+ rows - Example: 10k rows went from ~O(n²) to O(n) operations These optimizations keep CI runtime predictable with large dataset tests.
- Create TEST_STATE_VARIABLE_CONVENTIONS.md with comprehensive guidance * Explains error state rebinding vs discarding patterns * Provides clear decision matrix for when to use each pattern * Documents all current code patterns with examples - Update CLAUDE.md with quick reference * Add Test Variable Naming Conventions section * Include code examples for rebinding and discarding patterns * Reference detailed guide for in-depth information - Clarify error state handling in test files * savepoint_replication_test.exs: Add comment explaining why error state is rebound * savepoint_test.exs: Add comment explaining why error state is discarded These patterns are important for: - State threading consistency across test suite - Clarity on whether error states are reused - Understanding NIF state semantics (errors return updated state) Tests: All passing (18 savepoint tests, 4 replication tests) Fixes: Review feedback about variable rebinding patterns in error handling
Enhance test/pool_load_test.exs with better error verification and edge-case coverage:
FIXES:
- Line 268: Explicit error verification for BAD SQL query
* Changed from _error_result = ... (implicit) to explicit assert
* Catches regressions where invalid SQL unexpectedly succeeds
* Follows error state handling patterns from TEST_STATE_VARIABLE_CONVENTIONS.md
NEW TESTS:
1. Concurrent connections with edge-case data (NULL, empty, large values)
- 5 concurrent connections
- 5 edge-case values per connection = 25 total rows
- Verifies NULL values, empty strings, 1KB strings, special chars
- Tests data integrity under concurrent load
2. Concurrent transactions with edge-case data maintaining isolation
- 4 concurrent transactions
- 5 edge-case values per transaction = 20 total rows
- Verifies transaction isolation with edge-case data
- Confirms NULL values survive transaction boundaries
HELPERS:
- generate_edge_case_values/1: Generates NULL, empty string, large string, special chars
- insert_edge_case_value/2: Reusable insertion helper for edge-case data
COVERAGE:
- Tests concurrent inserts with NULL values
- Tests concurrent inserts with empty strings ("")
- Tests concurrent inserts with 1KB strings
- Tests special characters in concurrent scenarios
- Tests edge-case data within transactions
All tests pass: 10 tests, 0 failures, 1.0s execution time
Test tags: slow, flaky (included for verification)
REGRESSION PREVENTION:
- Catches NULL value handling failures under load
- Catches empty string corruption under concurrent writes
- Catches large string handling issues in transactions
- Catches error handling regressions
…ents Document comprehensive work completed in this session: PART 1: TEST STATE VARIABLE NAMING CONVENTIONS - Created TEST_STATE_VARIABLE_CONVENTIONS.md with detailed guidance - Updated CLAUDE.md with quick reference section - Applied patterns to savepoint test files with clarifying comments - Established clear conventions for state threading in error handling PART 2: POOL LOAD TEST IMPROVEMENTS - Fixed implicit error verification (line 268: BAD SQL) - Added edge-case testing helpers for NULL, empty strings, large values - Created 2 new concurrent test scenarios with comprehensive coverage - Prevented 5+ categories of regressions KEY ACHIEVEMENTS: ✅ 22 savepoint tests passing ✅ 10 pool load tests passing (0 failures) ✅ 2 comprehensive guides created ✅ 500+ lines of test improvements ✅ All changes pushed to remote REGRESSION PREVENTION: - NULL value handling failures under load - Empty string corruption under concurrent writes - Large string integrity in transactions - Error handling failures masked by implicit assertions - State threading correctness in error cases SESSION STATISTICS: - Files created: 2 - Files modified: 4 - New test scenarios: 2 - Total passing tests: 32 - Execution time: ~1.5s
Apply formatting fixes to ensure code style consistency: ELIXIR FORMATTING: - test/pool_load_test.exs: Fix comment alignment and indentation * Comments moved to separate lines above items in edge_case_values list * Fixed indentation in describe blocks (transaction isolation) * Aligned with mix format style guide RUST FORMATTING: - native/ecto_libsql/src/tests/error_handling_tests.rs: Fix import ordering * Move super::test_utils import before external crates (libsql) * Matches Rust convention: internal imports before external - native/ecto_libsql/src/tests/integration_tests.rs: Fix import ordering * Move super::test_utils import before external crates (libsql) * Ensures consistent module import order VERIFICATION: ✅ mix format --check-formatted: All files pass ✅ cargo fmt --check: All files pass ✅ mix compile: 0 errors, 0 warnings ✅ Tests: 32 tests, 0 failures ✅ No functional changes - formatting only
Document comprehensive formatting verification completed before finalization: VERIFICATION RESULTS: ✅ Elixir formatting: All files pass mix format --check-formatted ✅ Rust formatting: All files pass cargo fmt --check ✅ Compilation: 0 errors, 0 warnings ✅ Tests: 32 tests passing (0 failures) FILES FORMATTED: - test/pool_load_test.exs: Comments realigned, indentation fixed - error_handling_tests.rs: Import ordering corrected - integration_tests.rs: Import ordering corrected STANDARDS APPLIED: - Elixir: 98 char line length, 2-space indent, comments above items - Rust: Internal imports before external, 100 char line length, 4-space indent QUALITY METRICS: - All checks: PASS - Compilation: PASS - Tests: 32/32 passing - Ready for: Code review, merging, production No functional changes - formatting only
- Add test for connection recovery with edge-case data (NULL, empty, large values) - Add test for prepared statements with edge-case data cleanup - Update TESTING.md with comprehensive edge-case testing guide - Extend coverage matrix: error recovery and resource cleanup now test edge cases - Test results: 34/34 passing (up from 32) - All code formatted and verified
- Add test for concurrent connections with Unicode data (Chinese, Arabic, emoji) - Add helper functions: generate_unicode_edge_case_values and insert_unicode_edge_case_value - Test verifies 5 concurrent connections × 5 Unicode values = 25 rows - Includes Latin accents (café), Chinese (中文), Arabic (العربية), and emoji (😀🎉❤️) - Test results: 35/35 passing (up from 34) - All code formatted and verified
- Document all 3 new tests added in this session - Detail Unicode support validation with Chinese, Arabic, emoji - Coverage matrix and regression prevention guide - Future enhancement opportunities identified - 35/35 tests passing, 0% failure rate
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @EDGE_CASE_TESTING_SUMMARY.md:
- Around line 76-84: The fenced code blocks in EDGE_CASE_TESTING_SUMMARY.md lack
language identifiers, causing markdownlint MD040 failures; update every
triple-backtick block (including the examples containing "Running ExUnit with
seed: 345447, max_cases: 22", "Expected [[2]] NULL values, got [[0]] → Caught",
"Expected [[2]] empty strings, got [[0]] → Caught", "Inserted 1KB string,
retrieve different size → Caught", "Statement not properly closed → Would hang
in connection pool → Caught by cleanup verification", "Insert `!@#$%^&*()`,
retrieve different value → Caught", and the git status block "On branch
consolidate-tests ... working tree clean") to add a language tag (e.g., ```text)
for all occurrences referenced (lines ~76-84, 146-168, 211-215) so each opening
``` becomes ```text.
In @FORMATTING_VERIFICATION_REPORT.md:
- Around line 128-144: The two fenced code blocks in
FORMATTING_VERIFICATION_REPORT.md (the blocks showing the "FAILED" output and
the "After Formatting" output around lines 128–144) are missing language
identifiers and should be updated to use a language tag (e.g., change the
opening triple backticks to ```bash for each block) and ensure each block has a
matching closing triple backtick; update both the "FAILED" block and the "After
Formatting" block to include the same language identifier for correct linting
and highlighting.
In @test/pool_load_test.exs:
- Around line 540-624: The test's task path can return {:error,
:some_edge_case_inserts_failed} without calling
EctoLibSql.Native.close_stmt(stmt), leaking the prepared statement; modify the
anonymous task in the "prepared statements with edge-case data cleaned up
correctly" test so that after preparing (EctoLibSql.Native.prepare) you always
attempt to close the prepared statement (EctoLibSql.Native.close_stmt)
regardless of whether execute_stmt/4 calls succeed or fail — e.g., check that
stmt is bound and call close_stmt in both the success branch and the error
branch (or use an ensure/after block around the execute loop to call close_stmt
if prepare succeeded), and safely ignore or log any close errors so the test can
still return the appropriate {:ok, :prepared_with_edge_cases} or {:error, ...}
result.
In @test/savepoint_replication_test.exs:
- Around line 21-60: In the on_exit/1 closure add a remote table teardown by
calling EctoLibSql.handle_execute with "DROP TABLE IF EXISTS #{test_table}"
(empty params and opts) using the available state before calling
EctoLibSql.disconnect; ensure you wrap it in a try/rescue (or case) to ignore
errors so cleanup never blocks disconnect, mirroring the pattern used for remote
teardown.
🧹 Nitpick comments (5)
TEST_STATE_VARIABLE_CONVENTIONS.md (1)
16-95: Markdown formatting issues require correction.The content effectively distinguishes between "rebind vs discard" cases, but several markdown formatting issues need addressing per linting standards:
- Lines 35, 121, 176: Fenced code blocks should specify language (
elixir)- Lines 54, 76: Section titles using emphasis (
**) should be headings (###)♻️ Proposed markdown formatting fixes
-### When to Rebind vs. Discard +### When to Rebind vs. Discard -**Case 1: Error state is NOT needed for subsequent operations** → Discard with `_state` +#### Case 1: Error state is NOT needed for subsequent operations -``` +```elixir # savepoint_test.exs line 342 (original test) result = EctoLibSql.handle_execute(sql, params, [], trx_state) assert {:error, _reason, _state} = result # Rollback uses the ORIGINAL trx_state, not the error state :ok = Native.rollback_to_savepoint_by_name(trx_state, "sp1") -``` +``` -**Case 2: Error state IS needed for subsequent operations** → Rebind to reuse variable name +#### Case 2: Error state IS needed for subsequent operations -``` +```elixir # savepoint_replication_test.exs line 221 (replication test) result = EctoLibSql.handle_execute(sql, params, [], trx_state) assert {:error, _reason, trx_state} = result # Next operation MUST use the updated trx_state :ok = Native.rollback_to_savepoint_by_name(trx_state, "sp1") -``` +``` -### 2. Error Handling Pattern +### 2. Error Handling Pattern -**For error cases where state continues to be used:** +#### For error cases where state continues to be used: -``` +```elixir # ✅ GOOD: Clear that the error state will be reused result = EctoLibSql.handle_execute(sql, params, [], trx_state) assert {:error, _reason, trx_state} = result # Rebind - state is needed next # Continue using trx_state :ok = EctoLibSql.Native.rollback_to_savepoint_by_name(trx_state, "sp1") -``` +``` -**For error cases where state is terminal:** +#### For error cases where state is terminal: -``` +```elixir # ✅ GOOD: Clear that the error state is discarded result = EctoLibSql.handle_execute(sql, params, [], conn) assert {:error, %EctoLibSql.Error{}, _conn} = result # Discard - not needed again -``` +``` -**Alternative: Use intermediate variable (more explicit but verbose)** +#### Alternative: Use intermediate variable -``` +```elixir # ✅ ALTERNATIVE: If clarity is critical, use different variable result = EctoLibSql.handle_execute(sql, params, [], trx_state) assert {:error, _reason, updated_trx_state} = result # Now it's explicit that the state was updated :ok = EctoLibSql.Native.rollback_to_savepoint_by_name(updated_trx_state, "sp1") -``` +```test/pool_load_test.exs (2)
60-68: Consider threading the returnedstatefrom inserts, even in non-transaction flows.Right now
insert_edge_case_value/2returns a new state but callers typically ignore it and keep using the originalstate. If state is ever enriched/mutated by the NIF layer, this becomes brittle; threading it costs little and matches the conventions you’re documenting. Based on learnings, prefer explicit state threading for NIF operations.Also applies to: 140-211
693-726: Thread the transaction state through successive inserts usingEnum.reduce/3.The transaction state is managed server-side via the transaction registry (keyed by
trx_idandconn_id), so the current code does work. However, usingEnum.mapand discarding intermediate states violates the "always thread state" convention and creates fragile code that could break if implementation details change. The proposedEnum.reducefix is the defensive approach and aligns with the established pattern of carrying the latest state forward through all transactional operations.Proposed fix
- insert_results = - Enum.map(edge_values, fn value -> - {:ok, _query, _result, new_state} = insert_edge_case_value(trx_state, value) - new_state - end) - - # Use final state after all inserts - final_trx_state = List.last(insert_results) || trx_state + final_trx_state = + Enum.reduce(edge_values, trx_state, fn value, acc_trx_state -> + {:ok, _query, _result, next_trx_state} = insert_edge_case_value(acc_trx_state, value) + next_trx_state + end)SESSION_SUMMARY_CONVENTIONS_AND_EDGE_CASES.md (1)
76-171: Align doc spelling with en-AU (eg “parameterised”, “behaviour”) to match repo conventions.A quick pass over this doc to switch US spellings to en-AU would keep it consistent with the rest of the documentation conventions. As per coding guidelines, docs should use British/Australian English (outside SQL keywords/compatibility phrasing).
test/savepoint_replication_test.exs (1)
15-20: Avoid compile-time env “snapshots” for the skip gate (local dev footgun).
Because@turso_uri/@turso_tokenare read at compile-time, toggling env vars without recompiling can leave tests incorrectly skipped (or unskipped). Consider moving the gating to runtime (e.g., via a tag like@moduletag :tursoand excluding it intest/test_helper.exsunless env vars are present). As per coding guidelines, keep comments/docs in en-AU English (this file already does).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CLAUDE.mdEDGE_CASE_TESTING_SUMMARY.mdFORMATTING_VERIFICATION_REPORT.mdPOOL_LOAD_TEST_IMPROVEMENTS.mdSESSION_SUMMARY_CONVENTIONS_AND_EDGE_CASES.mdTESTING.mdTEST_STATE_VARIABLE_CONVENTIONS.mdnative/ecto_libsql/src/tests/error_handling_tests.rsnative/ecto_libsql/src/tests/integration_tests.rstest/pool_load_test.exstest/savepoint_replication_test.exstest/savepoint_test.exs
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{md,markdown}
📄 CodeRabbit inference engine (CLAUDE.md)
Use British/Australian English for all documentation files (except SQL keywords and compatibility requirements)
Files:
POOL_LOAD_TEST_IMPROVEMENTS.mdTEST_STATE_VARIABLE_CONVENTIONS.mdEDGE_CASE_TESTING_SUMMARY.mdTESTING.mdSESSION_SUMMARY_CONVENTIONS_AND_EDGE_CASES.mdFORMATTING_VERIFICATION_REPORT.md
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
POOL_LOAD_TEST_IMPROVEMENTS.mdTEST_STATE_VARIABLE_CONVENTIONS.mdtest/savepoint_test.exsEDGE_CASE_TESTING_SUMMARY.mdTESTING.mdSESSION_SUMMARY_CONVENTIONS_AND_EDGE_CASES.mdtest/savepoint_replication_test.exsnative/ecto_libsql/src/tests/integration_tests.rsnative/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exsFORMATTING_VERIFICATION_REPORT.md
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/savepoint_test.exstest/savepoint_replication_test.exstest/pool_load_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/savepoint_test.exstest/savepoint_replication_test.exstest/pool_load_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/savepoint_test.exstest/savepoint_replication_test.exstest/pool_load_test.exs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Always format Rust code before committing usingcargo fmt
NEVER use.unwrap()in production Rust code—usesafe_lockhelpers and proper error handling instead (tests may use.unwrap()for simplicity)
Files:
native/ecto_libsql/src/tests/integration_tests.rsnative/ecto_libsql/src/tests/error_handling_tests.rs
native/ecto_libsql/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
native/ecto_libsql/src/**/*.rs: Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Validate transaction ownership by checking thatentry.conn_id == conn_idbefore performing transaction operations
Add#[rustler::nif]attribute to NIF functions—modern Rustler auto-detects them without manual registration
Files:
native/ecto_libsql/src/tests/integration_tests.rsnative/ecto_libsql/src/tests/error_handling_tests.rs
native/ecto_libsql/src/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
native/ecto_libsql/src/tests/integration_tests.rsnative/ecto_libsql/src/tests/error_handling_tests.rs
🧠 Learnings (24)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
POOL_LOAD_TEST_IMPROVEMENTS.mdTEST_STATE_VARIABLE_CONVENTIONS.mdtest/savepoint_test.exsEDGE_CASE_TESTING_SUMMARY.mdTESTING.mdSESSION_SUMMARY_CONVENTIONS_AND_EDGE_CASES.mdtest/savepoint_replication_test.exsnative/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
TEST_STATE_VARIABLE_CONVENTIONS.mdtest/savepoint_test.exsTESTING.mdtest/savepoint_replication_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
TEST_STATE_VARIABLE_CONVENTIONS.mdtest/savepoint_test.exsTESTING.mdtest/savepoint_replication_test.exsnative/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
TEST_STATE_VARIABLE_CONVENTIONS.mdtest/savepoint_test.exsTESTING.mdtest/savepoint_replication_test.exsnative/ecto_libsql/src/tests/integration_tests.rsnative/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use savepoints for partial rollback within transactions to enable error recovery patterns in batch operations
Applied to files:
TEST_STATE_VARIABLE_CONVENTIONS.mdtest/savepoint_test.exstest/savepoint_replication_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Use proper error handling patterns with case matching and with clauses to handle both success and error tuples from NIF operations
Applied to files:
TEST_STATE_VARIABLE_CONVENTIONS.mdtest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
TEST_STATE_VARIABLE_CONVENTIONS.mdnative/ecto_libsql/src/tests/integration_tests.rsnative/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
Applied to files:
test/savepoint_test.exsnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use WAL (Write-Ahead Logging) mode for better concurrency in production: `EctoLibSql.Pragma.set_journal_mode(state, :wal)`
Applied to files:
test/savepoint_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Applied to files:
test/savepoint_test.exsnative/ecto_libsql/src/tests/integration_tests.rsnative/ecto_libsql/src/tests/error_handling_tests.rstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to AGENTS.md : Update AGENTS.md API reference when adding new public NIF functions or Ecto features that users should know about
Applied to files:
TESTING.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Always format Elixir code before committing using `mix format --check-formatted`
Applied to files:
test/savepoint_replication_test.exsFORMATTING_VERIFICATION_REPORT.md
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*.exs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-08T04:56:27.393Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.393Z
Learning: Applies to priv/repo/migrations/*.exs : Use STRICT tables in migrations for type safety when database schema stability is critical
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/turso_remote_test.exs : Turso remote tests should be skipped by default if credentials are missing and only run when explicitly needed due to cost implications
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Lock Arc<Mutex<T>> using `safe_lock_arc(&client, "function_name client")?` instead of `.unwrap()` to prevent panics
Applied to files:
native/ecto_libsql/src/tests/integration_tests.rsnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Add `#[rustler::nif]` attribute to NIF functions—modern Rustler auto-detects them without manual registration
Applied to files:
native/ecto_libsql/src/tests/integration_tests.rsnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/connection.rs : Lock a registry using `safe_lock(®ISTRY, "function_name context")?` instead of `.unwrap()` to prevent panics
Applied to files:
native/ecto_libsql/src/tests/integration_tests.rsnative/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Validate and document unsupported Ecto features by returning `:unsupported` atom errors in Rust implementations with clear documentation and comprehensive tests
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : NEVER use `.unwrap()` in production Rust code—use `safe_lock` helpers and proper error handling instead (tests may use `.unwrap()` for simplicity)
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{md,markdown} : Use British/Australian English for all documentation files (except SQL keywords and compatibility requirements)
Applied to files:
native/ecto_libsql/src/tests/error_handling_tests.rs
🧬 Code graph analysis (4)
test/savepoint_replication_test.exs (2)
lib/ecto_libsql.ex (2)
handle_execute(148-188)disconnect(131-133)lib/ecto_libsql/native.ex (5)
begin(828-838)create_savepoint(1805-1807)release_savepoint_by_name(1836-1838)commit(853-857)rollback_to_savepoint_by_name(1877-1879)
native/ecto_libsql/src/tests/integration_tests.rs (1)
native/ecto_libsql/src/tests/test_utils.rs (2)
setup_test_db(81-85)new(38-40)
native/ecto_libsql/src/tests/error_handling_tests.rs (4)
native/ecto_libsql/src/tests/test_utils.rs (3)
setup_test_db_with_prefix(105-109)new(38-40)drop(44-63)lib/ecto_libsql/query.ex (1)
to_string(49-51)native/ecto_libsql/src/tests/integration_tests.rs (2)
row(106-106)row(294-294)native/ecto_libsql/src/transaction.rs (1)
drop(141-150)
test/pool_load_test.exs (2)
lib/ecto_libsql.ex (2)
handle_execute(148-188)disconnect(131-133)lib/ecto_libsql/native.ex (5)
begin(828-838)commit(853-857)prepare(951-959)execute_stmt(991-1020)close_stmt(1080-1082)
🪛 LanguageTool
TEST_STATE_VARIABLE_CONVENTIONS.md
[uncategorized] ~13-~13: Possible missing preposition found.
Context: ...lear convention for managing this state threading. ## Pattern: Error Cases with State Re...
(AI_HYDRA_LEO_MISSING_IN)
EDGE_CASE_TESTING_SUMMARY.md
[style] ~190-~190: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ng with 50+ concurrent connections - Very large datasets (10K+ rows) - Extended tran...
(EN_WEAK_ADJECTIVE)
SESSION_SUMMARY_CONVENTIONS_AND_EDGE_CASES.md
[style] ~238-~238: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... data (BLOB) under concurrent load - Very large datasets (10K+ rows) 2. **Stress testi...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
TEST_STATE_VARIABLE_CONVENTIONS.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
76-76: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
121-121: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
176-176: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
EDGE_CASE_TESTING_SUMMARY.md
76-76: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
146-146: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
151-151: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
156-156: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
166-166: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
211-211: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
FORMATTING_VERIFICATION_REPORT.md
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (rust)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
🔇 Additional comments (31)
FORMATTING_VERIFICATION_REPORT.md (1)
1-187: Verify appropriateness of this report as a permanent committed artefact.This file appears to be a generated or manually created verification report documenting formatting checks performed during the test consolidation work. Before merging, clarify whether this should be:
- A permanent documentation file (if so, consider whether it should live in a separate documentation directory)
- A temporary build/CI artefact (if so, consider adding it to
.gitignoreor removing it from the PR)- Part of release notes or migration documentation (if so, integrate it into the appropriate documentation structure)
Additionally, ensure the document uses British/Australian English throughout, consistent with the coding guidelines for Markdown files (e.g., "organised" rather than "organized").
native/ecto_libsql/src/tests/integration_tests.rs (1)
10-10: LGTM! RAII cleanup pattern properly implemented.The introduction of
TestDbGuardensures automatic cleanup of database files on scope exit, which is excellent Rust practice. The guard leverages Drop semantics to guarantee cleanup even if tests panic.Also applies to: 15-18
native/ecto_libsql/src/tests/error_handling_tests.rs (8)
1-19: Excellent documentation and test organisation.The file header clearly articulates the purpose of these error handling tests and provides valuable context about preventing VM crashes. The focus areas comprehensively cover critical error scenarios.
25-178: Comprehensive constraint violation coverage.These tests properly verify that constraint violations (NOT NULL, UNIQUE, PRIMARY KEY, CHECK) return errors gracefully instead of panicking, which is critical for BEAM VM stability.
184-256: Good coverage of SQL syntax and semantic errors.Tests verify that invalid SQL, non-existent tables, non-existent columns, and malformed queries all return errors appropriately without panicking.
262-349: Appropriate handling of parameter binding edge cases.Tests correctly acknowledge that libsql behaviour varies for parameter count mismatches, focusing on the critical requirement: no panics. The type coercion test correctly verifies SQLite's type affinity behaviour.
355-480: Thorough transaction error handling coverage.Tests verify that double commits, double rollbacks, and invalid transaction state transitions all return errors gracefully. The test_query_after_rollback additionally validates correct rollback semantics.
486-537: Prepared statement error cases properly tested.Tests verify that preparing invalid SQL and executing with parameter mismatches handle errors gracefully rather than panicking.
543-615: Platform-specific error handling and persistence testing.The conditional compilation for Unix/Windows invalid path tests is appropriate. The persistence test, whilst not strictly error handling, provides valuable coverage of database lifecycle correctness.
621-761: Excellent edge case coverage.Tests comprehensively cover empty SQL, whitespace-only SQL, very long queries, Unicode handling, and SQL injection prevention through parameterised queries. The injection test correctly verifies that malicious strings are safely treated as literal data.
POOL_LOAD_TEST_IMPROVEMENTS.md (7)
1-28: Well-structured problem/solution format for error handling.The introduction and first issue clearly articulate the problem with implicit error handling, and the solution demonstrates proper pattern matching with state threading. The format is easy to follow and educationally sound.
29-62: Excellent helper function design for edge-case testing.The
generate_edge_case_values/1andinsert_edge_case_value/4helpers are well-designed for reusability. The documentation clearly explains the purpose of each function and the pattern supports consistent edge-case test coverage across the test suite.
58-100: Comprehensive test scenario documentation.The test descriptions for concurrent connections and transaction isolation clearly articulate what is being tested and why. The use of scenario checkmarks makes verification straightforward, and the line number ranges help readers navigate the implementation.
101-131: Clear coverage matrix and verified test results.The matrix effectively summarises test coverage, and the concrete test results (10/10 passing) provide confidence in the implementation. The time breakdown demonstrates acceptable performance impact.
133-161: Strong regression prevention documentation.The specific examples of regressions that the new tests catch (NULL insertion failures, empty string corruption, large string handling, error recovery) demonstrate thorough thinking about failure modes. This adds credibility to the testing approach.
167-208: Well-explained implementation patterns and reasonable performance trade-offs.The state threading and error verification patterns are clearly explained with rationale. The performance analysis (50–100 ms per test run) demonstrates consideration of test suite execution time, and the suggestion to exclude slow tests with flags is practical.
210-233: Comprehensive navigation and future roadmap.The related documentation section and future improvements list demonstrate forward-thinking about test maintenance and extension. The checklist format effectively confirms completion of the work described.
TEST_STATE_VARIABLE_CONVENTIONS.md (4)
1-15: Clear context and purpose for state variable conventions.The introduction effectively establishes the problem: managing state threading when operations return tuples with updated state even on error. This sets up the need for clear naming conventions.
54-115: Excellent variable naming conventions table.The scope-based naming convention table (connection, transaction, cursor, prepared statement) provides clear guidance for test writers. The consistency of naming patterns makes code review easier and reduces cognitive load when reading tests.
117-161: Pragmatic guidance on rebinding decisions with clear status.The current issues section effectively explains why two nearly identical patterns are both correct (one needs state, one doesn't) based on actual usage patterns in the codebase. The implementation checklist provides a practical decision tree for test writers.
163-195: Strong real-world pattern examples from the codebase.The three pattern examples (discard unused, rebind and use, discarded in terminal operations) with specific file references demonstrate these conventions are already in use. This increases confidence that the documented conventions reflect actual practice.
TESTING.md (4)
720-790: Well-integrated edge-case testing guide with consistent documentation style.The new Edge-Case Testing Guide section integrates seamlessly with existing documentation. The structure (What's Tested, Test Locations, Helper Functions, When to Use, Expected Coverage) mirrors the approach used elsewhere in TESTING.md, providing consistency for readers. The concrete examples using
generate_edge_case_values/1helper connect well to the actual test implementations.
791-826: Smooth integration of edge-case guidance into existing documentation.The new edge-case testing section transitions naturally into the existing "Known Test Limitations" and "Contributing Tests Checklist" sections. The documentation flow remains logical and the checklist naturally references the new edge-case coverage expectations introduced earlier.
745-770: Clear helper function documentation supporting edge-case testing.The documented helpers (
generate_edge_case_values/1andinsert_edge_case_value/4) are presented with clear purpose and usage. The examples show practical application and align with the detailed guide in POOL_LOAD_TEST_IMPROVEMENTS.md.
735-744: Accurate test location references with clear coverage summary.The test location references for pool load tests and transaction isolation tests precisely indicate where edge-case scenarios are implemented. The descriptive notes help readers understand what each test covers without needing to navigate the code.
test/savepoint_test.exs (2)
36-39: Centralised cleanup looks good; please verify helper semantics match the old behaviour.This is a nice simplification, but it’s worth confirming
EctoLibSql.TestHelpers.cleanup_db_files/1(a) removes-shm/-walas before, and (b) safely ignores missing files (eg if SQLite didn’t create WAL files). Based on learnings, ensure cleanup remains robust under concurrent/failed tests.
335-347: Double-check whether discarding the error state is always safe before savepoint rollback.The comment clarifies intent, but the test still assumes rolling back via the pre-error
trx_stateis valid. If the NIF returns a mutated transaction state on errors, you may need to rebind and use it forrollback_to_savepoint_by_name/2(or explicitly assert the state is unchanged). Based on learnings, this is exactly the “rebind vs discard” boundary that can bite later.test/pool_load_test.exs (1)
19-39: Setup/teardown is solid and nicely isolated per test DB.Cleanup via
EctoLibSql.TestHelpers.cleanup_db_files/1+ disconnect inon_exitis the right direction for keeping the suite tidy. Based on learnings, this aligns with the centralised cleanup approach.test/savepoint_replication_test.exs (3)
210-238: Rollback savepoint operation returns correctly; proposed change contradicts actual function signature.The
rollback_to_savepoint_by_namefunction returns:okon success and{:error, reason}on failure—it does not return state. This is consistent withcreate_savepointand other savepoint operations. The test code correctly uses:okpattern matching and already handles state rebinding correctly from thehandle_executeerror tuple. The proposed diff would introduce a type mismatch with the actual NIF implementation.Likely an incorrect or invalid review comment.
112-127: No changes required—the test code is correct.The savepoint functions (
create_savepoint,rollback_to_savepoint_by_name,release_savepoint_by_name) return:okor{:error, reason}and do not return updated state. The current test usage is correct. Additionally,commit/1returns{:ok, String.t()}, not:ok, which the test correctly handles with{:ok, _state}.Likely an incorrect or invalid review comment.
67-84: The proposed fix does not match the actual API signatures of these functions.The review's analysis is incorrect:
create_savepointandrelease_savepoint_by_namecorrectly return:okor{:error, reason}, not{:ok, new_state}. The test's:ok =matching is correct.commitreturns{:ok, String.t()}(a success message string per the spec and Rust implementation) not{:ok, State.t()}. The test's{:ok, _state} =matching is incorrect, but not in the way the review suggests.The actual issue is that
commitreturns a string message ("commit success"), so the test should match with{:ok, _msg}rather than{:ok, _state}. The proposed diff incorrectly suggestscreate_savepointandrelease_savepoint_by_namereturn state tuples when they do not.Likely an incorrect or invalid review comment.
cursor_streaming_large_test.exs: - Add error handling tests for malformed SQL, syntax errors, non-existent tables - Add edge case tests for empty result sets, NULL values, empty strings - Add transaction behavior tests for cursor after rollback - Add concurrency tests for multiple cursors with interleaved fetches pool_load_test.exs: - Add concurrent load tests for NULL-only and empty-string-only values - Add large dataset test (500 rows across 5 connections) - Add type conversion tests (ints, floats, timestamps) - Add transaction rollback tests including constraint violations - Add mixed commit/rollback consistency tests
Ensure close_stmt is always called regardless of execute_stmt success/failure by wrapping the execution loop in try/after block.
Apply same try/after pattern to ensure close_stmt is always called.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @test/cursor_streaming_large_test.exs:
- Line 494: Change any American English spelling in the test descriptions and
documentation to British English; specifically rename the describe block "cursor
transaction behavior" to "cursor transaction behaviour" (and update any other
comments/strings in this file that use American spellings to their
British/Australian equivalents).
In @test/pool_load_test.exs:
- Around line 305-307: The test is asserting the number of result rows instead
of the COUNT(*) value — verify_result.rows will always be a single row; extract
the count from verify_result.rows (the first row's first column), convert it to
an integer if it's returned as a string or numeric type, and assert that that
extracted count is > 0; reference verify_result and verify_result.rows when
locating the code to change.
- Around line 1279-1289: The test case is matching the wrong error tuple shape;
change the pattern matching on result from a 4-tuple to the 3-tuple shape
produced by handle_execute so the error branch actually matches. Update the
clause that currently reads {:error, _query, _reason, trx_state} to match
{:error, _reason, trx_state} and ensure you still call
EctoLibSql.Native.rollback(trx_state) and return {:ok, :correctly_rolled_back};
leave the success branch ({:ok, _query, _result, trx_state}) unchanged.
🧹 Nitpick comments (7)
test/savepoint_replication_test.exs (5)
19-19: Consider using||for consistency with Elixir conventions.Whilst
orworks correctly here,||is more commonly used in Elixir for boolean operations and would be more idiomatic.♻️ Proposed refactor
- @moduletag skip: is_nil(@turso_uri) || is_nil(@turso_token) + @moduletag skip: is_nil(@turso_uri) || is_nil(@turso_token)
27-27: Consider using||for consistency with Elixir conventions.Similar to line 19, using
||instead oforwould be more idiomatic for boolean operations in Elixir.♻️ Proposed refactor
- if not (is_nil(@turso_uri) or is_nil(@turso_token)) do + if not (is_nil(@turso_uri) || is_nil(@turso_token)) do
49-69: Consider consolidating cleanup logic for better readability.Whilst the current cleanup approach is functional and correctly handles failures, consolidating the multiple try/rescue blocks could improve maintainability.
♻️ Proposed refactor
on_exit(fn -> - # Drop remote table to clean up Turso database - try do - EctoLibSql.handle_execute( - "DROP TABLE IF EXISTS #{test_table}", - [], - [], - state - ) - rescue - _ -> :ok - end - - try do - EctoLibSql.disconnect([], state) - rescue - _ -> :ok - end - + # Clean up remote table and disconnect + catch_error = fn fun -> + try do + fun.() + rescue + _ -> :ok + end + end + + catch_error.(fn -> + EctoLibSql.handle_execute( + "DROP TABLE IF EXISTS #{test_table}", + [], + [], + state + ) + end) + + catch_error.(fn -> EctoLibSql.disconnect([], state) end) + EctoLibSql.TestHelpers.cleanup_db_files(test_db) end)
74-203: Consider adding verification that remote sync actually occurred.Whilst these tests validate savepoint behavior, they don't verify that data was actually synchronized to the remote Turso database. The tests assume sync succeeded but don't confirm it. Consider adding a verification step that queries the remote database directly to ensure replication occurred.
225-234: Consider asserting on the specific error reason.Whilst the test correctly handles the error scenario, it only verifies that an error occurred without checking the error reason. Consider asserting on the expected constraint violation error to make the test more robust.
♻️ Proposed enhancement
result = EctoLibSql.handle_execute( "INSERT INTO #{table} (id, name) VALUES (?, ?)", [100, "Duplicate"], [], trx_state ) # Rebind trx_state - error tuple contains updated transaction state needed for recovery - assert {:error, _reason, trx_state} = result + assert {:error, %{sqlite: %{message: message}}, trx_state} = result + assert message =~ ~r/UNIQUE constraint failed|PRIMARY KEY/itest/pool_load_test.exs (2)
17-17: Unused alias declaration.The
alias EctoLibSqlon line 17 is not used anywhere in the module — all references use the full module nameEctoLibSql. Consider removing it or actually using the alias throughout the file for consistency.♻️ Suggested fix
- alias EctoLibSql
800-810: State not properly chained through sequential inserts.Each call to
insert_edge_case_value(trx_state, value)uses the originaltrx_staterather than threading the updated state from the previous insert. While this may work due to how the underlying connection tracks transaction state by ID, it's inconsistent with the pattern used elsewhere in these tests and could mask issues if state tracking changes.Consider threading the state through the fold:
♻️ Suggested improvement
- insert_results = - Enum.map(edge_values, fn value -> - {:ok, _query, _result, new_state} = insert_edge_case_value(trx_state, value) - new_state - end) - - # Use final state after all inserts - final_trx_state = List.last(insert_results) || trx_state + final_trx_state = + Enum.reduce(edge_values, trx_state, fn value, current_state -> + {:ok, _query, _result, new_state} = insert_edge_case_value(current_state, value) + new_state + end)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/cursor_streaming_large_test.exstest/pool_load_test.exstest/savepoint_replication_test.exs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/cursor_streaming_large_test.exstest/pool_load_test.exstest/savepoint_replication_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/cursor_streaming_large_test.exstest/pool_load_test.exstest/savepoint_replication_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/cursor_streaming_large_test.exstest/pool_load_test.exstest/savepoint_replication_test.exs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
test/cursor_streaming_large_test.exstest/pool_load_test.exstest/savepoint_replication_test.exs
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Validate and document unsupported Ecto features by returning `:unsupported` atom errors in Rust implementations with clear documentation and comprehensive tests
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.393Z
Learning: Applies to priv/repo/migrations/*.exs : Use STRICT tables in migrations for type safety when database schema stability is critical
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use cursor streaming for large result sets to avoid loading entire datasets into memory
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exstest/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exstest/savepoint_replication_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exstest/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Use proper error handling patterns with case matching and with clauses to handle both success and error tuples from NIF operations
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Applied to files:
test/pool_load_test.exstest/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
test/pool_load_test.exstest/savepoint_replication_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use savepoints for partial rollback within transactions to enable error recovery patterns in batch operations
Applied to files:
test/pool_load_test.exstest/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Always format Elixir code before committing using `mix format --check-formatted`
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*.exs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-08T04:56:27.393Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.393Z
Learning: Applies to priv/repo/migrations/*.exs : Use STRICT tables in migrations for type safety when database schema stability is critical
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/turso_remote_test.exs : Turso remote tests should be skipped by default if credentials are missing and only run when explicitly needed due to cost implications
Applied to files:
test/savepoint_replication_test.exs
🧬 Code graph analysis (2)
test/cursor_streaming_large_test.exs (1)
lib/ecto_libsql.ex (6)
handle_execute(148-188)disconnect(131-133)handle_declare(466-484)handle_fetch(415-442)handle_begin(312-317)handle_rollback(350-358)
test/pool_load_test.exs (2)
lib/ecto_libsql.ex (2)
handle_execute(148-188)disconnect(131-133)lib/ecto_libsql/native.ex (6)
begin(828-838)commit(853-857)prepare(951-959)execute_stmt(991-1020)close_stmt(1080-1082)rollback(870-874)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (15)
test/savepoint_replication_test.exs (3)
1-12: LGTM: Clear documentation following British English standards.The module documentation clearly articulates the test focus areas and credential requirements, using proper British English spelling ("behaviour").
74-203: Well-structured test coverage for savepoint scenarios with replication.The three tests comprehensively cover the essential savepoint behaviors: basic operations, rollback preservation of outer transactions, and nested savepoint handling. The test logic correctly uses the original connection state for verification queries after commit.
206-261: Excellent error recovery pattern with correct state handling.This test demonstrates the proper error recovery pattern using savepoints. The critical rebinding of
trx_statefrom the error tuple (line 234) is correctly implemented, allowing the transaction to continue after rolling back to the savepoint. This follows the error recovery pattern described in the learnings.Based on learnings, savepoints enable error recovery patterns in batch operations.
test/cursor_streaming_large_test.exs (6)
1-34: Excellent test setup and isolation strategy.The use of an in-memory database with proper cleanup in
on_exitensures test isolation. The table schema appropriately supports the various test scenarios (filtering, ordering, binary data, etc.).
36-319: Comprehensive coverage of cursor streaming scenarios.This test suite thoroughly validates cursor streaming across diverse SQL operations: large result sets, varied batch sizes, filters, joins, aggregations, computed columns, and query lifecycle. The progression from simple to complex queries demonstrates robust testing methodology.
321-492: Thorough error handling and edge case validation.The error handling tests properly use pattern matching on
{:error, _reason, _state}tuples, and the edge case coverage (NULL values, empty strings, mixed scenarios) aligns excellently with the testing requirements. The distinction between NULL and empty strings is particularly well-validated.
495-560: Excellent transaction rollback and isolation coverage.The transaction tests properly validate cursor behaviour after rollback (with graceful handling of multiple possible outcomes) and verify that cursors see uncommitted changes within the same transaction. This aligns with the transaction rollback scenario requirements from the learnings.
562-676: Comprehensive concurrent operation validation.The concurrent cursor tests thoroughly validate multiple cursors with interleaved fetches, separate queries, and cursor isolation from data modifications. This meets the concurrent operations requirement from the learnings and demonstrates robust multi-cursor handling.
682-802: Well-designed helper functions with efficiency optimisations.The helper functions demonstrate excellent engineering:
insert_rowsuses prepared statements to reduce overhead (lines 684-688, cleanup at 706)- Accumulator patterns in
fetch_all_ids(736-755) andfetch_all_cursor_rows(757-774) avoid O(n²) list concatenation- Pattern matching on
{:ok, _}at line 694 is acceptable for test simplicity per the learnings- Semantic alias functions (776-791) improve readability
test/pool_load_test.exs (6)
45-91: LGTM!The helper functions provide good coverage of edge cases including NULL values, empty strings, large strings, special characters, and various Unicode encodings. The separation of concerns is clear.
311-400: LGTM!The long-running operation tests properly handle transactions with explicit commit result checking and appropriate timeouts. The error handling using
casestatements andflunk/1provides clear failure messages.
402-576: LGTM!The connection recovery tests comprehensively verify that connections remain functional after query errors. The use of pin operators to confirm state consistency is a good practice.
578-721: LGTM!The resource cleanup tests properly verify prepared statement lifecycle management under concurrent load, including creation, execution with edge-case data, and explicit cleanup via
close_stmt.
873-1101: LGTM!The concurrent load edge case tests provide excellent coverage including NULL-only inserts, empty strings, large datasets (500 rows), and type conversion with boundary values. The extended timeouts for large dataset tests are appropriate.
1103-1370: Transaction rollback tests provide good coverage.The rollback tests comprehensively cover: pure rollback scenarios, mixed commit/rollback patterns, constraint violation handling, and edge-case data rollback. The test at lines 1230-1313 has an error tuple pattern issue (flagged separately), but the overall structure is sound.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/pool_load_test.exs (3)
291-308: Consider more specific Unicode data verification.Line 307 only verifies that some rows exist (
length(verify_result.rows) > 0), but doesn't confirm the Unicode characters are correctly preserved. Consider asserting a specific count or verifying that specific Unicode patterns were retrieved correctly.♻️ More specific verification
- # Should retrieve some of the Unicode values - # (exact count depends on LIKE behavior with Unicode) - assert length(verify_result.rows) > 0 + # Should retrieve rows matching Unicode patterns + # Expected: 5 tasks × at least 4 matching patterns = at least 20 rows + [[count]] = verify_result.rows + assert count >= 20, "Expected at least 20 rows with Unicode patterns, got #{count}"
816-826: Consider using Enum.reduce for cleaner state threading.The current pattern collects all intermediate states into a list and then takes the last one. This works but is somewhat awkward. Using
Enum.reducewould more clearly express the intent of threading state through multiple operations.♻️ Cleaner state threading with Enum.reduce
- # Insert edge-case values within transaction - edge_values = generate_edge_case_values(task_num) - - insert_results = - Enum.map(edge_values, fn value -> - {:ok, _query, _result, new_state} = insert_edge_case_value(trx_state, value) - new_state - end) - - # Use final state after all inserts - final_trx_state = List.last(insert_results) || trx_state + # Insert edge-case values within transaction, threading state through + edge_values = generate_edge_case_values(task_num) + + final_trx_state = + Enum.reduce(edge_values, trx_state, fn value, acc_state -> + {:ok, _query, _result, new_state} = insert_edge_case_value(acc_state, value) + new_state + end)
1347-1356: State should be threaded through transaction operations.Lines 1350-1353 reuse the same
trx_statefor all inserts without threading the updated state through. Whilst this works in a rollback scenario, it's not following best practices for state management in Elixir.♻️ Thread state through inserts
# Insert edge-case values in transaction edge_values = generate_edge_case_values(task_num) - _insert_results = - Enum.map(edge_values, fn value -> - insert_edge_case_value(trx_state, value) + final_trx_state = + Enum.reduce(edge_values, trx_state, fn value, acc_state -> + {:ok, _query, _result, new_state} = insert_edge_case_value(acc_state, value) + new_state end) # Always rollback - edge-case data should not persist - case EctoLibSql.Native.rollback(trx_state) do + case EctoLibSql.Native.rollback(final_trx_state) do
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/pool_load_test.exs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/pool_load_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/pool_load_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/pool_load_test.exs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
test/pool_load_test.exs
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use savepoints for partial rollback within transactions to enable error recovery patterns in batch operations
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Use proper error handling patterns with case matching and with clauses to handle both success and error tuples from NIF operations
Applied to files:
test/pool_load_test.exs
🧬 Code graph analysis (1)
test/pool_load_test.exs (2)
lib/ecto_libsql.ex (2)
handle_execute(148-188)disconnect(131-133)lib/ecto_libsql/native.ex (6)
begin(828-838)commit(853-857)prepare(951-959)execute_stmt(991-1020)close_stmt(1080-1082)rollback(870-874)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (7)
test/pool_load_test.exs (7)
1-39: LGTM! Excellent test setup and documentation.The module documentation clearly outlines critical scenarios, and the setup properly creates isolated test databases with cleanup via
TestHelpers.cleanup_db_files. The use ofon_exitensures resources are cleaned up even if tests fail.
45-91: LGTM! Comprehensive edge case helpers.The helper functions provide excellent coverage of edge cases including NULL values, empty strings, large datasets, and Unicode data (Chinese, Arabic, emoji). This aligns well with testing requirements.
311-400: LGTM! Robust long-running operation tests.The tests properly handle transaction duration scenarios with explicit error handling (lines 361-367, 377-388) and clear failure messages using
flunk. The pattern of explicitly matching commit results is excellent for debugging.
402-576: LGTM! Thorough connection recovery testing.The tests comprehensively verify that connections remain functional after errors, including with edge-case data (NULL, empty strings, large values). The pattern of explicit error matching and recovery verification (lines 531-542) ensures robust error handling.
578-738: LGTM! Excellent prepared statement resource management.The tests properly verify prepared statement cleanup under concurrent load with defensive error handling (lines 606-610, 686-690). Testing with edge-case data ensures the prepared statement path handles NULL and special values correctly.
889-1117: LGTM! Comprehensive concurrent load edge case testing.Excellent coverage of edge cases under concurrent load:
- NULL values vs empty strings properly distinguished (lines 971-982)
- Large datasets (500 rows) validate scalability
- Type conversions verify data integrity across int/float/text types
- Maximum value boundaries tested (line 1079)
This thoroughly addresses the edge case testing requirements.
1246-1329: LGTM! Excellent constraint violation handling.The test properly verifies that rollback occurs after constraint violations (lines 1295-1299), and confirms that only the original row persists (line 1328). This validates proper error handling and transactional integrity.
- cursor_streaming_large_test.exs: 'behavior' -> 'behaviour' - pool_load_test.exs: Extract COUNT(*) value from result.rows properly - pool_load_test.exs: Fix error tuple from 4-tuple to 3-tuple pattern
- Verify specific Unicode patterns (café, 中文, العربية, emoji) are preserved - Use Enum.reduce for cleaner state threading in concurrent transactions - Thread state properly through transaction operations before rollback
- Use || instead of 'or' for idiomatic boolean operations - Consolidate cleanup logic into single for loop with try/rescue - Add sync verification via max_write_replication_index after commits - Assert specific constraint violation error reason
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/savepoint_replication_test.exs (1)
1-277: Consider expanding test coverage for edge cases.The current tests effectively cover savepoint replication behaviour with happy paths, constraint violations, and rollback scenarios. However, per the coding guidelines, consider adding test cases for:
- NULL values in savepoint operations (e.g., inserting NULL into TEXT columns)
- Empty strings in replicated transactions
- Type conversions during savepoint operations
- Large datasets to verify replication frame handling under load
These additions would provide more comprehensive coverage of edge cases that could surface issues in production replication scenarios.
As per coding guidelines, tests should cover edge cases including NULL values, empty strings, large datasets, type conversions, and concurrent operations.
test/cursor_streaming_large_test.exs (1)
759-774: Consider matching the efficiency pattern offetch_all_ids_acc.The
fetch_all_cursor_rows_accfunction usesEnum.reverse(result.rows) ++ accon each iteration (line 768, 772), which is correct but less efficient than the nested list pattern used infetch_all_ids_acc(lines 744-755). For consistency and a slight performance improvement, consider collecting batches as nested lists (like[result.rows | acc]) and flattening once at the end.♻️ Optional refactor to match fetch_all_ids_acc pattern
defp fetch_all_cursor_rows_acc(state, cursor, query, opts, acc) do case EctoLibSql.handle_fetch(query, cursor, opts, state) do {:cont, result, next_state} -> - # Prepend reversed batch to accumulator to maintain order - new_acc = Enum.reverse(result.rows) ++ acc - fetch_all_cursor_rows_acc(next_state, cursor, query, opts, new_acc) + # Collect batches as nested lists to avoid intermediate reversals + fetch_all_cursor_rows_acc(next_state, cursor, query, opts, [result.rows | acc]) {:halt, result, _state} -> - Enum.reverse(result.rows) ++ acc + [result.rows | acc] |> Enum.reverse() |> List.flatten() end endThen update the calling function:
defp fetch_all_cursor_rows(state, cursor, query, opts) do fetch_all_cursor_rows_acc(state, cursor, query, opts, []) - |> Enum.reverse() endtest/pool_load_test.exs (1)
242-245: Consider handling the DELETE result for test robustness.While it's acceptable to skip error handling in test setup, capturing the result might help diagnose issues if the cleanup operation fails:
♻️ Optional improvement
- {:ok, state} = EctoLibSql.connect(database: test_db, busy_timeout: 30_000) - EctoLibSql.handle_execute("DELETE FROM test_data", [], [], state) - EctoLibSql.disconnect([], state) + {:ok, state} = EctoLibSql.connect(database: test_db, busy_timeout: 30_000) + {:ok, _query, _result, _state} = EctoLibSql.handle_execute("DELETE FROM test_data", [], [], state) + EctoLibSql.disconnect([], state)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/cursor_streaming_large_test.exstest/pool_load_test.exstest/savepoint_replication_test.exs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/cursor_streaming_large_test.exstest/savepoint_replication_test.exstest/pool_load_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/cursor_streaming_large_test.exstest/savepoint_replication_test.exstest/pool_load_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/cursor_streaming_large_test.exstest/savepoint_replication_test.exstest/pool_load_test.exs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
test/cursor_streaming_large_test.exstest/savepoint_replication_test.exstest/pool_load_test.exs
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use cursor streaming for large result sets to avoid loading entire datasets into memory
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
test/cursor_streaming_large_test.exstest/savepoint_replication_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/cursor_streaming_large_test.exstest/savepoint_replication_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/cursor_streaming_large_test.exstest/savepoint_replication_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Use proper error handling patterns with case matching and with clauses to handle both success and error tuples from NIF operations
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use savepoints for partial rollback within transactions to enable error recovery patterns in batch operations
Applied to files:
test/savepoint_replication_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Always format Elixir code before committing using `mix format --check-formatted`
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*.exs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-08T04:56:27.393Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.393Z
Learning: Applies to priv/repo/migrations/*.exs : Use STRICT tables in migrations for type safety when database schema stability is critical
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/turso_remote_test.exs : Turso remote tests should be skipped by default if credentials are missing and only run when explicitly needed due to cost implications
Applied to files:
test/savepoint_replication_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Applied to files:
test/savepoint_replication_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
test/savepoint_replication_test.exstest/pool_load_test.exs
🧬 Code graph analysis (1)
test/pool_load_test.exs (2)
lib/ecto_libsql.ex (2)
handle_execute(148-188)disconnect(131-133)lib/ecto_libsql/native.ex (6)
begin(828-838)commit(853-857)prepare(951-959)execute_stmt(991-1020)close_stmt(1080-1082)rollback(870-874)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (15)
test/savepoint_replication_test.exs (3)
1-19: LGTM! Excellent test documentation and skip logic.The module documentation clearly describes the test scenarios using proper British English ("behaviour"), and the skip logic correctly prevents tests from running when Turso credentials are missing, aligning with the guideline to skip remote tests by default when credentials are unavailable.
21-68: Solid setup and cleanup pattern.The setup correctly creates unique databases and tables per test for isolation, and the cleanup logic properly orders operations (drop table → disconnect → cleanup files) with error handling to ensure cleanup never blocks subsequent tests.
213-275: Excellent demonstration of savepoint error recovery pattern.This test correctly demonstrates the error recovery pattern using savepoints in batch operations. The pattern matching on line 243 properly captures the updated transaction state from the error tuple (
{:error, reason, trx_state}), enabling recovery via rollback to savepoint. This aligns with the learning about using savepoints for partial rollback within transactions.test/cursor_streaming_large_test.exs (5)
1-34: LGTM! Excellent test setup.The module structure and setup block are well-designed. The in-memory database with proper cleanup on exit, clear comments explaining test intent, and appropriate use of British English ("behaviour" in line 494) all follow best practices.
36-319: LGTM! Comprehensive streaming test coverage.This test suite excellently validates cursor streaming behaviour across varied scenarios including large datasets (1K, 10K rows), batch size handling, filtering, JOINs, aggregations, and computed columns. The tests properly verify memory-conscious streaming with batched fetches, which aligns with the learning that cursor streaming should be used for large result sets to avoid loading entire datasets into memory.
Based on learnings, these tests appropriately cover the happy path with successful operations across multiple SQL constructs.
321-492: LGTM! Thorough error handling and edge case coverage.The error handling tests properly validate malformed SQL, syntax errors, and non-existent tables with appropriate pattern matching on
{:error, _reason, _state}tuples. Edge case coverage for NULL values, empty strings, and their combinations is excellent, meeting the learnings requirement for comprehensive edge case testing including NULL values and empty strings.Based on learnings, this properly handles database errors with pattern matching on error tuples rather than using bare unwrap operations.
494-560: LGTM! Robust transaction rollback testing.These tests properly validate cursor behaviour within transactions and after rollback, meeting the learnings requirement for transaction rollback scenario coverage. The defensive handling of multiple possible outcomes at lines 519-531 demonstrates good testing practice by accommodating different valid implementation behaviours.
562-676: LGTM! Excellent concurrent operations coverage.The concurrent cursor tests thoroughly validate multiple cursors with interleaved fetches, separate cursors for concurrent tasks, and cursor isolation when data is modified. This meets the learnings requirement for concurrent operations testing and demonstrates proper handling of cursor state across concurrent scenarios.
test/pool_load_test.exs (7)
1-39: LGTM! Well-structured test module with proper setup and cleanup.The module documentation clearly outlines critical scenarios, and the setup block properly creates isolated test databases with unique names to prevent conflicts. The use of
on_exitwithTestHelpers.cleanup_db_filesensures proper resource cleanup.
45-92: LGTM! Comprehensive edge case and Unicode data generators.The helper functions provide excellent coverage of edge cases including NULL values, empty strings, large strings (1KB), special characters, and various Unicode character sets (Latin with accents, Chinese, Arabic, emoji, and mixed). This aligns well with the testing requirements.
310-399: LGTM! Well-designed long-running transaction tests.The tests appropriately validate that long-running operations don't cause timeout issues. The shorter
busy_timeout: 5000in the first test appears intentional for testing timeout behavior, while the concurrent transaction test includes robust error handling with explicitflunkon commit failures.
401-575: LGTM! Comprehensive connection recovery tests.The tests thoroughly validate that connections remain functional after SQL errors. Proper use of pattern matching with the pin operator (
^state) ensures state immutability after errors, and the concurrent recovery test demonstrates that multiple connections can recover independently. Excellent coverage of edge cases including NULL values.
577-737: LGTM! Excellent resource cleanup patterns.The tests demonstrate proper resource management with nested
try/afterblocks ensuring prepared statements are always closed, even rescuing potential errors during cleanup. The edge-case test thoroughly validates NULL value handling and proper data persistence. This is a model for resource cleanup under concurrent load.
739-1113: LGTM! Comprehensive coverage of transaction isolation and edge cases.The tests thoroughly validate transaction isolation under concurrent load and cover extensive edge cases including NULL-only data, empty strings, large datasets (500 rows), and type conversions with boundary values (max int64, max float64). The 60-second timeout for the large dataset test is appropriately sized for the workload.
1115-1383: LGTM! Thorough transaction rollback coverage.The tests comprehensively validate rollback behaviour under concurrent load, including:
- Concurrent rollbacks leaving no data
- Mixed commit/rollback scenarios with proper counting verification
- Rollback after constraint violations (UNIQUE constraint)
- Rollback with edge-case data
The DELETE operations in test setup (lines 1121, 1179, 1332) without error checking are acceptable for test initialisation. This aligns perfectly with the requirement to test transaction rollback scenarios.
Based on learnings, the tests thoroughly cover all required scenarios.
- Refactor fetch_all_cursor_rows_acc to use nested list pattern (matching fetch_all_ids_acc) for better performance - Add pattern match on DELETE result in pool_load_test cleanup for earlier failure detection
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @test/cursor_streaming_large_test.exs:
- Around line 744-755: The helper loop fetch_all_ids_acc/5 currently only
handles {:cont, result, next_state} and {:halt, result, _state} and will raise a
CaseClauseError on {:error, reason, state}; update fetch_all_ids_acc/5 (and the
other helpers fetch_all_cursor_rows_acc/5 and count_batches/4) to explicitly
match {:error, reason, state} and call ExUnit.flunk/1 (or flunk/1) with a
descriptive message that includes the reason (and optionally state) so test
failures are reported clearly instead of raising a case-clause error. Ensure the
new clause returns or halts appropriately after flunking.
- Around line 682-708: The insert_rows function can leak the prepared statement
if an insert raises mid-loop; wrap the loop that calls
EctoLibSql.Native.execute_stmt in a try...after (or try do ... rescue ... after)
so that EctoLibSql.Native.close_stmt(stmt) always runs regardless of errors,
e.g., prepare the statement with EctoLibSql.Native.prepare, then in a try do
perform the Enum.reduce/Enum.each executing execute_stmt, and in the after block
call EctoLibSql.Native.close_stmt(stmt) to ensure cleanup even on failure.
- Around line 9-34: The on_exit currently closes using the original setup
`state`, which can become stale; instead extract and store the connection
identifier from the initial `state` (e.g., `conn_id = state.conn_id` or
pattern-match the tuple returned by EctoLibSql.connect) and call
EctoLibSql.disconnect([conn_id]) inside on_exit so you always teardown the
specific connection created by setup (update the `setup` block to return the
conn_id and use that in on_exit rather than the entire `state` variable).
In @test/pool_load_test.exs:
- Around line 19-39: The test setup ignores the updated connection state
returned by EctoLibSql.handle_execute/4 and later calls EctoLibSql.disconnect/2
with the original state; capture the fourth element from handle_execute/4 (e.g.
pattern match {:ok, _query, _result, state} or rename to new_state) and use that
updated state in the on_exit callback when calling EctoLibSql.disconnect([],
state) so disconnect gets the latest connection state.
🧹 Nitpick comments (3)
test/cursor_streaming_large_test.exs (1)
650-675: “Cursor isolation” assertion doesn’t assert isolation.
assert total_fetched >= 50will pass even if the cursor is badly broken; it also doesn’t distinguish “snapshot” vs “sees new rows” semantics. Consider asserting an allowed range (e.g.,100..200) or splitting into two explicit expectations depending on intended behaviour.test/pool_load_test.exs (2)
75-91: Minor DRY: duplicated insert helpers can be one function.
insert_unicode_edge_case_value/2andinsert_edge_case_value/2are identical; consider keeping just one.
93-311: Consider enabling WAL mode in the test setup to reduce flakiness under concurrent load.File-based SQLite benefits significantly from Write-Ahead Logging when handling simultaneous writers. The current setup relies solely on per-connection
busy_timeout(30,000 ms), which doesn't address the underlying lock contention with multiple concurrent writers. Enable WAL mode in the setup block usingEctoLibSql.Pragma.set_journal_mode(state, :wal)before test table creation, and optionally pair it withEctoLibSql.Pragma.set_synchronous(state, :normal)for the test environment. This approach is already used intest/ecto_sql_transaction_compat_test.exsand aligns with the codebase's concurrency patterns.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/cursor_streaming_large_test.exstest/pool_load_test.exs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use cursor streaming for large result sets to avoid loading entire datasets into memory
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Use proper error handling patterns with case matching and with clauses to handle both success and error tuples from NIF operations
Applied to files:
test/cursor_streaming_large_test.exstest/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use savepoints for partial rollback within transactions to enable error recovery patterns in batch operations
Applied to files:
test/pool_load_test.exs
🧬 Code graph analysis (1)
test/cursor_streaming_large_test.exs (2)
lib/ecto_libsql.ex (6)
handle_execute(148-188)disconnect(131-133)handle_declare(466-484)handle_fetch(415-442)handle_begin(312-317)handle_rollback(350-358)lib/ecto_libsql/native.ex (3)
prepare(951-959)execute_stmt(991-1020)close_stmt(1080-1082)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Rust Fuzz Testing
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
test/pool_load_test.exs (1)
2-14: Module doc looks good (AU spelling, clear intent).This reads well and matches the stated test scope.
cursor_streaming_large_test.exs: - Add error handling to fetch_all_ids_acc, fetch_all_cursor_rows_acc, and count_batches to report failures clearly instead of CaseClauseError - Wrap insert_rows in try/after to ensure prepared statement cleanup on error - Fix on_exit to use conn_id instead of potentially stale state pool_load_test.exs: - Capture updated state from handle_execute and use conn_id in on_exit for reliable connection cleanup
- Use proper EctoLibSql.State struct in on_exit disconnect calls - Replace List.flatten with Enum.concat for single-level flattening in fetch_all_cursor_rows and fetch_all_ids helpers (List.flatten was incorrectly flattening row data like [[1, 'a'], [2, 'b']] into [1, 'a', 2, 'b'])
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @test/cursor_streaming_large_test.exs:
- Around line 762-782: fetch_all_cursor_rows/4 currently reverses the
accumulated batches then calls List.flatten/1 which deep-flattens each row;
instead preserve row structure by concatenating only the outer batch level.
Change fetch_all_cursor_rows to reverse the accumulator from
fetch_all_cursor_rows_acc and then use a one-level concat (e.g. Enum.concat or
similar) rather than List.flatten so callers still receive [[id, value], ...];
update references to fetch_all_cursor_rows and fetch_all_cursor_rows_acc
accordingly.
- Around line 684-812: The helper functions (insert_rows/4, fetch_all_rows/4,
fetch_remaining_rows/4, fetch_all_ids/4, fetch_all_ids_acc/5,
fetch_all_cursor_rows/4, fetch_all_cursor_rows_acc/5, fetch_all_binary_rows/4,
fetch_all_computed_rows/4, fetch_all_group_rows/4, fetch_all_distinct_rows/4,
count_batches/4) follow Elixir snake_case; align the repo guideline to the
language convention by updating the style guide to require snake_case for Elixir
helpers (or, if the project truly prefers camelCase, rename these functions
consistently across the test suite and any callers)—pick one approach and make
the naming consistent across the codebase and the guideline.
- Around line 30-33: The on_exit teardown currently passes a plain map to
EctoLibSql.disconnect which expects a %EctoLibSql.State{} struct; replace the
map argument with an actual %EctoLibSql.State{conn_id: conn_id} (or obtain the
real state used in the test) so the pattern match in EctoLibSql.disconnect/2
succeeds and teardown won't crash.
In @test/pool_load_test.exs:
- Around line 48-95: The helper names use snake_case and duplicate logic: rename
generate_edge_case_values/1 -> generateEdgeCaseValues/1 and
generate_unicode_edge_case_values/1 -> generateUnicodeEdgeCaseValues/1 to follow
the camelCase guideline, and consolidate the identical insert_edge_case_value/2
and insert_unicode_edge_case_value/2 into a single insertEdgeCaseValue/2 (or
similarly camelCased) function; replace all callers to use the new camelCase
names and the single insert function so there’s no duplicated implementation.
- Around line 316-405: The test names claim “timeout issues” but the bodies only
sleep inside transactions and assert commits; either rename the two tests to
reflect they check long-held transactions/commit behavior (change "long
transaction doesn't cause timeout issues" to e.g. "long transaction doesn't
prevent commit" and change "multiple concurrent transactions complete despite
duration" to e.g. "multiple concurrent transactions complete despite holding"),
or if you actually want to test timeout behavior, modify the tests to set a
small busy_timeout on the connection and perform a DB-blocking operation that
exceeds that timeout (then assert the connection returns a timeout/error),
updating the test bodies around EctoLibSql.connect, EctoLibSql.Native.begin, and
EctoLibSql.handle_execute accordingly.
- Around line 19-42: The setup opens a DB connection and leaves it open until
on_exit, skewing load tests and misusing disconnect/2 by passing a map; after
creating the schema with EctoLibSql.handle_execute in setup, call
EctoLibSql.disconnect([], state) immediately to close the connection, and change
the on_exit callback to only call
EctoLibSql.TestHelpers.cleanup_db_files(test_db); remove or ignore the
%{conn_id: conn_id} disconnect and ensure you pass the full state struct
(variable state) to disconnect when closing the connection.
- Around line 1125-1130: The test is discarding results from DELETE operations
and ignoring EctoLibSql.rollback results, which can mask adapter failures and
leave transactions open; update each call to EctoLibSql.handle_execute("DELETE
FROM test_data", [], [], state) to capture and assert its result (e.g.,
pattern-match on {:ok, _} or assert success) instead of discarding it, and in
the constraint-violation test capture the result of EctoLibSql.rollback (in both
case branches) and assert it succeeds (or handle {:error, reason} explicitly),
ensuring you also propagate or handle any error so transactions are not left
open; locate these calls by the function names EctoLibSql.handle_execute and
EctoLibSql.rollback in test/pool_load_test.exs and replace the ignored calls
with proper pattern matches or assertions.
🧹 Nitpick comments (4)
test/cursor_streaming_large_test.exs (3)
615-650: Test name says “concurrent tasks”, but there’s no concurrency here.This test is currently sequential cursor usage, not Task-based concurrency (Line 615). Either rename it to reflect interleaving/sequential behaviour, or make it genuinely concurrent (likely needing a shared DB that supports multiple connections, not
:memory:per-connection semantics). Based on learnings, concurrency coverage is expected for tests.
54-65: Potentially slow CI: 10k row inserts are per-row NIF calls; consider a transaction and/or a:slowtag.The 10k insert loop (Line 54 onward; implemented in
insert_rows/4Lines 684-710) may dominate runtime. Wrapping inserts in a transaction (or reducing dataset sizes while keeping cursor-batching assertions) will usually help.Also applies to: 684-710
712-736: Duplicate helpers:fetch_all_rows/4andfetch_remaining_rows/4are identical.You can keep one and alias the other, or parameterise the small behaviour difference if you intended one.
test/pool_load_test.exs (1)
96-314: Consider threading state updates through sequential operations usingEnum.reducerather thanEnum.mapfor consistency with adapter semantics.While the current code functions correctly (no transactions are active, so state updates are identity operations), the pattern of ignoring returned state from
handle_execute/4is fragile. The adapter contract specifies thathandle_execute/4returns{:ok, query, result, updated_state}or{:error, error, updated_state}, and state carries mutable fields (trx_id,mode,sync) beyondconn_id. If the connection state ever needs to track mutable changes (such as when transactions are introduced), this pattern will fail silently. For example, line 825 elsewhere in the test file demonstrates the correct pattern usingEnum.reduceto thread state through sequential operations. Applying the same pattern here would make the code more robust and consistent with the rest of the codebase.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/cursor_streaming_large_test.exstest/pool_load_test.exs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/pool_load_test.exstest/cursor_streaming_large_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/pool_load_test.exstest/cursor_streaming_large_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/pool_load_test.exstest/cursor_streaming_large_test.exs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
test/pool_load_test.exstest/cursor_streaming_large_test.exs
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
test/pool_load_test.exstest/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Applied to files:
test/pool_load_test.exstest/cursor_streaming_large_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/pool_load_test.exstest/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
test/pool_load_test.exstest/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/pool_load_test.exstest/cursor_streaming_large_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use savepoints for partial rollback within transactions to enable error recovery patterns in batch operations
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
test/pool_load_test.exstest/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Use proper error handling patterns with case matching and with clauses to handle both success and error tuples from NIF operations
Applied to files:
test/pool_load_test.exstest/cursor_streaming_large_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use cursor streaming for large result sets to avoid loading entire datasets into memory
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
test/cursor_streaming_large_test.exs
🧬 Code graph analysis (1)
test/cursor_streaming_large_test.exs (1)
lib/ecto_libsql.ex (6)
handle_execute(148-188)disconnect(131-133)handle_declare(466-484)handle_fetch(415-442)handle_begin(312-317)handle_rollback(350-358)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (rust)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
🔇 Additional comments (2)
test/pool_load_test.exs (2)
1-18: Good test intent and clear module-level scope.
The moduledoc reads well and uses “behaviour” (AU spelling) consistently.
407-450: The state assertion with^stateis correct.The API contract in
handle_executeexplicitly guarantees that on error, the identical state is returned. All error paths in the implementation—whether throughdo_query,do_execute_with_trx, orformat_query_result—return the original state unchanged. Pinning with the pin operator (^state) is therefore the right approach and validates the expected API contract.Likely an incorrect or invalid review comment.
| defp insert_rows(state, start_id, end_id, batch_id) do | ||
| # Use a prepared statement to reduce overhead per insert | ||
| {:ok, stmt} = | ||
| EctoLibSql.Native.prepare( | ||
| state, | ||
| "INSERT INTO large_data (id, batch_id, sequence, value) VALUES (?, ?, ?, ?)" | ||
| ) | ||
|
|
||
| try do | ||
| Enum.reduce(start_id..end_id, state, fn id, acc_state -> | ||
| value = "value_#{id}_batch_#{batch_id}" | ||
|
|
||
| {:ok, _changes} = | ||
| EctoLibSql.Native.execute_stmt( | ||
| acc_state, | ||
| stmt, | ||
| "INSERT INTO large_data (id, batch_id, sequence, value) VALUES (?, ?, ?, ?)", | ||
| [id, batch_id, id - start_id + 1, value] | ||
| ) | ||
|
|
||
| acc_state | ||
| end) | ||
| after | ||
| # Always clean up prepared statement, even on error | ||
| EctoLibSql.Native.close_stmt(stmt) | ||
| end | ||
| end | ||
|
|
||
| defp fetch_all_rows(state, cursor, query, opts) do | ||
| case EctoLibSql.handle_fetch(query, cursor, opts, state) do | ||
| {:cont, result, next_state} -> | ||
| result.num_rows + fetch_all_rows(next_state, cursor, query, opts) | ||
|
|
||
| {:halt, result, _state} -> | ||
| result.num_rows | ||
|
|
||
| {:error, reason, _state} -> | ||
| flunk("Cursor fetch failed with error: #{inspect(reason)}") | ||
| end | ||
| end | ||
|
|
||
| defp fetch_remaining_rows(state, cursor, query, opts) do | ||
| case EctoLibSql.handle_fetch(query, cursor, opts, state) do | ||
| {:cont, result, next_state} -> | ||
| result.num_rows + fetch_remaining_rows(next_state, cursor, query, opts) | ||
|
|
||
| {:halt, result, _state} -> | ||
| result.num_rows | ||
|
|
||
| {:error, reason, _state} -> | ||
| flunk("Cursor fetch failed with error: #{inspect(reason)}") | ||
| end | ||
| end | ||
|
|
||
| defp fetch_all_ids(state, cursor, query, opts) do | ||
| # Use accumulator to avoid O(n²) list concatenation. | ||
| # Collect batches in reverse order, then flatten with nested reverses for correctness. | ||
| fetch_all_ids_acc(state, cursor, query, opts, []) | ||
| |> Enum.reverse() | ||
| |> List.flatten() | ||
| end | ||
|
|
||
| defp fetch_all_ids_acc(state, cursor, query, opts, acc) do | ||
| case EctoLibSql.handle_fetch(query, cursor, opts, state) do | ||
| {:cont, result, next_state} -> | ||
| ids = Enum.map(result.rows, fn [id] -> id end) | ||
| # Collect batches as nested lists to avoid intermediate reversals | ||
| fetch_all_ids_acc(next_state, cursor, query, opts, [ids | acc]) | ||
|
|
||
| {:halt, result, _state} -> | ||
| ids = Enum.map(result.rows, fn [id] -> id end) | ||
| [ids | acc] | ||
|
|
||
| {:error, reason, _state} -> | ||
| flunk("Cursor fetch failed in fetch_all_ids_acc: #{inspect(reason)}") | ||
| end | ||
| end | ||
|
|
||
| # Generic helper to collect all rows from a cursor by repeatedly fetching batches | ||
| # Uses nested list pattern to avoid O(n²) list concatenation with ++ | ||
| defp fetch_all_cursor_rows(state, cursor, query, opts) do | ||
| fetch_all_cursor_rows_acc(state, cursor, query, opts, []) | ||
| |> Enum.reverse() | ||
| |> List.flatten() | ||
| end | ||
|
|
||
| defp fetch_all_cursor_rows_acc(state, cursor, query, opts, acc) do | ||
| case EctoLibSql.handle_fetch(query, cursor, opts, state) do | ||
| {:cont, result, next_state} -> | ||
| # Collect batches as nested lists to avoid intermediate reversals | ||
| fetch_all_cursor_rows_acc(next_state, cursor, query, opts, [result.rows | acc]) | ||
|
|
||
| {:halt, result, _state} -> | ||
| [result.rows | acc] | ||
|
|
||
| {:error, reason, _state} -> | ||
| flunk("Cursor fetch failed in fetch_all_cursor_rows_acc: #{inspect(reason)}") | ||
| end | ||
| end | ||
|
|
||
| # Aliases for backwards compatibility and semantic clarity | ||
| defp fetch_all_binary_rows(state, cursor, query, opts) do | ||
| fetch_all_cursor_rows(state, cursor, query, opts) | ||
| end | ||
|
|
||
| defp fetch_all_computed_rows(state, cursor, query, opts) do | ||
| fetch_all_cursor_rows(state, cursor, query, opts) | ||
| end | ||
|
|
||
| defp fetch_all_group_rows(state, cursor, query, opts) do | ||
| fetch_all_cursor_rows(state, cursor, query, opts) | ||
| end | ||
|
|
||
| defp fetch_all_distinct_rows(state, cursor, query, opts) do | ||
| fetch_all_cursor_rows(state, cursor, query, opts) | ||
| end | ||
|
|
||
| defp count_batches(state, cursor, query, opts) do | ||
| case EctoLibSql.handle_fetch(query, cursor, opts, state) do | ||
| {:cont, _result, next_state} -> | ||
| 1 + count_batches(next_state, cursor, query, opts) | ||
|
|
||
| {:halt, _result, _state} -> | ||
| 1 | ||
|
|
||
| {:error, reason, _state} -> | ||
| flunk("Cursor fetch failed in count_batches: #{inspect(reason)}") | ||
| end | ||
| end |
There was a problem hiding this comment.
Naming style: helpers use snake_case, but repo guidelines mention camelCase for Elixir functions.
If the guideline is intentional, these helpers (insert_rows/4, fetch_all_rows/4, etc.) don’t comply; otherwise, consider correcting the guideline (Elixir convention is snake_case). As per coding guidelines, please align one way or the other.
🤖 Prompt for AI Agents
In @test/cursor_streaming_large_test.exs around lines 684 - 812, The helper
functions (insert_rows/4, fetch_all_rows/4, fetch_remaining_rows/4,
fetch_all_ids/4, fetch_all_ids_acc/5, fetch_all_cursor_rows/4,
fetch_all_cursor_rows_acc/5, fetch_all_binary_rows/4, fetch_all_computed_rows/4,
fetch_all_group_rows/4, fetch_all_distinct_rows/4, count_batches/4) follow
Elixir snake_case; align the repo guideline to the language convention by
updating the style guide to require snake_case for Elixir helpers (or, if the
project truly prefers camelCase, rename these functions consistently across the
test suite and any callers)—pick one approach and make the naming consistent
across the codebase and the guideline.
| defp generate_edge_case_values(task_num) do | ||
| [ | ||
| # Normal string | ||
| "normal_value_#{task_num}", | ||
| # NULL value | ||
| nil, | ||
| # Empty string | ||
| "", | ||
| # Large string (1KB) | ||
| String.duplicate("x", 1000), | ||
| # Special characters | ||
| "special_chars_!@#$%^&*()_+-=[]{};" | ||
| ] | ||
| end | ||
|
|
||
| defp generate_unicode_edge_case_values(task_num) do | ||
| [ | ||
| # Latin with accents (ê, á, ü) | ||
| "café_#{task_num}", | ||
| # Chinese characters (中文) | ||
| "chinese_中文_#{task_num}", | ||
| # Arabic characters (العربية) | ||
| "arabic_العربية_#{task_num}", | ||
| # Emoji (😀, 🎉, ❤️) | ||
| "emoji_😀🎉❤️_#{task_num}", | ||
| # Mixed: combining all above | ||
| "mixed_café_中文_العربية_😀_#{task_num}" | ||
| ] | ||
| end | ||
|
|
||
| defp insert_unicode_edge_case_value(state, value) do | ||
| EctoLibSql.handle_execute( | ||
| "INSERT INTO test_data (value) VALUES (?)", | ||
| [value], | ||
| [], | ||
| state | ||
| ) | ||
| end | ||
|
|
||
| defp insert_edge_case_value(state, value) do | ||
| EctoLibSql.handle_execute( | ||
| "INSERT INTO test_data (value) VALUES (?)", | ||
| [value], | ||
| [], | ||
| state | ||
| ) | ||
| end | ||
|
|
There was a problem hiding this comment.
Helper naming doesn’t match the repo guideline (camelCase) and there’s easy dedupe.
If that guideline is intentional, generate_edge_case_values/1, generate_unicode_edge_case_values/1, etc. should be renamed. Also insert_unicode_edge_case_value/2 and insert_edge_case_value/2 are identical.
🤖 Prompt for AI Agents
In @test/pool_load_test.exs around lines 48 - 95, The helper names use
snake_case and duplicate logic: rename generate_edge_case_values/1 ->
generateEdgeCaseValues/1 and generate_unicode_edge_case_values/1 ->
generateUnicodeEdgeCaseValues/1 to follow the camelCase guideline, and
consolidate the identical insert_edge_case_value/2 and
insert_unicode_edge_case_value/2 into a single insertEdgeCaseValue/2 (or
similarly camelCased) function; replace all callers to use the new camelCase
names and the single insert function so there’s no duplicated implementation.
| describe "long-running operations" do | ||
| @tag :slow | ||
| @tag :flaky | ||
| test "long transaction doesn't cause timeout issues", %{test_db: test_db} do | ||
| {:ok, state} = EctoLibSql.connect(database: test_db, busy_timeout: 5000) | ||
|
|
||
| try do | ||
| # Start longer transaction | ||
| {:ok, trx_state} = EctoLibSql.Native.begin(state) | ||
|
|
||
| {:ok, _query, _result, trx_state} = | ||
| EctoLibSql.handle_execute( | ||
| "INSERT INTO test_data (value, duration) VALUES (?, ?)", | ||
| ["long", 100], | ||
| [], | ||
| trx_state | ||
| ) | ||
|
|
||
| # Simulate some work | ||
| Process.sleep(100) | ||
|
|
||
| {:ok, _committed_state} = EctoLibSql.Native.commit(trx_state) | ||
| after | ||
| EctoLibSql.disconnect([], state) | ||
| end | ||
| end | ||
|
|
||
| @tag :slow | ||
| @tag :flaky | ||
| test "multiple concurrent transactions complete despite duration", %{test_db: test_db} do | ||
| tasks = | ||
| Enum.map(1..3, fn i -> | ||
| Task.async(fn -> | ||
| {:ok, state} = EctoLibSql.connect(database: test_db, busy_timeout: 30_000) | ||
|
|
||
| try do | ||
| {:ok, trx_state} = EctoLibSql.Native.begin(state) | ||
|
|
||
| {:ok, _query, _result, trx_state} = | ||
| EctoLibSql.handle_execute( | ||
| "INSERT INTO test_data (value) VALUES (?)", | ||
| ["trx_#{i}"], | ||
| [], | ||
| trx_state | ||
| ) | ||
|
|
||
| # Hold transaction | ||
| Process.sleep(50) | ||
|
|
||
| # Explicitly handle commit result to catch errors | ||
| case EctoLibSql.Native.commit(trx_state) do | ||
| {:ok, _committed_state} -> | ||
| {:ok, :committed} | ||
|
|
||
| {:error, reason} -> | ||
| {:error, {:commit_failed, reason}} | ||
| end | ||
| after | ||
| EctoLibSql.disconnect([], state) | ||
| end | ||
| end) | ||
| end) | ||
|
|
||
| results = Task.await_many(tasks, 30_000) | ||
|
|
||
| # All commits should succeed; fail test if any error occurred | ||
| Enum.each(results, fn result -> | ||
| case result do | ||
| {:ok, :committed} -> | ||
| :ok | ||
|
|
||
| {:error, {:commit_failed, reason}} -> | ||
| flunk("Transaction commit failed: #{inspect(reason)}") | ||
|
|
||
| other -> | ||
| flunk("Unexpected result from concurrent transaction: #{inspect(other)}") | ||
| end | ||
| end) | ||
|
|
||
| # Verify all inserts | ||
| {:ok, state} = EctoLibSql.connect(database: test_db, busy_timeout: 30_000) | ||
|
|
||
| {:ok, _query, result, _state} = | ||
| EctoLibSql.handle_execute("SELECT COUNT(*) FROM test_data", [], [], state) | ||
|
|
||
| EctoLibSql.disconnect([], state) | ||
|
|
||
| assert [[3]] = result.rows | ||
| end | ||
| end |
There was a problem hiding this comment.
Test names over-claim “timeout issues” vs what’s exercised.
Process.sleep/1 inside a transaction is fine, but it’s not really a long-running query/timeout scenario. Consider renaming to match what’s asserted, or extend the test to actually cover the timeout behaviour you care about.
🤖 Prompt for AI Agents
In @test/pool_load_test.exs around lines 316 - 405, The test names claim
“timeout issues” but the bodies only sleep inside transactions and assert
commits; either rename the two tests to reflect they check long-held
transactions/commit behavior (change "long transaction doesn't cause timeout
issues" to e.g. "long transaction doesn't prevent commit" and change "multiple
concurrent transactions complete despite duration" to e.g. "multiple concurrent
transactions complete despite holding"), or if you actually want to test timeout
behavior, modify the tests to set a small busy_timeout on the connection and
perform a DB-blocking operation that exceeds that timeout (then assert the
connection returns a timeout/error), updating the test bodies around
EctoLibSql.connect, EctoLibSql.Native.begin, and EctoLibSql.handle_execute
accordingly.
| describe "resource cleanup under load" do | ||
| @tag :slow | ||
| @tag :flaky | ||
| test "prepared statements cleaned up under concurrent load", %{test_db: test_db} do | ||
| tasks = | ||
| Enum.map(1..5, fn i -> | ||
| Task.async(fn -> | ||
| {:ok, state} = EctoLibSql.connect(database: test_db, busy_timeout: 30_000) | ||
|
|
||
| try do | ||
| {:ok, stmt} = | ||
| EctoLibSql.Native.prepare( | ||
| state, | ||
| "INSERT INTO test_data (value) VALUES (?)" | ||
| ) | ||
|
|
||
| try do | ||
| {:ok, _} = | ||
| EctoLibSql.Native.execute_stmt( | ||
| state, | ||
| stmt, | ||
| "INSERT INTO test_data (value) VALUES (?)", | ||
| ["prep_#{i}"] | ||
| ) | ||
|
|
||
| {:ok, :prepared_and_cleaned} | ||
| after | ||
| # Always close the prepared statement, ignore errors | ||
| try do | ||
| EctoLibSql.Native.close_stmt(stmt) | ||
| rescue | ||
| _ -> :ok | ||
| end | ||
| end | ||
| after | ||
| EctoLibSql.disconnect([], state) | ||
| end | ||
| end) | ||
| end) | ||
|
|
||
| results = Task.await_many(tasks, 30_000) | ||
|
|
||
| # Verify all prepared statement operations succeeded | ||
| Enum.each(results, fn result -> | ||
| case result do | ||
| {:ok, :prepared_and_cleaned} -> | ||
| :ok | ||
|
|
||
| {:error, reason} -> | ||
| flunk("Prepared statement operation failed: #{inspect(reason)}") | ||
|
|
||
| other -> | ||
| flunk("Unexpected result from prepared statement task: #{inspect(other)}") | ||
| end | ||
| end) | ||
|
|
||
| # Verify all inserts succeeded | ||
| {:ok, state} = EctoLibSql.connect(database: test_db, busy_timeout: 30_000) | ||
|
|
||
| {:ok, _query, result, _state} = | ||
| EctoLibSql.handle_execute("SELECT COUNT(*) FROM test_data", [], [], state) | ||
|
|
||
| EctoLibSql.disconnect([], state) | ||
|
|
||
| assert [[5]] = result.rows | ||
| end | ||
|
|
||
| @tag :slow | ||
| @tag :flaky | ||
| test "prepared statements with edge-case data cleaned up correctly", %{ | ||
| test_db: test_db | ||
| } do | ||
| tasks = | ||
| Enum.map(1..5, fn task_num -> | ||
| Task.async(fn -> | ||
| {:ok, state} = EctoLibSql.connect(database: test_db, busy_timeout: 30_000) | ||
|
|
||
| try do | ||
| {:ok, stmt} = | ||
| EctoLibSql.Native.prepare( | ||
| state, | ||
| "INSERT INTO test_data (value) VALUES (?)" | ||
| ) | ||
|
|
||
| try do | ||
| # Execute prepared statement with edge-case data | ||
| edge_values = generate_edge_case_values(task_num) | ||
|
|
||
| execute_results = | ||
| Enum.map(edge_values, fn value -> | ||
| EctoLibSql.Native.execute_stmt( | ||
| state, | ||
| stmt, | ||
| "INSERT INTO test_data (value) VALUES (?)", | ||
| [value] | ||
| ) | ||
| end) | ||
|
|
||
| # All executions should succeed | ||
| all_ok = Enum.all?(execute_results, fn r -> match?({:ok, _}, r) end) | ||
|
|
||
| if all_ok do | ||
| {:ok, :prepared_with_edge_cases} | ||
| else | ||
| {:error, :some_edge_case_inserts_failed} | ||
| end | ||
| after | ||
| # Always close the prepared statement, ignore errors | ||
| try do | ||
| EctoLibSql.Native.close_stmt(stmt) | ||
| rescue | ||
| _ -> :ok | ||
| end | ||
| end | ||
| after | ||
| EctoLibSql.disconnect([], state) | ||
| end | ||
| end) | ||
| end) | ||
|
|
||
| results = Task.await_many(tasks, 30_000) | ||
|
|
||
| # Verify all prepared statement operations succeeded | ||
| Enum.each(results, fn result -> | ||
| case result do | ||
| {:ok, :prepared_with_edge_cases} -> | ||
| :ok | ||
|
|
||
| {:error, reason} -> | ||
| flunk("Prepared statement with edge-case data failed: #{inspect(reason)}") | ||
|
|
||
| other -> | ||
| flunk("Unexpected result from prepared statement edge-case task: #{inspect(other)}") | ||
| end | ||
| end) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the definition of close_stmt
rg "close_stmt" --max-count 20 -B 2 -A 2Repository: ocean/ecto_libsql
Length of output: 15672
🏁 Script executed:
# Search for how close_stmt is used in other tests and code
rg "close_stmt" --type ex --type exs -B 1 -A 1Repository: ocean/ecto_libsql
Length of output: 87
🏁 Script executed:
# Find the close_stmt implementation in Elixir
grep -n "def close_stmt" lib/ecto_libsql/native.ex -A 5Repository: ocean/ecto_libsql
Length of output: 208
🏁 Script executed:
# Find the close_stmt implementation in Rust
rg "close_stmt\|close.*stmt" native/ecto_libsql/src/ -B 2 -A 10Repository: ocean/ecto_libsql
Length of output: 43
🏁 Script executed:
# Find the close function that close_stmt calls
grep -n "def close" lib/ecto_libsql/native.ex -A 3Repository: ocean/ecto_libsql
Length of output: 482
🏁 Script executed:
# Search for close function in Rust files
find native -name "*.rs" -exec grep -l "close" {} \;Repository: ocean/ecto_libsql
Length of output: 229
🏁 Script executed:
# Find the close function/NIF in statement.rs
cat native/ecto_libsql/src/statement.rs | grep -n "close" -B 5 -A 10Repository: ocean/ecto_libsql
Length of output: 760
🏁 Script executed:
# Search more specifically for the close function definition
grep -n "pub fn close" native/ecto_libsql/src/statement.rs -A 15Repository: ocean/ecto_libsql
Length of output: 43
🏁 Script executed:
# Also check lib.rs for close implementation
grep -n "pub fn close" native/ecto_libsql/src/lib.rs -A 15Repository: ocean/ecto_libsql
Length of output: 43
🏁 Script executed:
# Broader search for close in lib.rs and statement.rs
grep -n "fn close" native/ecto_libsql/src/lib.rs native/ecto_libsql/src/statement.rsRepository: ocean/ecto_libsql
Length of output: 43
🏁 Script executed:
# Check what NIF functions are exported/registered
rg "#\[rustler::nif" native/ecto_libsql/src/ -A 1 | grep -i close -B 1 -A 1Repository: ocean/ecto_libsql
Length of output: 230
🏁 Script executed:
# Get the full implementation of close function in connection.rs
grep -n "pub fn close" native/ecto_libsql/src/connection.rs -A 30Repository: ocean/ecto_libsql
Length of output: 1543
Reconsider rescue-all pattern in cleanup code—prefer asserting or logging close failures for test visibility.
The close_stmt/1 NIF can raise when a statement is not found in the registry. However, rescuing all errors in the after block may mask genuine resource-management issues. Since the statement should still exist after a successful prepare, consider either removing the rescue (let failures surface), explicitly asserting the return value (:ok = close_stmt(...)), or at least logging unexpected failures instead of silently swallowing them.
- Change concurrent mutex test from :skip to :flaky (it passes) - Standardize SQLite limitation tests with :sqlite_limitation tag - Add :sqlite_limitation to excluded tags in test_helper.exs - Create issue el-6r5 to document SQLite/PostgreSQL differences Tests affected: - error_handling_test.exs:178 - now :flaky (runs on CI exclude) - ecto_sql_transaction_compat_test.exs:218,228 - :sqlite_limitation - ecto_sql_compatibility_test.exs:86 - :sqlite_limitation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/cursor_streaming_large_test.exs (1)
784-799: Consider removing or documenting the alias functions.These four alias functions (
fetch_all_binary_rows,fetch_all_computed_rows,fetch_all_group_rows,fetch_all_distinct_rows) are identical wrappers aroundfetch_all_cursor_rows. Whilst the comment mentions "backwards compatibility and semantic clarity", they may add confusion. Consider either:
- Removing them and using
fetch_all_cursor_rowsdirectly- Adding @doc annotations explaining their purpose
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.beads/last-touchedtest/cursor_streaming_large_test.exstest/ecto_sql_compatibility_test.exstest/ecto_sql_transaction_compat_test.exstest/error_handling_test.exstest/pool_load_test.exstest/test_helper.exs
🚧 Files skipped from review as they are similar to previous changes (3)
- test/ecto_sql_transaction_compat_test.exs
- test/pool_load_test.exs
- .beads/last-touched
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/test_helper.exstest/cursor_streaming_large_test.exstest/error_handling_test.exstest/ecto_sql_compatibility_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/test_helper.exstest/cursor_streaming_large_test.exstest/error_handling_test.exstest/ecto_sql_compatibility_test.exs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
test/test_helper.exstest/cursor_streaming_large_test.exstest/error_handling_test.exstest/ecto_sql_compatibility_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/cursor_streaming_large_test.exstest/error_handling_test.exstest/ecto_sql_compatibility_test.exs
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
test/test_helper.exstest/cursor_streaming_large_test.exstest/error_handling_test.exstest/ecto_sql_compatibility_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/test_helper.exstest/cursor_streaming_large_test.exstest/error_handling_test.exstest/ecto_sql_compatibility_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*.exs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/test_helper.exstest/error_handling_test.exstest/ecto_sql_compatibility_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/turso_remote_test.exs : Turso remote tests should be skipped by default if credentials are missing and only run when explicitly needed due to cost implications
Applied to files:
test/test_helper.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use cursor streaming for large result sets to avoid loading entire datasets into memory
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/cursor_streaming_large_test.exstest/error_handling_test.exstest/ecto_sql_compatibility_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Use proper error handling patterns with case matching and with clauses to handle both success and error tuples from NIF operations
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.{ex,exs} : Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to **/*.rs : Use British/Australian English for all Rust code comments and documentation (except SQL keywords and compatibility requirements)
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
test/cursor_streaming_large_test.exs
📚 Learning: 2026-01-08T04:56:27.393Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.393Z
Learning: Applies to priv/repo/migrations/*.exs : Use STRICT tables in migrations for type safety when database schema stability is critical
Applied to files:
test/ecto_sql_compatibility_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto/adapters/libsql.ex : Implement type dumpers for converting Ecto types to SQLite values using `dumpers/2` function
Applied to files:
test/ecto_sql_compatibility_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto/adapters/libsql/connection.ex : Add SQLite function support by defining expression handlers for native SQLite functions
Applied to files:
test/ecto_sql_compatibility_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
Applied to files:
test/ecto_sql_compatibility_test.exs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (19)
test/error_handling_test.exs (2)
178-179: LGTM! Appropriate tag for concurrency test.Changing from
@tag :skipto@tag :flakyis sensible - this allows the test to run on CI whilst acknowledging its timing-sensitive nature. The test verifies mutex safety under concurrent stress, which is valuable but inherently prone to occasional timing issues.
223-223: LGTM! Clean consolidation of database cleanup.Using the centralised
EctoLibSql.TestHelpers.cleanup_db_files/1helper simplifies cleanup and ensures consistency with other tests in the suite.test/ecto_sql_compatibility_test.exs (2)
54-56: LGTM! Consistent cleanup pattern.The
on_exitcallback now uses the centralised cleanup helper, which handles all associated database files (WAL, SHM, journal, etc.) in one call.
86-88: LGTM! Well-documented limitation.Replacing
@tag :skipwith@tag :sqlite_limitationand adding an explanatory comment provides better documentation of why this test behaves differently. This makes it clear the limitation is inherent to SQLite rather than a bug to fix.test/test_helper.exs (4)
1-5: LGTM! Clear documentation of test exclusion categories.The comments provide good context for why different test categories exist and when they're excluded.
12-19: LGTM! Sensible test exclusion strategy.The exclusion logic appropriately differentiates between CI and local runs, ensuring CI skips flaky tests whilst still running more comprehensive tests than local development.
47-62: LGTM! Well-structured cleanup helper.The function correctly handles the common database auxiliary files. Ignoring
File.rmreturn values is appropriate here since files may not exist (e.g.,-walonly exists in WAL mode), and cleanup failures during tests are rarely critical.
75-85: LGTM! Comprehensive pattern-based cleanup.The two-step approach is sensible:
Path.wildcard(pattern)finds matching DB files, thencleanup_db_files/1handles each and its auxiliary files- The secondary
pattern <> "-*"catches orphaned auxiliary files that may exist without a main DB fileThis provides thorough cleanup for test databases with dynamic names.
test/cursor_streaming_large_test.exs (11)
9-36: LGTM! Clean test setup with proper cleanup.The setup correctly establishes an in-memory database, creates the test table, and ensures disconnection in
on_exit. Using:memory:avoids file cleanup concerns.
38-115: LGTM! Comprehensive streaming tests.Good coverage of cursor streaming fundamentals: varying dataset sizes, batch size enforcement, filtered queries, and ordering verification. The tests properly exercise the cursor lifecycle with
handle_declareandhandle_fetch.
116-161: LGTM! Thorough binary data handling test.The test verifies that BLOB data is correctly streamed through cursors, including preservation of binary size. The 1KB payload per row is a reasonable size to test binary handling without excessive memory usage.
255-320: LGTM! Good SQL feature coverage through cursors.These tests verify that complex SQL operations (GROUP BY, OFFSET/LIMIT, DISTINCT) work correctly when results are streamed through cursors. The assertions validate both row counts and data correctness.
323-374: LGTM! Proper error handling coverage.The tests verify that cursor operations return appropriate error tuples for invalid SQL, syntax errors, and non-existent tables. Empty result sets are also handled correctly. This aligns with the project's error handling guidelines.
376-493: LGTM! Excellent edge case coverage for NULL handling.These tests verify the important distinction between NULL and empty strings, which is a common source of bugs. The test at line 492 correctly expects SQLite's 1/0 representation for boolean IS NULL results.
496-562: LGTM! Transaction behaviour tests are well-structured.The rollback test correctly handles multiple possible outcomes (error, halt, or cont), acknowledging that cursor behaviour after rollback may vary by implementation. The uncommitted changes test verifies read-your-writes within a transaction.
564-678: LGTM! Cursor isolation tests are comprehensive.The tests verify that multiple cursors maintain independent state when interleaved, and that cursors are isolated from concurrent data modifications. Note that these tests run sequentially within the same process, testing logical isolation rather than true parallelism.
738-760: LGTM! Efficient list accumulation pattern.Good use of the accumulator pattern with
Enum.reverse() |> Enum.concat()to avoid O(n²) list concatenation. The comments clearly explain the performance rationale.
225-238: Batch count expectation is correct.The assertion for 11 batches is accurate. The cursor implementation returns
{:halt, result, state}when rows are empty (lib/ecto_libsql.ex line 434), and thecount_batchesfunction counts each fetch result—including the final empty batch—as one batch. With 1000 rows and batch size 100, this yields 10 full batches plus 1 final batch with 0 rows = 11 total.
684-710: No action needed. The code correctly uses the Native module's wrapper API.The
prepare/2function is intentionally designed to accept the State struct, not a raw connection ID. All wrapper functions in the Native module follow this pattern—they accept State as the first parameter, extract theconn_idvia pattern matching, and pass it to the underlying NIF functions. The test code at lines 686–690 correctly passesstatetoprepare/2, and the subsequent calls toexecute_stmt/4andclose_stmt/1are consistent with the established API design.Likely an incorrect or invalid review comment.
- Close setup connection immediately after schema creation - Consolidate duplicate insert_unicode_edge_case_value into insert_edge_case_value - Assert DELETE results instead of discarding them - Handle rollback results properly in constraint violation test
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @test/pool_load_test.exs:
- Around line 682-686: The blanket rescue around the call to
Native.close_stmt(stmt) masks unexpected errors; change it to catch only the
expected exception types (for example rescue e in [ArgumentError, RuntimeError]
-> :ok) or pattern-match on the specific exception(s) your Native.close_stmt/1
can raise, and re-raise or log any other exceptions instead of swallowing them;
ensure the code refers to Native.close_stmt/1 and the stmt variable so
unexpected failures surface for debugging.
🧹 Nitpick comments (1)
test/pool_load_test.exs (1)
815-819: State threading pattern is correct but assumes success.The
Enum.reduce/3pattern correctly threads the transaction state through multiple operations. However, it assumes all inserts succeed (the pattern match{:ok, ...}will raise aMatchErroron failure).This is acceptable for tests where failures should be immediately visible, but consider whether explicit error handling would make test failures clearer:
Optional: Explicit error handling
final_trx_state = Enum.reduce_while(edge_values, {:ok, trx_state}, fn value, {:ok, acc_state} -> case insert_edge_case_value(acc_state, value) do {:ok, _query, _result, new_state} -> {:cont, {:ok, new_state}} {:error, reason, _state} -> {:halt, {:error, reason}} end end) case final_trx_state do {:ok, state} -> # continue with commit {:error, reason} -> # handle error endHowever, the current approach (fail-fast with pattern match) is clearer for tests.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/pool_load_test.exs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ex,exs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ex,exs}: Use British/Australian English for all Elixir code comments and documentation (except SQL keywords and compatibility requirements)
Always format Elixir code before committing usingmix format --check-formattedUse camelCase for function names in Elixir (e.g.,
defmodule,def handle_execute)
Files:
test/pool_load_test.exs
test/**/*.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests may use
.unwrap()for simplicity when handling test data and setup
Files:
test/pool_load_test.exs
test/**/*test.exs
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Files:
test/pool_load_test.exs
**/*.{ex,exs,rs,md}
📄 CodeRabbit inference engine (CLAUDE.md)
When ending a work session, complete the mandatory workflow: file issues for remaining work, run quality gates (tests, linters, builds), update issue status, commit changes with
git commit, clean up stashes, and provide context for next session
Files:
test/pool_load_test.exs
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to test/**/*test.exs : Tests must cover happy path (successful operations), error cases (invalid IDs, missing resources, constraint violations), edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto/adapters/libsql.ex : Implement type loaders for converting SQLite values to Ecto types using `loaders/2` function
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Drop locks before async operations in Tokio runtime blocks to avoid holding locks across await points
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : All database connection errors and query failures must be handled gracefully with pattern matching on `{:ok, ...}` and `{:error, ...}` tuples - never use bare unwrap operations
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Validate transaction ownership by checking that `entry.conn_id == conn_id` before performing transaction operations
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/tests/**/*.rs : Tests may use `.unwrap()` for simplicity when handling test data and setup
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Handle Options using `.ok_or_else()` pattern with proper error messages instead of `.unwrap()`
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Use proper error handling patterns with case matching and with clauses to handle both success and error tuples from NIF operations
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to lib/ecto_libsql/native.ex : Provide NIF stubs and safe wrapper functions in the Native module for all Rust NIF functions
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/connection.rs : Lock a registry using `safe_lock(®ISTRY, "function_name context")?` instead of `.unwrap()` to prevent panics
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/*.rs : Lock Arc<Mutex<T>> using `safe_lock_arc(&client, "function_name client")?` instead of `.unwrap()` to prevent panics
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-06T06:36:01.585Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T06:36:01.585Z
Learning: Applies to native/ecto_libsql/src/**/*.rs : Add `#[rustler::nif]` attribute to NIF functions—modern Rustler auto-detects them without manual registration
Applied to files:
test/pool_load_test.exs
📚 Learning: 2026-01-08T04:56:27.394Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T04:56:27.394Z
Learning: Applies to **/*.ex : Use savepoints for partial rollback within transactions to enable error recovery patterns in batch operations
Applied to files:
test/pool_load_test.exs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
test/pool_load_test.exs (5)
1-41: Excellent test isolation and cleanup setup!The per-test database creation with unique naming and centralized cleanup using
EctoLibSql.TestHelpers.cleanup_db_files/1provides proper test isolation. The setup connection is correctly closed before tests run.
47-85: Well-structured edge case helpers!The helper functions provide comprehensive coverage of edge cases (NULL, empty strings, large values, Unicode variants) and promote code reuse throughout the test suite. The inline comments clearly document each edge case type.
95-95: Verify timeout configuration is appropriate.The
busy_timeout: 30_000(30 seconds) is used consistently throughout the tests. Whilst this helps avoid spurious failures under load, it may also mask genuine performance issues or deadlock scenarios.Consider:
- Whether this timeout is representative of production configuration
- If tests should also verify operations complete within a more aggressive timeout (e.g., 5 seconds) to catch performance regressions
- Adding a test variant with shorter timeouts to detect lock contention issues earlier
1-1388: Comprehensive test coverage aligning with requirements!This test suite excellently covers all required scenarios per the learnings:
- ✅ Happy path (successful operations)
- ✅ Error cases (syntax errors, constraint violations)
- ✅ Edge cases (NULL values, empty strings, large datasets, Unicode)
- ✅ Transaction rollback scenarios
- ✅ Type conversions (integers, floats, strings, timestamps)
- ✅ Concurrent operations (multiple connections, transactions)
The test structure with helper functions, proper resource cleanup, and consistent error handling patterns demonstrates good test design. The main improvements needed are addressing the flaky test behaviour and narrowing rescue clauses.
Based on learnings, this test suite meets the coverage requirements for test/**/*test.exs files.
88-90: The @flaky tag on these tests is intentional, not indicative of problematic code. Pertest_helper.exs, these tests are deliberately excluded from normal test runs ("excluded by default to avoid CI brittleness"), as they are load/stress tests designed for concurrent access pattern validation. The current implementation already includes proper task synchronisation viaTask.await_many, generous timeouts (30–60 seconds), and resource cleanup with try/after blocks. A recent commit ("fix: improve pool_load_test.exs robustness") indicates flakiness concerns have already been addressed. No changes required.
| try do | ||
| EctoLibSql.Native.close_stmt(stmt) | ||
| rescue | ||
| _ -> :ok | ||
| end |
There was a problem hiding this comment.
Narrow the rescue clause to avoid masking bugs.
The rescue clause catches all exceptions, which could hide critical bugs in Native.close_stmt/1 such as segfaults, type errors, or resource leaks. Whilst ensuring cleanup is important, overly broad error suppression can mask issues during development.
♻️ Suggested refinement
try do
EctoLibSql.Native.close_stmt(stmt)
rescue
- _ -> :ok
+ error in [RuntimeError, ArgumentError] ->
+ # Expected errors during cleanup - safe to ignore
+ :ok
endAlternatively, if all errors are genuinely expected during test cleanup, add a comment explaining why broad error suppression is necessary.
📝 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.
| try do | |
| EctoLibSql.Native.close_stmt(stmt) | |
| rescue | |
| _ -> :ok | |
| end | |
| try do | |
| EctoLibSql.Native.close_stmt(stmt) | |
| rescue | |
| error in [RuntimeError, ArgumentError] -> | |
| # Expected errors during cleanup - safe to ignore | |
| :ok | |
| end |
| try do | ||
| EctoLibSql.Native.close_stmt(stmt) | ||
| rescue | ||
| _ -> :ok | ||
| end |
There was a problem hiding this comment.
Narrow the rescue clause to avoid masking bugs.
Same issue as lines 602-606: the rescue clause catches all exceptions, potentially hiding critical bugs in Native.close_stmt/1. Consider catching only specific expected exception types.
🤖 Prompt for AI Agents
In @test/pool_load_test.exs around lines 682 - 686, The blanket rescue around
the call to Native.close_stmt(stmt) masks unexpected errors; change it to catch
only the expected exception types (for example rescue e in [ArgumentError,
RuntimeError] -> :ok) or pattern-match on the specific exception(s) your
Native.close_stmt/1 can raise, and re-raise or log any other exceptions instead
of swallowing them; ensure the code refers to Native.close_stmt/1 and the stmt
variable so unexpected failures surface for debugging.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.