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

fix(arangodb): tests to pass on ARM CPUs - change default image to 3.11.x where ARM image is published #479

Merged
merged 1 commit into from
Mar 20, 2024
Merged

fix(arangodb): tests to pass on ARM CPUs - change default image to 3.11.x where ARM image is published #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 1 addition & 1 deletion modules/arangodb/testcontainers/arangodb/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class ArangoDbContainer(DbContainer):
>>> from testcontainers.arangodb import ArangoDbContainer
>>> from arango import ArangoClient

>>> with ArangoDbContainer("arangodb:3.9.1") as arango:
>>> with ArangoDbContainer("arangodb:3.11.8") as arango:
... client = ArangoClient(hosts=arango.get_connection_url())
...
... # Connect
Expand Down
18 changes: 9 additions & 9 deletions modules/arangodb/tests/test_arangodb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
from arango.exceptions import DatabaseCreateError, ServerVersionError

from testcontainers.arangodb import ArangoDbContainer
import platform

ARANGODB_IMAGE_NAME = "arangodb"
IMAGE_VERSION = "3.11.8"


def arango_test_ops(arango_client, expeced_version, username="root", password=""):
Expand Down Expand Up @@ -50,8 +52,7 @@ def test_docker_run_arango():
"""
Test ArangoDB container with default settings.
"""
image_version = "3.9.1"
image = f"{ARANGODB_IMAGE_NAME}:{image_version}"
image = f"{ARANGODB_IMAGE_NAME}:{IMAGE_VERSION}"
arango_root_password = "passwd"

with ArangoDbContainer(image) as arango:
Expand All @@ -62,22 +63,22 @@ def test_docker_run_arango():
with pytest.raises(DatabaseCreateError):
sys_db.create_database("test")

arango_test_ops(arango_client=client, expeced_version=image_version, password=arango_root_password)
arango_test_ops(arango_client=client, expeced_version=IMAGE_VERSION, password=arango_root_password)


def test_docker_run_arango_without_auth():
"""
Test ArangoDB container with ARANGO_NO_AUTH var set.
"""
image_version = "3.9.1"
image = f"{ARANGODB_IMAGE_NAME}:{image_version}"
image = f"{ARANGODB_IMAGE_NAME}:{IMAGE_VERSION}"

with ArangoDbContainer(image, arango_no_auth=True) as arango:
client = ArangoClient(hosts=arango.get_connection_url())

arango_test_ops(arango_client=client, expeced_version=image_version, password="")
arango_test_ops(arango_client=client, expeced_version=IMAGE_VERSION, password="")


@pytest.mark.skipif(platform.processor() == "arm", reason="Test does not run on machines with ARM CPU")
def test_docker_run_arango_older_version():
"""
Test ArangoDB container with older tag/version.
Expand All @@ -100,8 +101,7 @@ def test_docker_run_arango_random_root_password():
"""
Test ArangoDB container with ARANGO_RANDOM_ROOT_PASSWORD var set.
"""
image_version = "3.9.1"
image = f"{ARANGODB_IMAGE_NAME}:{image_version}"
image = f"{ARANGODB_IMAGE_NAME}:{IMAGE_VERSION}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is now only using version 3.11.x, maybe we can remove this test and instead only have a single test for the 3.11 version that works on both x86 and ARM architecture?

Or do we have a strategy on supporting official modules with older versions? @balint-backmaker @alexanderankin @kiview

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@santi I would also advocate for your proposal. It's not not clear to me what value that test provides. But I am not really into the depths of arangodb. Actually I never used it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have 4 tests in this file where it checks different combinations of passwords and an older version of the container image. Part of the testing also includes inserting data and checking that you get the desired data out again.

I don't think testing queries outside of a simple version check on the _system table should be in scope for Testcontainers, that is better suited for testing in the client package itself. Supplying the wrong password as part of connection string is also not really the scope of this project

Therefore I suggest:

  • Remove test_docker_run_arango_without_auth() and test_docker_run_arango_random_root_password(), then combine test_docker_run_arango() with arango_test_ops() and remove logic related to testing queries and DB setup other than checking the _system database for the current version.
  • Keep the old version test for now. Doesn't really hurt having it here for documented backwards compatibility

Copy link
Member

Choose a reason for hiding this comment

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

I really dont see a reason to change anything more than necessary here, now that we have filters to only run module specific tests when something in the module changes.

(sorry for radio silence, was stressed last week, then got sick (probably not a coincidence; still sick).

arango_root_password = "passwd"

with ArangoDbContainer(image, arango_random_root_password=True) as arango:
Expand All @@ -110,4 +110,4 @@ def test_docker_run_arango_random_root_password():
# Test invalid auth (we don't know the password in random mode)
sys_db = client.db("_system", username="root", password=arango_root_password)
with pytest.raises(ServerVersionError):
assert sys_db.version() == image_version
assert sys_db.version() == IMAGE_VERSION