test: clean up DuckDB tables leaked by test_sql_error_handling#9352
Closed
akshayka wants to merge 1 commit into
Closed
test: clean up DuckDB tables leaked by test_sql_error_handling#9352akshayka wants to merge 1 commit into
akshayka wants to merge 1 commit into
Conversation
These tests create tables on DuckDB's default in-memory connection (via `marimo._sql.sql.sql()` without an engine, and bare `duckdb.sql(...)`), which is a module-level singleton shared with any other test that reads from `connection=None`. Left-behind tables leak across the pytest-xdist worker, causing unrelated tests to see extra tables in the `main` schema. Seen on CI as a flaky failure of `tests/_data/test_get_datasets.py::test_get_databases` and `::test_get_databases_with_no_tables` when xdist scheduled the error handling tests onto the same worker first. An autouse fixture that drops the known set of created tables after each test eliminates the leak.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes flaky Windows CI failures caused by DuckDB tables leaking between tests when using DuckDB’s process-wide default in-memory connection.
Changes:
- Add a module-level autouse pytest fixture to drop a known set of DuckDB tables after each test.
- Document why these tables leak across tests (default DuckDB connection shared by
sql()withoutengine=andduckdb.sql(...)).
Comment on lines
+44
to
+45
| for table in _LEAKED_DUCKDB_TABLES: | ||
| duckdb.sql(f"DROP TABLE IF EXISTS {table}") |
There was a problem hiding this comment.
Use duckdb.execute(...) (or otherwise force execution) for the DROP statements instead of duckdb.sql(...). In this repo, duckdb.execute is used for DDL/cleanup on the default connection (e.g., tests/_data/test_get_datasets.py:398,413 and tests/_data/test_preview_column.py:50), and it makes the teardown intent clearer and can batch multiple DROP statements in one call.
Suggested change
| for table in _LEAKED_DUCKDB_TABLES: | |
| duckdb.sql(f"DROP TABLE IF EXISTS {table}") | |
| drop_statements = "; ".join( | |
| f"DROP TABLE IF EXISTS {table}" for table in _LEAKED_DUCKDB_TABLES | |
| ) | |
| duckdb.execute(drop_statements) |
Comment on lines
+37
to
+46
| @pytest.fixture(autouse=True) | ||
| def _cleanup_duckdb_default_connection(): | ||
| yield | ||
| if not HAS_DUCKDB: | ||
| return | ||
| import duckdb | ||
|
|
||
| for table in _LEAKED_DUCKDB_TABLES: | ||
| duckdb.sql(f"DROP TABLE IF EXISTS {table}") | ||
|
|
There was a problem hiding this comment.
Consider adding a small regression check to ensure this autouse teardown actually prevents cross-test leakage on the default DuckDB connection. For example, one test can create one of the tables in _LEAKED_DUCKDB_TABLES via duckdb.sql(...), and a following test can assert that table is absent (e.g., via SHOW TABLES)—the fixture should have dropped it between tests. This helps ensure the flake fix keeps working as tests evolve.
6 tasks
Light2Dark
added a commit
that referenced
this pull request
Apr 24, 2026
## 📝 Summary <!-- If this PR closes any issues, list them here by number (e.g., Closes #123). Detail the specific changes made in this pull request. Explain the problem addressed and how it was resolved. If applicable, provide before and after comparisons, screenshots, or any relevant details to help reviewers understand the changes easily. --> Supersedes #9352 by ensuring each test runs in a duckdb connection that closes automatically. ## 📋 Pre-Review Checklist <!-- These checks need to be completed before a PR is reviewed --> - [x] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (Please provide a link if applicable). - [x] Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it. - [ ] Video or media evidence is provided for any visual changes (optional). <!-- PR is more likely to be merged if evidence is provided for changes made --> ## ✅ Merge Checklist - [x] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [ ] Documentation has been updated where applicable, including docstrings for API changes. - [ ] Tests have been added for the changes made. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Use a fresh in-memory `duckdb` connection per test to isolate state and prevent flaky SQL error-handling tests. Tests now pass `engine=duckdb_conn` and call `duckdb_conn.sql(...)`, so connections close automatically after each test. - **Bug Fixes** - Add `pytest` fixture `duckdb_conn` that creates `duckdb.connect(':memory:')` and closes it after each test. - Replace implicit engine/global `duckdb.sql(...)` with explicit `engine=duckdb_conn` and `duckdb_conn.sql(...)`. - Stabilizes tests for missing tables/columns, syntax errors, hints, and long-statement truncation. <sup>Written for commit 359e017. Summary will update on new commits.</sup> <!-- End of auto-generated description by cubic. -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request was authored by a coding agent.
Summary
Fixes flaky Windows CI failures in
tests/_data/test_get_datasets.py::test_get_databasesand::test_get_databases_with_no_tables.Root cause
marimo._sql.sql.sql()called without anengine=argument (and bareduckdb.sql(...)) both target DuckDB's process-wide default in-memory connection.get_databases_from_duckdb(connection=None)reads from that same singleton. Tests intests/_sql/test_sql_error_handling.pycreate tables (test_error_table,test_type_table,test_hints_table,test_columns,hint_test_table,hint_multiline_table) withCREATE OR REPLACE TABLEand never drop them. Whenever pytest-xdist happens to schedule them beforetest_get_databases*on the same worker, the leaked tables appear in themainschema and the assertion fails.CI evidence (run 24859690600):
test_get_databasesat ~94% — FAILED.test_get_databasesat ~11% (before any error-handling tests landed on that worker) — PASSED.Reproduced locally by running
tests/_sql/test_sql_error_handling.pyfollowed by the two target tests in the same process:Fix
Add a single module-level autouse fixture that drops the known set of leaked tables after each test. Scoped narrowly to this file since it owns the pollution.
Summary by cubic
Clean up DuckDB tables created by error-handling tests to stop cross-test leakage on the default in-memory connection. This removes flaky CI failures in database discovery tests.
pytestfixture intests/_sql/test_sql_error_handling.pyto drop known tables after each test on the defaultduckdbconnection.mainschema whenpytest-xdistreuses a worker.test_error_table,test_type_table,test_hints_table,test_columns,hint_test_table,hint_multiline_table.Written for commit a540a0b. Summary will update on new commits.