From 5e25b9657cc030ddf6334c26930841adb6439e44 Mon Sep 17 00:00:00 2001 From: Jorge Alvarez Jarreta Date: Wed, 3 Jul 2024 17:59:01 +0100 Subject: [PATCH 01/10] engine.execute() has been removed in SQLAlchemy 2.0 --- src/ensembl/utils/database/dbconnection.py | 19 ------------- tests/database/test_dbconnection.py | 31 ++-------------------- 2 files changed, 2 insertions(+), 48 deletions(-) diff --git a/src/ensembl/utils/database/dbconnection.py b/src/ensembl/utils/database/dbconnection.py index 931f4c3..2886d2e 100644 --- a/src/ensembl/utils/database/dbconnection.py +++ b/src/ensembl/utils/database/dbconnection.py @@ -152,25 +152,6 @@ def dispose(self) -> None: """Disposes of the connection pool.""" self._engine.dispose() - def execute(self, statement: Query, parameters=None, execution_options=None) -> sqlalchemy.engine.Result: - """Executes the given SQL query and returns its result. - - See `sqlalchemy.engine.Connection.execute()` method for more information about the - additional arguments. - - Args: - statement: SQL query to execute. - parameters: Parameters which will be bound into the statement. - execution_options: Optional dictionary of execution options, which will be associated - with the statement execution. - - """ - if isinstance(statement, str): - statement = text(statement) # type: ignore[assignment] - return self.connect().execute( - statement=statement, parameters=parameters, execution_options=execution_options - ) # type: ignore[call-overload] - def _enable_sqlite_savepoints(self, engine: sqlalchemy.engine.Engine) -> None: """Enables SQLite SAVEPOINTS to allow session rollbacks.""" diff --git a/tests/database/test_dbconnection.py b/tests/database/test_dbconnection.py index 2c4e392..bc251a4 100644 --- a/tests/database/test_dbconnection.py +++ b/tests/database/test_dbconnection.py @@ -157,34 +157,7 @@ def test_dispose(self) -> None: num_conn = self.dbc._engine.pool.checkedin() # pylint: disable=protected-access assert num_conn == 0, "A new pool should have 0 checked-in connections" - @pytest.mark.dependency(name="test_exec", depends=["test_init"], scope="class") - @pytest.mark.parametrize( - "query, nrows, expectation", - [ - param("SELECT * FROM gibberish", 6, does_not_raise(), id="Valid string query"), - param(text("SELECT * FROM gibberish"), 6, does_not_raise(), id="Valid text query"), - param( - "SELECT * FROM my_table", - 0, - raises(SQLAlchemyError, match=r"(my_table.* doesn't exist|no such table: my_table)"), - id="Querying an unexistent table", - ), - ], - ) - def test_execute(self, query: Query, nrows: int, expectation: ContextManager) -> None: - """Tests `DBConnection.execute()` method. - - Args: - query: SQL query. - nrows: Number of rows expected to be returned from the query. - expectation: Context manager for the expected exception. - - """ - with expectation: - result = self.dbc.execute(query) - assert len(result.fetchall()) == nrows, "Unexpected number of rows returned" - - @pytest.mark.dependency(depends=["test_init", "test_connect", "test_exec"], scope="class") + @pytest.mark.dependency(depends=["test_init", "test_connect"], scope="class") @pytest.mark.parametrize( "identifier, rows_to_add, before, after", [ @@ -232,7 +205,7 @@ def test_session_scope( results = session.execute(query) assert len(results.fetchall()) == after - @pytest.mark.dependency(depends=["test_init", "test_connect", "test_exec"], scope="class") + @pytest.mark.dependency(depends=["test_init", "test_connect"], scope="class") def test_test_session_scope(self) -> None: """Tests `DBConnection.test_session_scope()` method.""" # Session requires mapped classes to interact with the database From 669282bee797fc23e84e1ee46da8be539453caf7 Mon Sep 17 00:00:00 2001 From: Jorge Alvarez Jarreta Date: Wed, 3 Jul 2024 17:59:39 +0100 Subject: [PATCH 02/10] ignore cache folder for ruff --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index c28bf46..c659004 100644 --- a/.gitignore +++ b/.gitignore @@ -137,3 +137,6 @@ dmypy.json # Cython debug symbols cython_debug/ + +# Ruff cache +.ruff_cache/ From 7ad25e92c281ad7146dc69d080c9674bdfba48bf Mon Sep 17 00:00:00 2001 From: Jorge Alvarez Jarreta Date: Wed, 3 Jul 2024 18:00:58 +0100 Subject: [PATCH 03/10] update library's version --- src/ensembl/utils/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ensembl/utils/__init__.py b/src/ensembl/utils/__init__.py index 5eea5e8..f090386 100644 --- a/src/ensembl/utils/__init__.py +++ b/src/ensembl/utils/__init__.py @@ -14,7 +14,7 @@ # limitations under the License. """Ensembl Python general-purpose utils library.""" -__version__ = "0.3.0" +__version__ = "0.4.0" __all__ = [ "StrPath", From 74a2c3ba60c384fd741c787a0777e5edb937bfac Mon Sep 17 00:00:00 2001 From: Jorge Alvarez Jarreta Date: Wed, 3 Jul 2024 18:18:43 +0100 Subject: [PATCH 04/10] remove unused imports --- src/ensembl/utils/database/dbconnection.py | 2 +- tests/database/test_dbconnection.py | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/ensembl/utils/database/dbconnection.py b/src/ensembl/utils/database/dbconnection.py index 2886d2e..1b141b4 100644 --- a/src/ensembl/utils/database/dbconnection.py +++ b/src/ensembl/utils/database/dbconnection.py @@ -41,7 +41,7 @@ from typing import ContextManager, Generator, Optional, TypeVar import sqlalchemy -from sqlalchemy import create_engine, event, text +from sqlalchemy import create_engine, event from sqlalchemy.orm import sessionmaker from sqlalchemy.schema import MetaData, Table diff --git a/tests/database/test_dbconnection.py b/tests/database/test_dbconnection.py index bc251a4..5b0b900 100644 --- a/tests/database/test_dbconnection.py +++ b/tests/database/test_dbconnection.py @@ -14,21 +14,19 @@ # limitations under the License. """Unit testing of `ensembl.utils.database.dbconnection` module.""" -from contextlib import nullcontext as does_not_raise import os from pathlib import Path -from typing import ContextManager import pytest -from pytest import FixtureRequest, param, raises +from pytest import FixtureRequest, param from sqlalchemy import text, VARCHAR from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column from sqlalchemy.engine.url import make_url -from sqlalchemy.exc import IntegrityError, SQLAlchemyError +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.automap import automap_base from sqlalchemy_utils import create_database, database_exists, drop_database -from ensembl.utils.database import DBConnection, Query, UnitTestDB +from ensembl.utils.database import DBConnection, UnitTestDB class MockBase(DeclarativeBase): From 16a39050ca655095c8a229ff7f35758a4350b555 Mon Sep 17 00:00:00 2001 From: Matthieu Barba Date: Wed, 10 Jul 2024 16:46:46 +0100 Subject: [PATCH 05/10] Allow src_path to not exists Then just create an empty test database without schema and data. --- src/ensembl/utils/plugin.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ensembl/utils/plugin.py b/src/ensembl/utils/plugin.py index fab37c9..a6e4e8d 100644 --- a/src/ensembl/utils/plugin.py +++ b/src/ensembl/utils/plugin.py @@ -129,7 +129,7 @@ def _db_factory(src: StrPath, name: Optional[str] = None) -> UnitTestDB: """Returns a unit test database. Args: - src: Directory path where the test database schema and content files are located. + src: Directory path where the test database schema and content files are located, if any. name: Name to give to the new database. See `UnitTestDB` for more information. """ @@ -137,6 +137,8 @@ def _db_factory(src: StrPath, name: Optional[str] = None) -> UnitTestDB: if not src_path.is_absolute(): src_path = data_dir / src_path db_key = name if name else src_path.name + if not src_path.exists(): + src_path = None return created.setdefault(db_key, UnitTestDB(server_url, dump_dir=src_path, name=name)) yield _db_factory From 780174813dddcfd4e443690c513369ff25d8cdf1 Mon Sep 17 00:00:00 2001 From: Matthieu Barba Date: Thu, 11 Jul 2024 15:44:58 +0100 Subject: [PATCH 06/10] Fix dump_dir type --- src/ensembl/utils/plugin.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/ensembl/utils/plugin.py b/src/ensembl/utils/plugin.py index a6e4e8d..7a19583 100644 --- a/src/ensembl/utils/plugin.py +++ b/src/ensembl/utils/plugin.py @@ -137,9 +137,8 @@ def _db_factory(src: StrPath, name: Optional[str] = None) -> UnitTestDB: if not src_path.is_absolute(): src_path = data_dir / src_path db_key = name if name else src_path.name - if not src_path.exists(): - src_path = None - return created.setdefault(db_key, UnitTestDB(server_url, dump_dir=src_path, name=name)) + dump_dir: Path | None = src_path if src_path.exists() else None + return created.setdefault(db_key, UnitTestDB(server_url, dump_dir=dump_dir, name=name)) yield _db_factory # Drop all unit test databases unless the user has requested to keep them From 32efd8a8d2369300f89a0f9c34ac59277653df99 Mon Sep 17 00:00:00 2001 From: Matthieu Barba Date: Thu, 11 Jul 2024 17:28:25 +0100 Subject: [PATCH 07/10] Properly test paths, need actual unitestdb --- tests/plugin/test_plugin.py | 34 +++++++++++-------- .../plugin/test_plugin/dump_dir/gibberish.txt | 6 ++++ tests/plugin/test_plugin/dump_dir/table.sql | 7 ++++ 3 files changed, 33 insertions(+), 14 deletions(-) create mode 100644 tests/plugin/test_plugin/dump_dir/gibberish.txt create mode 100644 tests/plugin/test_plugin/dump_dir/table.sql diff --git a/tests/plugin/test_plugin.py b/tests/plugin/test_plugin.py index 8e4d71e..cc87524 100644 --- a/tests/plugin/test_plugin.py +++ b/tests/plugin/test_plugin.py @@ -21,13 +21,12 @@ from dataclasses import dataclass from pathlib import Path from typing import Callable, ContextManager -from unittest.mock import patch import pytest from pytest import FixtureRequest, param, raises from ensembl.utils import StrPath -from ensembl.utils.database import StrURL +from ensembl.utils.database import StrURL, UnitTestDB @dataclass @@ -80,14 +79,22 @@ def test_assert_files( @pytest.mark.parametrize( - "dump_dir, db_name", + "dump_dir, make_absolute, db_name, expected_tables", [ - (Path("dump_dir"), "dump_dir"), - (Path("dump_dir").resolve(), "test_db"), + param(Path("dump_dir"), False, "relative_dump_db", ["gibberish"], id="Relative dump_dir"), + param(Path("dump_dir"), True, "absolute_dump_db", ["gibberish"], id="Absolute dump_dir"), + param(Path("not_a_dir"), False, "no_dump_db", [], id="Non-existent dump_dir"), ], ) -@patch("ensembl.utils.plugin.UnitTestDB", new=MockTestDB) -def test_db_factory(request: FixtureRequest, db_factory: Callable, dump_dir: Path, db_name: str) -> None: +def test_db_factory( + request: FixtureRequest, + db_factory: Callable[[Path, str], UnitTestDB], + data_dir: Path, + dump_dir: Path, + make_absolute: bool, + db_name: str, + expected_tables: list[str], +) -> None: """Tests the `db_factory` fixture. Args: @@ -97,10 +104,9 @@ def test_db_factory(request: FixtureRequest, db_factory: Callable, dump_dir: Pat db_name: Name to give to the new database. """ - test_db = db_factory(dump_dir, db_name) - assert test_db.server_url == request.config.getoption("server") - assert test_db.name == db_name - if dump_dir.is_absolute(): - assert test_db.dump_dir == dump_dir - else: - assert test_db.dump_dir.stem == str(dump_dir) + if make_absolute: + dump_dir = Path(data_dir, dump_dir).absolute() + test_db: UnitTestDB = db_factory(dump_dir, db_name) + assert test_db.dbc.url.startswith(request.config.getoption("server")) + assert Path(test_db.dbc.db_name).stem.endswith(db_name) + assert set(test_db.dbc.tables.keys()) == set(expected_tables) diff --git a/tests/plugin/test_plugin/dump_dir/gibberish.txt b/tests/plugin/test_plugin/dump_dir/gibberish.txt new file mode 100644 index 0000000..cbc82f3 --- /dev/null +++ b/tests/plugin/test_plugin/dump_dir/gibberish.txt @@ -0,0 +1,6 @@ +1 grp1 11 +2 grp1 12 +3 grp2 21 +4 grp2 22 +5 grp2 23 +6 grp3 31 diff --git a/tests/plugin/test_plugin/dump_dir/table.sql b/tests/plugin/test_plugin/dump_dir/table.sql new file mode 100644 index 0000000..7b1e856 --- /dev/null +++ b/tests/plugin/test_plugin/dump_dir/table.sql @@ -0,0 +1,7 @@ +CREATE TABLE `gibberish` ( + `id` INTEGER NOT NULL, + `grp` VARCHAR(20) DEFAULT "", + `value` INT DEFAULT NULL, + PRIMARY KEY (`id`, `grp`) +); +CREATE INDEX `id_idx` ON `gibberish` (`id`); From 46675930476f21c3db4059d86a109ef137eec302 Mon Sep 17 00:00:00 2001 From: Matthieu Barba Date: Fri, 12 Jul 2024 09:36:55 +0100 Subject: [PATCH 08/10] Update doc --- tests/plugin/test_plugin.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/plugin/test_plugin.py b/tests/plugin/test_plugin.py index cc87524..f1149d1 100644 --- a/tests/plugin/test_plugin.py +++ b/tests/plugin/test_plugin.py @@ -100,8 +100,11 @@ def test_db_factory( Args: request: Fixture that provides information of the requesting test function. db_factory: Fixture that provides a unit test database factory. + data_dir: Directory where this specific test data are stored. dump_dir: Directory path where the test database schema and content files are located. + make_absolute: change the dump_dir from relative to absolute (based on `data_dir`). db_name: Name to give to the new database. + expected_tables: List of tables that should be loaded in the test database. """ if make_absolute: From 11e997cffcb06af7e196d8bf857c440500d6d59a Mon Sep 17 00:00:00 2001 From: Matthieu Barba Date: Fri, 12 Jul 2024 09:37:19 +0100 Subject: [PATCH 09/10] Remove unused mockdb --- tests/plugin/test_plugin.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/tests/plugin/test_plugin.py b/tests/plugin/test_plugin.py index f1149d1..950f092 100644 --- a/tests/plugin/test_plugin.py +++ b/tests/plugin/test_plugin.py @@ -29,18 +29,6 @@ from ensembl.utils.database import StrURL, UnitTestDB -@dataclass -class MockTestDB: - """Mocks `UnitTestDB` class by just storing the three arguments provided.""" - - server_url: StrURL - dump_dir: StrPath - name: str - - def drop(self) -> None: - """Mocks `UnitTestDB.drop()` method.""" - - @pytest.mark.dependency(name="test_data_dir") def test_data_dir(request: FixtureRequest, data_dir: Path) -> None: """Tests the `data_dir` fixture. From 5c368a5bcb73dcc0cf159d317d36ccf105ed8cd1 Mon Sep 17 00:00:00 2001 From: Matthieu Barba Date: Fri, 12 Jul 2024 09:38:52 +0100 Subject: [PATCH 10/10] Remove unused imports --- tests/plugin/test_plugin.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/plugin/test_plugin.py b/tests/plugin/test_plugin.py index 950f092..05f6c14 100644 --- a/tests/plugin/test_plugin.py +++ b/tests/plugin/test_plugin.py @@ -18,15 +18,13 @@ """ from contextlib import nullcontext as does_not_raise -from dataclasses import dataclass from pathlib import Path from typing import Callable, ContextManager import pytest from pytest import FixtureRequest, param, raises -from ensembl.utils import StrPath -from ensembl.utils.database import StrURL, UnitTestDB +from ensembl.utils.database import UnitTestDB @pytest.mark.dependency(name="test_data_dir")