-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
Adapt testcontainers to be testing framework agnostic #82
Changes from 8 commits
8cd73b2
40bdc7e
4a1c910
3770849
8bad275
59a5484
7924b0d
2c643bd
661370f
7ae9a4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,6 +10,10 @@ | |||||
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||||||
# License for the specific language governing permissions and limitations | ||||||
# under the License. | ||||||
# This is for Python 3.7 and 3.8 to support generic types | ||||||
# like `dict` instead of `typing.Dict | ||||||
from __future__ import annotations | ||||||
|
||||||
import logging | ||||||
import os | ||||||
from typing import Optional | ||||||
|
@@ -19,6 +23,7 @@ | |||||
from testcontainers.core.waiting_utils import wait_container_is_ready, wait_for_logs | ||||||
|
||||||
from cratedb_toolkit.testing.testcontainers.util import KeepaliveContainer, asbool | ||||||
from cratedb_toolkit.util import DatabaseAdapter | ||||||
|
||||||
logger = logging.getLogger(__name__) | ||||||
|
||||||
|
@@ -38,6 +43,7 @@ | |||||
>>> import sqlalchemy | ||||||
|
||||||
>>> cratedb_container = CrateDBContainer("crate:5.2.3") | ||||||
>>> cratedb_container.start() | ||||||
>>> with cratedb_container as cratedb: | ||||||
... engine = sqlalchemy.create_engine(cratedb.get_connection_url()) | ||||||
... with engine.begin() as connection: | ||||||
|
@@ -51,44 +57,87 @@ | |||||
CRATEDB_PASSWORD = os.environ.get("CRATEDB_PASSWORD", "") | ||||||
CRATEDB_DB = os.environ.get("CRATEDB_DB", "doc") | ||||||
KEEPALIVE = asbool(os.environ.get("CRATEDB_KEEPALIVE", os.environ.get("TC_KEEPALIVE", False))) | ||||||
CMD_OPTS = { | ||||||
"discovery.type": "single-node", | ||||||
"node.attr.storage": "hot", | ||||||
"path.repo": "/tmp/snapshots", | ||||||
} | ||||||
|
||||||
# TODO: Dual-port use with 4200+5432. | ||||||
def __init__( | ||||||
self, | ||||||
image: str = "crate/crate:nightly", | ||||||
port: int = 4200, | ||||||
ports: Optional[dict] = None, | ||||||
user: Optional[str] = None, | ||||||
password: Optional[str] = None, | ||||||
dbname: Optional[str] = None, | ||||||
dialect: str = "crate", | ||||||
cmd_opts: Optional[dict] = None, | ||||||
**kwargs, | ||||||
) -> None: | ||||||
""" | ||||||
:param image: docker hub image path with optional tag | ||||||
:param ports: optional dict that maps a port inside the container to a port on the host machine; | ||||||
`None` as a map value generates a random port; | ||||||
Dicts are ordered. By convention the first key-val pair is designated to the HTTP interface. | ||||||
Example: {4200: None, 5432: 15431} - port 4200 inside the container will be mapped | ||||||
matriv marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
to a random port on the host, internal port 5432 for PSQL interface will be mapped | ||||||
to the 15432 port on the host. | ||||||
:param user: optional username to access the DB; if None, try respective environment variable | ||||||
matriv marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
:param password: optional password to access the DB; if None, try respective environment variable | ||||||
:param dbname: optional database name to access the DB; if None, try respective environment variable | ||||||
:param dialect: a string with the dialect to generate a DB URI | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we want to expose There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not 100% sure, but I think when passed The FootnotesThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not saying that it's part of the PR, just mentioning that it seems weird to me to expose a "dialect". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I've double checked the dialect thing and you are right @matriv - no need to have it in the class constructor. It's enough to have it in the |
||||||
:param cmd_opts: an optional dict with CLI arguments to be passed to the DB entrypoint inside the container | ||||||
:param kwargs: misc keyword arguments | ||||||
""" | ||||||
super().__init__(image=image, **kwargs) | ||||||
|
||||||
self._name = "testcontainers-cratedb" # -{os.getpid()} | ||||||
self._command = "-Cdiscovery.type=single-node -Ccluster.routing.allocation.disk.threshold_enabled=false" | ||||||
# TODO: Generalize by obtaining more_opts from caller. | ||||||
self._command += " -Cnode.attr.storage=hot" | ||||||
self._command += " -Cpath.repo=/tmp/snapshots" | ||||||
self._name = "testcontainers-cratedb" | ||||||
|
||||||
cmd_opts = cmd_opts if cmd_opts else {} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a nit: I usually write this like that. Is it a good idea?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||
self._command = self._build_cmd({**self.CMD_OPTS, **cmd_opts}) | ||||||
|
||||||
self.CRATEDB_USER = user or self.CRATEDB_USER | ||||||
self.CRATEDB_PASSWORD = password or self.CRATEDB_PASSWORD | ||||||
self.CRATEDB_DB = dbname or self.CRATEDB_DB | ||||||
|
||||||
self.port_to_expose = port | ||||||
self.port_mapping = ports if ports else {4200: None} | ||||||
self.port_to_expose, _ = list(self.port_mapping.items())[0] | ||||||
self.dialect = dialect | ||||||
|
||||||
def _configure(self) -> None: | ||||||
self.with_exposed_ports(self.port_to_expose) | ||||||
@staticmethod | ||||||
def _build_cmd(opts: dict) -> str: | ||||||
""" | ||||||
Return a string with command options concatenated and optimised for ES5 use | ||||||
""" | ||||||
cmd = [] | ||||||
for key, val in opts.items(): | ||||||
if isinstance(val, bool): | ||||||
val = str(val).lower() | ||||||
cmd.append("-C{}={}".format(key, val)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason not to use f-strings already?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||||||
return " ".join(cmd) | ||||||
|
||||||
def _configure_ports(self) -> None: | ||||||
""" | ||||||
Bind all the ports exposed inside the container to the same port on the host | ||||||
""" | ||||||
# If host_port is `None`, a random port to be generated | ||||||
for container_port, host_port in self.port_mapping.items(): | ||||||
self.with_bind_ports(container=container_port, host=host_port) | ||||||
|
||||||
def _configure_credentials(self) -> None: | ||||||
self.with_env("CRATEDB_USER", self.CRATEDB_USER) | ||||||
self.with_env("CRATEDB_PASSWORD", self.CRATEDB_PASSWORD) | ||||||
self.with_env("CRATEDB_DB", self.CRATEDB_DB) | ||||||
|
||||||
def get_connection_url(self, host=None) -> str: | ||||||
def _configure(self) -> None: | ||||||
self._configure_ports() | ||||||
self._configure_credentials() | ||||||
|
||||||
def get_connection_url(self, host=None, dialect=None) -> str: | ||||||
# TODO: When using `db_name=self.CRATEDB_DB`: | ||||||
# Connection.__init__() got an unexpected keyword argument 'database' | ||||||
return super()._create_connection_url( | ||||||
dialect=self.dialect, | ||||||
dialect=dialect or self.dialect, | ||||||
username=self.CRATEDB_USER, | ||||||
password=self.CRATEDB_PASSWORD, | ||||||
host=host, | ||||||
|
@@ -101,3 +150,52 @@ | |||||
# In `testcontainers-java`, there is the `HttpWaitStrategy`. | ||||||
# TODO: Provide a client instance. | ||||||
wait_for_logs(self, predicate="o.e.n.Node.*started", timeout=MAX_TRIES) | ||||||
|
||||||
|
||||||
class CrateDBTestAdapter: | ||||||
""" | ||||||
A little helper wrapping Testcontainer's `CrateDBContainer` and | ||||||
CrateDB Toolkit's `DatabaseAdapter`, agnostic of the test framework. | ||||||
""" | ||||||
|
||||||
def __init__(self, crate_version: str = "nightly", **kwargs): | ||||||
self.cratedb: Optional[CrateDBContainer] = None | ||||||
self.database: Optional[DatabaseAdapter] = None | ||||||
self.image: str = "crate/crate:{}".format(crate_version) | ||||||
|
||||||
def start(self, **kwargs): | ||||||
""" | ||||||
Start testcontainer, used for tests set up | ||||||
""" | ||||||
self.cratedb = CrateDBContainer(image=self.image, **kwargs) | ||||||
self.cratedb.start() | ||||||
self.database = DatabaseAdapter(dburi=self.get_connection_url()) | ||||||
|
||||||
def stop(self): | ||||||
""" | ||||||
Stop testcontainer, used for tests tear down | ||||||
""" | ||||||
if self.cratedb: | ||||||
self.cratedb.stop() | ||||||
|
||||||
def reset(self, tables: Optional[list] = None): | ||||||
""" | ||||||
Drop tables from the given list, used for tests set up or tear down | ||||||
""" | ||||||
if tables and self.database: | ||||||
for reset_table in tables: | ||||||
self.database.connection.exec_driver_sql(f"DROP TABLE IF EXISTS {reset_table};") | ||||||
|
||||||
def get_connection_url(self, *args, **kwargs): | ||||||
""" | ||||||
Return a URL for SQLAlchemy DB engine | ||||||
""" | ||||||
if self.cratedb: | ||||||
return self.cratedb.get_connection_url(*args, **kwargs) | ||||||
return None | ||||||
|
||||||
def get_http_url(self, **kwargs): | ||||||
""" | ||||||
Return a URL for CrateDB's HTTP endpoint | ||||||
""" | ||||||
return self.get_connection_url(dialect="http", **kwargs) | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a partial hot/cold targeted custom setup which should be irrelevant for generic test clusters and especially in a single-node setup.
@amotl Any reason for this configuration? (as it was in the codebase already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, well spotted. It is indeed a configuration detail which is specific to the test cases for
cratedb_toolkit.retention
, where this code was initially conceived for.In this spirit, it should not be part of the generic startup parameters, but at the same time, it shows we need the capacity to configure those details when needed.
@pilosus: Do you think we can improve this spot, so that corresponding configuration settings can be defined on behalf of the snippet in
conftest.py
? This time, it will probably not be so easy, because the test adapter will already need this information at startup time. Maybe you have an idea how to handle this elegantly?While being at it: Of course, it would not just be about the specific
node.attr.storage
parameter, but about any other parameters as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see you already added
cmd_opts
to the constructor. 🙇https://github.com/crate-workbench/cratedb-toolkit/blob/661370ffb0619e2a4c698c52627c99a1fb726bad/cratedb_toolkit/testing/testcontainers/cratedb.py#L94-L95
So,
{"node.attr.storage": "hot", "path.repo": "/tmp/snapshots"}
would just need to be moved over to the caller.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. And given that we use dict merge, you can even override the default settings
I'll remove the
node.attr.storage
from the default thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. What do you think about
path.repo
, @seut? It could be convenient for testing to have it configured by default. Because you didn't mention it in your request, do you think it can stay?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seut @amotl hmm, when I delete
node.attr.storage=hot
tests:
tests/retention/test_cli.py::test_run_delete_basic
tests/retention/test_cli.py::test_run_delete_dryrun
tests/retention/test_cli.py::test_run_reallocate
hang.
Here are the logs:
Interestingly enough,
test_run_delete_with_tags_match
works wellThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the test cases defined for
cratedb_toolkit.retention
need this setting to be configured, so it will need to go intotests/conftest.py
somehow. On the other hand, it should not be part of the generic configuration. That's yet another cliff we need to take.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the whole chain of the nested fixtures with different scopes, it will either require some more time from me next week, or a simpler solution with
cratedb
override fortests/retention/conftest.py
that havenode.attr.storage=hot
in it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can easily handle that on a later iteration, and/or discuss possible solutions beforehand. Thank you.