Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#58 Made the pytest-slc plugin using the pyexasol_connection #59

Merged
merged 7 commits into from
Sep 23, 2024
Merged

#58 Made the pytest-slc plugin using the pyexasol_connection #59

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d72ddc2
#58 Made the plugin using the pyexasol_connection
ahsimb Sep 20, 2024
a68a312
#58 Made a change in the upload_slc fixture
ahsimb Sep 20, 2024
4854404
Apply suggestions from code review
ahsimb Sep 23, 2024
f442c8c
#58 Made two versions of the deploy_slc fixture
ahsimb Sep 23, 2024
fee95ac
#58 Made two versions of the deploy_slc fixture
ahsimb Sep 23, 2024
6c8238e
#58 Made two versions of the deploy_slc fixture
ahsimb Sep 23, 2024
a3ec49a
#58 Applied the suggestion from the review.
ahsimb Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pytest-slc/doc/changes/changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* [unreleased](unreleased.md)
* [0.1.0](changes_0.1.0.md)
* [0.2.0](changes_0.2.0.md)
* [0.3.0](changes_0.3.0.md)

<!--- This MyST Parser Sphinx directive is necessary to keep Sphinx happy. We need list here all release letters again, because release droid and other scripts assume Markdown --->
```{toctree}
Expand All @@ -12,5 +13,6 @@ hidden:
unreleased
changes_0.1.0
changes_0.2.0
changes_0.3.0

```
9 changes: 9 additions & 0 deletions pytest-slc/doc/changes/changes_0.3.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# 0.3.0 - 2024-09-20

## Summary

🚀 Starting with this release `pytest-exasol-slc` connects to the database using a fixture from `pytest-exasol-extension`.

## Bug fixes

* #58: Used the `pyexasol_connection` fixture from the `pytest-exasol-extension`.
24 changes: 12 additions & 12 deletions pytest-slc/exasol/pytest_slc/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
from exasol.pytest_backend import paralleltask
import pytest

import pyexasol
import exasol.bucketfs as bfs
from exasol.python_extension_common.deployment.language_container_deployer import LanguageContainerDeployer
from exasol.python_extension_common.deployment.language_container_builder import LanguageContainerBuilder
Expand Down Expand Up @@ -65,16 +64,17 @@ def export_slc(slc_builder, export_slc_async) -> Path | None:

@pytest.fixture(scope="session")
def upload_slc(slc_builder, export_slc,
backend_aware_database_params, backend_aware_bucketfs_params):
pyexasol_connection, backend_aware_bucketfs_params):
"""
The fixture uploads language container to a database, according to the selected
backends.
The fixture provides a function uploading the language container to a database,
according to the selected backends.
"""
if (slc_builder is not None) and (export_slc is not None):
pyexasol_connection = pyexasol.connect(**backend_aware_database_params)
bucketfs_path = bfs.path.build_path(**backend_aware_bucketfs_params,
path=BFS_CONTAINER_DIRECTORY)
deployer = LanguageContainerDeployer(pyexasol_connection=pyexasol_connection,
bucketfs_path=bucketfs_path,
language_alias=slc_builder.language_alias)
deployer.run(container_file=export_slc, alter_system=True, allow_override=True)
def func(language_alias: str) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, you changed it to that, such that we can isolate the aliases per test, if it necessary. I could assume, in case of the deployer tests and the extension tests that could be necessary. Or, did you think of a different reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the motivation is described in the related ticket #58:

If an extension test uses both pyexasol_connection fixture from the pytest-extension and upload_slc fixture from the pytest-slc then the container may not get activated for the pyexasol_connection session if the pyexasol_connection fixture is created first. To make sure this doesn't happen the pytest-slc should depend on the pytest-extension and use its pyexasol_connection fixture instead of creating its own connection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I have some more questions:

  1. Could we make the function func use the language_alias that is already known to the slc_builder?
  2. Why does the fixture return a function instead of just uploading the SLC?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question was related to the function and not the Pyexasol connection

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to take the language_alias from the LanguageContainerBuilder. As we found, the language_alias is not actually required to build a language container; it is only for testing it, which we don't do. So it was removed from the LanguageContainerBuilder. But we do need it for the container deployer. Hence is the upload_slc function. We can provide the language_alias there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But good point , maybe we need a fixture that returns a function that activates the SLC for a connection

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to take the language_alias from the Builder ... language_alias is not actually required to build a language container ... But for the deployer. ...

Ah - very good, thank you!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decision:

  • Rename fixture upload_slc to deploy_slc
  • Add fixture deployed_slc which uses a dummy fixture for alias which needs be overwritten by the extension
  • the function returned by deploy_slc fixture uses a state variable stored in deploy_slc fixture to upload the slc only once and only activate it in subsequent calls

if (slc_builder is not None) and (export_slc is not None):
bucketfs_path = bfs.path.build_path(**backend_aware_bucketfs_params,
path=BFS_CONTAINER_DIRECTORY)
deployer = LanguageContainerDeployer(pyexasol_connection=pyexasol_connection,
bucketfs_path=bucketfs_path,
language_alias=language_alias)
deployer.run(container_file=export_slc, alter_system=True, allow_override=True)
return func
2 changes: 1 addition & 1 deletion pytest-slc/exasol/pytest_slc/version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@
# Do not edit this file manually!
# If you need to change the version, do so in the project.toml, e.g. by using `poetry version X.Y.Z`.
MAJOR = 0
MINOR = 2
MINOR = 3
PATCH = 0
VERSION = f"{MAJOR}.{MINOR}.{PATCH}"
452 changes: 241 additions & 211 deletions pytest-slc/poetry.lock
View file
Open in desktop

Large diffs are not rendered by default.

7 changes: 4 additions & 3 deletions pytest-slc/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tool.poetry]
name = "pytest-exasol-slc"
version = "0.2.0"
version = "0.3.0"
description = ""
authors = ["Mikhail Beck <[email protected]>"]
readme = "README.md"
Expand All @@ -9,8 +9,9 @@ packages = [{include = "exasol"}]
[tool.poetry.dependencies]
python = ">=3.10,<4"
pytest = ">=7,<9"
pytest-exasol-backend = ">=0.2.0"
exasol-python-extension-common = ">=0.4.0"
pytest-exasol-backend = ">=0.3.0"
pytest-exasol-extension = ">=0.1.0"
exasol-python-extension-common = ">=0.5.0"

[tool.poetry.plugins.pytest11]
slc = "exasol.pytest_slc"
Expand Down
9 changes: 5 additions & 4 deletions pytest-slc/test/integration/pytest_slc_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@

pytest_plugins = ["pytester"]

LANGUAGE_ALIAS = 'PYTHON3_PYTEST_SLC'

_test_code = dedent(fr"""
import pyexasol
import pytest
from exasol.python_extension_common.deployment.language_container_validator import temp_schema
from exasol.python_extension_common.deployment.language_container_builder import (
LanguageContainerBuilder, find_path_backwards)

LANGUAGE_ALIAS = 'PYTHON3_PYTEST_SLC'

@pytest.fixture(scope='session')
def slc_builder() -> LanguageContainerBuilder:
with LanguageContainerBuilder('test_container', LANGUAGE_ALIAS) as container_builder:
with LanguageContainerBuilder('test_container') as container_builder:
project_directory = find_path_backwards("pyproject.toml", "{__file__}").parent
container_builder.prepare_flavor(project_directory)
yield container_builder
Expand All @@ -24,7 +24,7 @@ def assert_udf_running(conn: pyexasol.ExaConnection):
with temp_schema(conn) as schema:
udf_name = 'TEST_UDF'
udf_create_sql = (
f'CREATE OR REPLACE {{LANGUAGE_ALIAS}} SCALAR SCRIPT "{{schema}}"."{{udf_name}}"() '
f'CREATE OR REPLACE {LANGUAGE_ALIAS} SCALAR SCRIPT "{{schema}}"."{{udf_name}}"() '
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure, but shouldn't all parameters be enclosed only in single braces to allow f-string substitution?

Suggested change
f'CREATE OR REPLACE {LANGUAGE_ALIAS} SCALAR SCRIPT "{{schema}}"."{{udf_name}}"() '
f'CREATE OR REPLACE {LANGUAGE_ALIAS} SCALAR SCRIPT "{schema}"."{udf_name}"() '

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a piece of code, which itself uses f-strings. This is the case for the schema and udf_name. In this code, the LANGUAGE_ALIAS is a constant which we insert externally, also using an f-string. Basically, there is an external and internal f-string.

'RETURNS BOOLEAN AS '
'def run(ctx): '
'return True '
Expand All @@ -35,6 +35,7 @@ def assert_udf_running(conn: pyexasol.ExaConnection):
assert result[0][0] is True
ckunki marked this conversation as resolved.
Show resolved Hide resolved

def test_upload_slc(upload_slc, backend_aware_database_params):
upload_slc("{LANGUAGE_ALIAS}")
assert_udf_running(pyexasol.connect(**backend_aware_database_params))
""")

Expand Down
Loading