Use pytest in SQLLogic Python test runner #16685
Merged
Conversation
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
Integrate with pytest to be able to use a fully-fledged test runner. Adds automatic discovery of .test files. Adds a few command line options inspired by Catch2.
Tishj
reviewed
Mar 17, 2025
|
I think we do want to replace the existing method of calling the python sqllogictest with |
Tishj
reviewed
Mar 17, 2025
Tishj
reviewed
Mar 17, 2025
| import typing | ||
| import warnings | ||
| from .skipped_tests import SKIPPED_TESTS | ||
|
|
There was a problem hiding this comment.
As evidenced by the failing python CI run, this should not live inside tools/pythonpkg/tests, as that is already a pytest directory, it should probably live inside tools/pythonpkg/sqllogic
There was a problem hiding this comment.
I have moved the files to tools/pythonpkg/sqllogic.
This resulted in an import error at first:
ImportError while importing test module '/Users/me/duckdb/tools/pythonpkg/sqllogic/test_sqllogic.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/homebrew/Cellar/python@3.13/3.13.2/Frameworks/Python.framework/Versions/3.13/lib/python3.13/importlib/__init__.py:88: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
tools/pythonpkg/sqllogic/test_sqllogic.py:18: in <module>
from sqllogictest.result import (
scripts/sqllogictest/result.py:33: in <module>
from .logger import SQLLogicTestLogger
scripts/sqllogictest/logger.py:4: in <module>
from duckdb import tokenize, token_type
tools/pythonpkg/duckdb/__init__.py:14: in <module>
from .duckdb import (
E ImportError: cannot import name 'SQLExpression' from 'duckdb.duckdb' (/Users/me/duckdb/tools/pythonpkg/duckdb/duckdb.cpython-313-darwin.so)
The reason is that tools/pythonpkg gets appended to sys.path when pytest starts so that pytest can then import the sqllogic package. (I think the sqllogic directory has to be a package to be able to import skipped_tests.py in conftest.py.)
I worked around this by changing the import mode to importlib which doesn't change sys.path. Doing this in conftest.py is already to late because then the changes to the path have already happened. Therefore, I added a pytest.ini file in 2f38099 which adds the --import-mode=importlib option when pytest is started.
There was a problem hiding this comment.
Awesome, thanks for the investigation and the writeup, that will help with maintaining 👍
…s captured, & formatting
Tishj
reviewed
Mar 18, 2025
Tishj
reviewed
Mar 18, 2025
| run: | | ||
| python3 tools/pythonpkg/scripts/sqllogictest_python.py |
There was a problem hiding this comment.
Also remove sqllogictest_python.py please, add FIXME's to the test_sqllogic.py where functionality is perhaps missing that is in sqllogictest_python.py, I think the regressions should be very minimal (?)
There was a problem hiding this comment.
Done. As far as I can see, the pytest runner covers almost all the functionality of the old one. The only thing that is missing is an equivalent to --file-list which accepts a file with a list of test cases to run.
--file-pathis replaced by passing the file path with-kor the name of the test case--start-offsetand--build-dirare available- Error reporting and skipping is handled by pytest
TEST_DIRECTORY_PATHis handled by pytest (including cleaning up the old ones)
SQLLogicTestExecutor hasn't been changed much thus far and works as before.
GitHub Actions runners set this automatically
With import mode prepend, the tools/pythonpkg directory is added to the sys.path.
This leads to import errors when importing the duckdb package because there is
also a directory named tools/pythonpkg/duckdb.
Hence, setting the import mode to 'importlib' ('append' would also work).
|
Made some changes to fix some issues diff --git a/scripts/sqllogictest/result.py b/scripts/sqllogictest/result.py
index e639f0baf8..1c74e82c97 100644
--- a/scripts/sqllogictest/result.py
+++ b/scripts/sqllogictest/result.py
@@ -99,7 +99,7 @@ class TestException(Exception):
__slots__ = ['data', 'message', 'result']
def __init__(self, data: SQLLogicStatementData, message: str, result: ExecuteResult):
- self.message = f'{str(data)} {message}'
+ self.message = message
super().__init__(self.message)
self.data = data
self.result = result
diff --git a/tools/pythonpkg/sqllogic/conftest.py b/tools/pythonpkg/sqllogic/conftest.py
index 498a020132..6520cd57f1 100644
--- a/tools/pythonpkg/sqllogic/conftest.py
+++ b/tools/pythonpkg/sqllogic/conftest.py
@@ -40,10 +40,16 @@ def pytest_addoption(parser: pytest.Parser):
parser.addoption("--rng-seed", type=int, dest="rng_seed", help="Random integer seed")
-@pytest.hookimpl(tryfirst=True)
+@pytest.hookimpl(hookwrapper=True)
def pytest_keyboard_interrupt(excinfo: pytest.ExceptionInfo):
- # TODO: CTRL+C does not immediately interrupt pytest. You sometimes have to press it multiple times.
- pytestmark = pytest.mark.skip(reason="Keyboard interrupt")
+ # Ensure all tests are properly cleaned up on keyboard interrupt
+ from .test_sqllogic import test_sqllogic
+ if hasattr(test_sqllogic, 'executor') and test_sqllogic.executor:
+ if test_sqllogic.executor.database and hasattr(test_sqllogic.executor.database, 'connection'):
+ test_sqllogic.executor.database.connection.interrupt()
+ test_sqllogic.executor.cleanup()
+ test_sqllogic.executor = None
+ yield
def pytest_configure(config: pytest.Config):
@@ -165,7 +171,7 @@ def pytest_collection_modifyitems(session: pytest.Session, config: pytest.Config
if start_offset < 0:
raise ValueError("--start-offset must be a non-negative integer")
elif end_offset < start_offset:
- raise ValueError("--end-offset must be greater than or equal to --start-offset")
+ raise ValueError(f"--end-offset ({end_offset}) must be greater than or equal to --start-offset")
max_end_offset = len(items) - 1
if end_offset > max_end_offset:
diff --git a/tools/pythonpkg/sqllogic/test_sqllogic.py b/tools/pythonpkg/sqllogic/test_sqllogic.py
index 3ad8ca58a4..55bfb8d4d7 100644
--- a/tools/pythonpkg/sqllogic/test_sqllogic.py
+++ b/tools/pythonpkg/sqllogic/test_sqllogic.py
@@ -4,6 +4,7 @@ import pathlib
import pytest
import sys
from typing import Any, Generator, Optional
+import signal
##### Copied from sqllogictest_python.py #####
@@ -23,8 +24,21 @@ from sqllogictest.result import (
ExecuteResult,
)
+def sigquit_handler(signum, frame):
+ # Access the executor from the test_sqllogic function
+ if hasattr(test_sqllogic, 'executor') and test_sqllogic.executor:
+ if test_sqllogic.executor.database and hasattr(test_sqllogic.executor.database, 'connection'):
+ test_sqllogic.executor.database.connection.interrupt()
+ test_sqllogic.executor.cleanup()
+ test_sqllogic.executor = None
+ # Re-raise the signal to let the default handler take over
+ signal.signal(signal.SIGQUIT, signal.default_int_handler)
+ os.kill(os.getpid(), signal.SIGQUIT)
+
+
+# Register the SIGQUIT handler
+signal.signal(signal.SIGQUIT, sigquit_handler)
-# This is pretty much just a VM
class SQLLogicTestExecutor(SQLLogicRunner):
def __init__(self, test_directory: str, build_directory: Optional[str] = None):
super().__init__(build_directory)
@@ -67,45 +81,64 @@ class SQLLogicTestExecutor(SQLLogicRunner):
test_delete_file(path + ".wal")
def execute_test(self, test: SQLLogicTest) -> ExecuteResult:
- self.reset()
- self.test = test
- self.original_sqlite_test = self.test.is_sqlite_test()
+ try:
+ self.reset()
+ self.test = test
+ self.original_sqlite_test = self.test.is_sqlite_test()
- # Top level keywords
- keywords = {'__TEST_DIR__': self.get_test_directory(), '__WORKING_DIRECTORY__': os.getcwd()}
+ # Top level keywords
+ keywords = {'__TEST_DIR__': self.get_test_directory(), '__WORKING_DIRECTORY__': os.getcwd()}
- def update_value(_: SQLLogicContext) -> Generator[Any, Any, Any]:
- # Yield once to represent one iteration, do not touch the keywords
- yield None
+ def update_value(_: SQLLogicContext) -> Generator[Any, Any, Any]:
+ # Yield once to represent one iteration, do not touch the keywords
+ yield None
- self.database = SQLLogicDatabase(':memory:', None)
- pool = self.database.connect()
- context = SQLLogicContext(pool, self, test.statements, keywords, update_value)
- pool.initialize_connection(context, pool.get_connection())
- # The outer context is not a loop!
- context.is_loop = False
+ self.database = SQLLogicDatabase(':memory:', None)
+ pool = self.database.connect()
+ context = SQLLogicContext(pool, self, test.statements, keywords, update_value)
+ pool.initialize_connection(context, pool.get_connection())
+ # The outer context is not a loop!
+ context.is_loop = False
- try:
- context.verify_statements()
- res = context.execute()
- except TestException as e:
- res = e.handle_result()
- if res.type == ExecuteResult.Type.SKIPPED:
- pytest.skip(str(e.message))
- else:
- pytest.fail(str(e.message), pytrace=False)
-
- self.database.reset()
-
- # Clean up any databases that we created
+ try:
+ context.verify_statements()
+ res = context.execute()
+ except TestException as e:
+ res = e.handle_result()
+ if res.type == ExecuteResult.Type.SKIPPED:
+ pytest.skip(str(e.message))
+ else:
+ pytest.fail(str(e.message), pytrace=False)
+
+ self.database.reset()
+
+ # Clean up any databases that we created
+ for loaded_path in self.loaded_databases:
+ if not loaded_path:
+ continue
+ # Only delete database files that were created during the tests
+ if not loaded_path.startswith(self.get_test_directory()):
+ continue
+ os.remove(loaded_path)
+ return res
+ except KeyboardInterrupt:
+ if self.database:
+ self.database.interrupt()
+ raise
+
+ def cleanup(self):
+ if self.database:
+ if hasattr(self.database, 'connection'):
+ self.database.connection.interrupt()
+ self.database.reset()
+ self.database = None
+ # Clean up any remaining test databases
for loaded_path in self.loaded_databases:
- if not loaded_path:
- continue
- # Only delete database files that were created during the tests
- if not loaded_path.startswith(self.get_test_directory()):
- continue
- os.remove(loaded_path)
- return res
+ if loaded_path and loaded_path.startswith(self.get_test_directory()):
+ try:
+ os.remove(loaded_path)
+ except FileNotFoundError:
+ pass
def test_sqllogic(test_script_path: pathlib.Path, pytestconfig: pytest.Config, tmp_path: pathlib.Path):
@@ -119,8 +152,14 @@ def test_sqllogic(test_script_path: pathlib.Path, pytestconfig: pytest.Config, t
build_dir = pytestconfig.getoption("build_dir")
executor = SQLLogicTestExecutor(str(tmp_path), build_dir)
- result = executor.execute_test(test)
- assert result.type == ExecuteResult.Type.SUCCESS
+ # Store executor in the function's arguments so it can be accessed by the interrupt handler
+ test_sqllogic.executor = executor
+ try:
+ result = executor.execute_test(test)
+ assert result.type == ExecuteResult.Type.SUCCESS
+ finally:
+ executor.cleanup()
+ test_sqllogic.executor = None
if __name__ == '__main__':
|
|
Here is a run of the "Python SQLLogicTest Library" job in my fork: https://github.com/Flogex/duckdb/actions/runs/13967583581/job/39101440384?pr=7 |
When using nargs=* and action=extend and a default value, then the default value is always prepended to the list (of directory paths, in this case)
|
@Tishj Here is the latest nightly test run: https://github.com/Flogex/duckdb/actions/runs/14022703354/job/39256726461 Three test failures
|
|
Yea agreed, I think the |
I excluded the three failing tests in 133ac9a |
…-pytes Last nightlyt
Tishj
approved these changes
Mar 27, 2025
|
Thanks! |
krlmlr
added a commit
to duckdb/duckdb-r
that referenced
this pull request
May 18, 2025
On COPY TO/FROM check the format during binding. (duckdb/duckdb#17381) Use pytest in SQLLogic Python test runner (duckdb/duckdb#16685) Fix JSON extension compilation on Ubuntu 22.04 (duckdb/duckdb#17434)
krlmlr
added a commit
to duckdb/duckdb-r
that referenced
this pull request
May 18, 2025
On COPY TO/FROM check the format during binding. (duckdb/duckdb#17381) Use pytest in SQLLogic Python test runner (duckdb/duckdb#16685) Fix JSON extension compilation on Ubuntu 22.04 (duckdb/duckdb#17434)
krlmlr
added a commit
to duckdb/duckdb-r
that referenced
this pull request
May 19, 2025
On COPY TO/FROM check the format during binding. (duckdb/duckdb#17381) Use pytest in SQLLogic Python test runner (duckdb/duckdb#16685) Fix JSON extension compilation on Ubuntu 22.04 (duckdb/duckdb#17434)
krlmlr
added a commit
to duckdb/duckdb-r
that referenced
this pull request
May 19, 2025
On COPY TO/FROM check the format during binding. (duckdb/duckdb#17381) Use pytest in SQLLogic Python test runner (duckdb/duckdb#16685) Fix JSON extension compilation on Ubuntu 22.04 (duckdb/duckdb#17434)
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 PR adds pytest as test runner for the existing SQLLogic Python tests. .test, .test_slow and .test_coverage files in a given test directory are are automatically discovered and added as test cases. The test runner also accepts a few options known from Catch2, like
--start-offset,--end-offsetor--order.Usage:
pytest 'tools/pythonpkg/tests/sqllogic/' --test-dir ./testpytest 'tools/pythonpkg/tests/sqllogic/' -m all --test-dir ./testpytest 'tools/pythonpkg/tests/sqllogic/test_sqllogic.py::test_sqllogic[test/sql/order/test_limit_parameter.test]' --test-dir ./testpytest 'tools/pythonpkg/tests/sqllogic/' -m slow --capture=no --verbose --test-dir ./testVerbose output:
Non-verbose output:
This PR leaves the current test runner untouched and does not use the test runner yet in CI. For now, I simply copied over the code for the
SQLLogicTestExecutorfrom the old test runner implementation and replaced the skip/fail methods with their pytest equivalents.