From 1024b9e6f66dbfee44cd53cc512884717a6dfaa5 Mon Sep 17 00:00:00 2001 From: Yury Dzerin Date: Wed, 19 Jan 2022 10:58:15 +0100 Subject: [PATCH 1/4] fix databases.get_many function, add integration test for it --- src/firebolt/service/database.py | 40 ++++++---- tests/integration/conftest.py | 79 +++++++++++++++++++ tests/integration/dbapi/conftest.py | 62 --------------- .../resource_manager/test_engine.py | 16 +++- 4 files changed, 119 insertions(+), 78 deletions(-) create mode 100644 tests/integration/conftest.py diff --git a/src/firebolt/service/database.py b/src/firebolt/service/database.py index 58536155685..430fd2ed96d 100644 --- a/src/firebolt/service/database.py +++ b/src/firebolt/service/database.py @@ -43,10 +43,10 @@ def get_id_by_name(self, name: str) -> str: def get_many( self, - name_contains: str, - attached_engine_name_eq: str, - attached_engine_name_contains: str, - order_by: Union[str, DatabaseOrder], + name_contains: Optional[str] = None, + attached_engine_name_eq: Optional[str] = None, + attached_engine_name_contains: Optional[str] = None, + order_by: Optional[Union[str, DatabaseOrder]] = None, ) -> List[Database]: """ Get a list of databases on Firebolt. @@ -63,20 +63,30 @@ def get_many( A list of databases matching the filters. """ - if isinstance(order_by, str): - order_by = DatabaseOrder[order_by] + params = {"page.first": "1000"} + if order_by: + if isinstance(order_by, str): + order_by = DatabaseOrder[order_by] + params["order_by"] = order_by.name + + if name_contains: + params["filter.name_contains"] = name_contains + + if attached_engine_name_eq: + params["filter.attached_engine_name_eq"] = attached_engine_name_eq + + if attached_engine_name_contains: + params[ + "filter.attached_engine_name_contains" + ] = attached_engine_name_contains + response = self.client.get( - url=ACCOUNT_DATABASES_URL.format(account_id=self.account_id), - params={ - "filter.name_contains": name_contains, - "filter.attached_engine_name_eq": attached_engine_name_eq, - "filter.attached_engine_name_contains": attached_engine_name_contains, - "order_by": order_by.name, - }, + url=ACCOUNT_DATABASES_URL.format(account_id=self.account_id), params=params ) + return [ - Database.parse_obj_with_service(obj=d, database_service=self) - for d in response.json()["databases"] + Database.parse_obj_with_service(obj=d["node"], database_service=self) + for d in response.json()["edges"] ] def create( diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py new file mode 100644 index 00000000000..bd30f1a4b15 --- /dev/null +++ b/tests/integration/conftest.py @@ -0,0 +1,79 @@ +from logging import getLogger +from os import environ + +from pytest import fixture + +from firebolt.service.manager import Settings + +LOGGER = getLogger(__name__) + +ENGINE_URL_ENV = "ENGINE_URL" +ENGINE_NAME_ENV = "ENGINE_NAME" +STOPPED_ENGINE_URL_ENV = "STOPPED_ENGINE_URL" +STOPPED_ENGINE_NAME_ENV = "STOPPED_ENGINE_NAME" +DATABASE_NAME_ENV = "DATABASE_NAME" +USER_NAME_ENV = "USER_NAME" +PASSWORD_ENV = "PASSWORD" +ACCOUNT_NAME_ENV = "ACCOUNT_NAME" +API_ENDPOINT_ENV = "API_ENDPOINT" + + +def must_env(var_name: str) -> str: + assert var_name in environ, f"Expected {var_name} to be provided in environment" + LOGGER.info(f"{var_name}: {environ[var_name]}") + return environ[var_name] + + +@fixture(scope="session") +def rm_settings(api_endpoint, username, password) -> Settings: + return Settings( + server=api_endpoint, + user=username, + password=password, + default_region="us-east-1", + ) + + +@fixture(scope="session") +def engine_url() -> str: + return must_env(ENGINE_URL_ENV) + + +@fixture(scope="session") +def stopped_engine_url() -> str: + return must_env(STOPPED_ENGINE_URL_ENV) + + +@fixture(scope="session") +def engine_name() -> str: + return must_env(ENGINE_NAME_ENV) + + +@fixture(scope="session") +def stopped_engine_name() -> str: + return must_env(STOPPED_ENGINE_URL_ENV) + + +@fixture(scope="session") +def database_name() -> str: + return must_env(DATABASE_NAME_ENV) + + +@fixture(scope="session") +def username() -> str: + return must_env(USER_NAME_ENV) + + +@fixture(scope="session") +def password() -> str: + return must_env(PASSWORD_ENV) + + +@fixture(scope="session") +def account_name() -> str: + return must_env(ACCOUNT_NAME_ENV) + + +@fixture(scope="session") +def api_endpoint() -> str: + return must_env(API_ENDPOINT_ENV) diff --git a/tests/integration/dbapi/conftest.py b/tests/integration/dbapi/conftest.py index 5cb8ec542a0..fc8d5d8b9ac 100644 --- a/tests/integration/dbapi/conftest.py +++ b/tests/integration/dbapi/conftest.py @@ -1,6 +1,5 @@ from datetime import date, datetime from logging import getLogger -from os import environ from typing import List from pytest import fixture @@ -11,67 +10,6 @@ LOGGER = getLogger(__name__) -ENGINE_URL_ENV = "ENGINE_URL" -ENGINE_NAME_ENV = "ENGINE_NAME" -STOPPED_ENGINE_URL_ENV = "STOPPED_ENGINE_URL" -STOPPED_ENGINE_NAME_ENV = "STOPPED_ENGINE_NAME" -DATABASE_NAME_ENV = "DATABASE_NAME" -USER_NAME_ENV = "USER_NAME" -PASSWORD_ENV = "PASSWORD" -ACCOUNT_NAME_ENV = "ACCOUNT_NAME" -API_ENDPOINT_ENV = "API_ENDPOINT" - - -def must_env(var_name: str) -> str: - assert var_name in environ, f"Expected {var_name} to be provided in environment" - LOGGER.info(f"{var_name}: {environ[var_name]}") - return environ[var_name] - - -@fixture(scope="session") -def engine_url() -> str: - return must_env(ENGINE_URL_ENV) - - -@fixture(scope="session") -def stopped_engine_url() -> str: - return must_env(STOPPED_ENGINE_URL_ENV) - - -@fixture(scope="session") -def engine_name() -> str: - return must_env(ENGINE_NAME_ENV) - - -@fixture(scope="session") -def stopped_engine_name() -> str: - return must_env(STOPPED_ENGINE_URL_ENV) - - -@fixture(scope="session") -def database_name() -> str: - return must_env(DATABASE_NAME_ENV) - - -@fixture(scope="session") -def username() -> str: - return must_env(USER_NAME_ENV) - - -@fixture(scope="session") -def password() -> str: - return must_env(PASSWORD_ENV) - - -@fixture(scope="session") -def account_name() -> str: - return must_env(ACCOUNT_NAME_ENV) - - -@fixture(scope="session") -def api_endpoint() -> str: - return must_env(API_ENDPOINT_ENV) - @fixture def all_types_query() -> str: diff --git a/tests/integration/resource_manager/test_engine.py b/tests/integration/resource_manager/test_engine.py index dc5e7863497..c5c4b9eb25a 100644 --- a/tests/integration/resource_manager/test_engine.py +++ b/tests/integration/resource_manager/test_engine.py @@ -2,7 +2,7 @@ import pytest -from firebolt.service.manager import ResourceManager +from firebolt.service.manager import ResourceManager, Settings from firebolt.service.types import EngineStatusSummary @@ -47,3 +47,17 @@ def test_copy_engine(): engine_revision=rm.engine_revisions.get_by_key(engine.latest_revision_key), ) assert engine_copy + + +def test_databases_get_many(rm_settings: Settings, database_name): + rm = ResourceManager(rm_settings) + + # get all databases, at least one should be returned + databases = rm.databases.get_many() + assert len(databases) > 0 + assert database_name in {db.name for db in databases} + + # get all databases, with name_contains + databases = rm.databases.get_many(name_contains=database_name) + assert len(databases) > 0 + assert database_name in {db.name for db in databases} From 63ef3ee93a1e3d999a3232671b8701210d36595c Mon Sep 17 00:00:00 2001 From: Yury Dzerin Date: Thu, 20 Jan 2022 15:06:13 +0100 Subject: [PATCH 2/4] add a unit test for databases.get_many --- tests/unit/service/conftest.py | 12 ++++++++++ tests/unit/service/test_database.py | 34 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/tests/unit/service/conftest.py b/tests/unit/service/conftest.py index f45b73984e2..f70e17ade3f 100644 --- a/tests/unit/service/conftest.py +++ b/tests/unit/service/conftest.py @@ -213,6 +213,18 @@ def do_mock( return do_mock +@pytest.fixture +def databases_get_callback(databases_url: str, mock_database) -> Callable: + def get_databases_callback_inner( + request: httpx.Request = None, **kwargs + ) -> Response: + return to_response( + status_code=httpx.codes.OK, json={"edges": [{"node": mock_database.dict()}]} + ) + + return get_databases_callback_inner + + @pytest.fixture def databases_url(settings: Settings, account_id: str) -> str: return f"https://{settings.server}" + ACCOUNT_DATABASES_URL.format( diff --git a/tests/unit/service/test_database.py b/tests/unit/service/test_database.py index 93e2274af56..328b53089f7 100644 --- a/tests/unit/service/test_database.py +++ b/tests/unit/service/test_database.py @@ -1,3 +1,4 @@ +import re from typing import Callable from pytest_httpx import HTTPXMock @@ -64,3 +65,36 @@ def test_database_get_by_name( database = manager.databases.get_by_name(name=mock_database.name) assert database.name == mock_database.name + + +def test_database_get_many( + httpx_mock: HTTPXMock, + auth_callback: Callable, + auth_url: str, + provider_callback: Callable, + provider_url: str, + settings: Settings, + account_id_callback: Callable, + account_id_url: str, + database_get_by_name_callback: Callable, + database_get_by_name_url: str, + databases_get_callback: Callable, + databases_url: str, + mock_database: Database, +): + + httpx_mock.add_callback(auth_callback, url=auth_url) + httpx_mock.add_callback(provider_callback, url=provider_url) + httpx_mock.add_callback(account_id_callback, url=account_id_url) + httpx_mock.add_callback(auth_callback, url=auth_url) + httpx_mock.add_callback( + databases_get_callback, + url=re.compile(databases_url + "?[a-zA-Z0-9=&]*"), + method="GET", + ) + + manager = ResourceManager(settings=settings) + databases = manager.databases.get_many() + + assert len(databases) == 1 + assert databases[0].name == mock_database.name From 83d7aebae5ddb970c4f87a6ee12ffa082907827c Mon Sep 17 00:00:00 2001 From: Yury Dzerin Date: Thu, 20 Jan 2022 15:17:30 +0100 Subject: [PATCH 3/4] add a unit test for databases.get_many --- tests/unit/service/test_database.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/unit/service/test_database.py b/tests/unit/service/test_database.py index 328b53089f7..3c8805ce611 100644 --- a/tests/unit/service/test_database.py +++ b/tests/unit/service/test_database.py @@ -94,7 +94,11 @@ def test_database_get_many( ) manager = ResourceManager(settings=settings) - databases = manager.databases.get_many() + databases = manager.databases.get_many( + name_contains=mock_database.name, + attached_engine_name_eq="mockengine", + attached_engine_name_contains="mockengine", + ) assert len(databases) == 1 assert databases[0].name == mock_database.name From 73f845e72fc6b93ac6f288297e30096426af59c6 Mon Sep 17 00:00:00 2001 From: Yury Dzerin Date: Thu, 20 Jan 2022 15:38:08 +0100 Subject: [PATCH 4/4] extend get_many integration tests --- tests/integration/resource_manager/test_engine.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/integration/resource_manager/test_engine.py b/tests/integration/resource_manager/test_engine.py index c5c4b9eb25a..279629da87d 100644 --- a/tests/integration/resource_manager/test_engine.py +++ b/tests/integration/resource_manager/test_engine.py @@ -49,7 +49,7 @@ def test_copy_engine(): assert engine_copy -def test_databases_get_many(rm_settings: Settings, database_name): +def test_databases_get_many(rm_settings: Settings, database_name, engine_name): rm = ResourceManager(rm_settings) # get all databases, at least one should be returned @@ -61,3 +61,13 @@ def test_databases_get_many(rm_settings: Settings, database_name): databases = rm.databases.get_many(name_contains=database_name) assert len(databases) > 0 assert database_name in {db.name for db in databases} + + # get all databases, with name_contains + databases = rm.databases.get_many(attached_engine_name_eq=engine_name) + assert len(databases) > 0 + assert database_name in {db.name for db in databases} + + # get all databases, with name_contains + databases = rm.databases.get_many(attached_engine_name_contains=engine_name) + assert len(databases) > 0 + assert database_name in {db.name for db in databases}