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

Jump to bottom

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

merged 1 commit into from
Mar 20, 2024

Conversation

max-pfeiffer
Copy link
Contributor

@max-pfeiffer max-pfeiffer commented Mar 15, 2024 •
edited
Loading

  • updated arangodb image version which now supports ARM CPUs
  • skipping test for legacy version on ARM CPU

fixes #449

Screenshot 2024-03-15 at 11 43 35

santi
santi reviewed Mar 15, 2024
View reviewed changes
modules/arangodb/tests/test_arangodb.py
@@ -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).

@alexanderankin alexanderankin changed the title fix: arangodb tests on ARM CPUs fix(arangodb): tests to pass on ARM CPUs - change default image to 3.11.x where ARM image is published Mar 20, 2024
@alexanderankin alexanderankin merged commit 7b58a50 into testcontainers:main Mar 20, 2024
15 checks passed
@github-actions github-actions bot mentioned this pull request Mar 20, 2024
Merged
alexanderankin pushed a commit that referenced this pull request Mar 24, 2024
@github-actions
chore(main): release testcontainers 4.2.0 (#484)
e962189 
🤖 I have created a release *beep* *boop*
---


##
[4.2.0](testcontainers-v4.1.0...testcontainers-v4.2.0)
(2024-03-24)


### Features

* support influxdb
([#413](#413))
([13742a5](13742a5))


### Bug Fixes

* **arangodb:** tests to pass on ARM CPUs - change default image to
3.11.x where ARM image is published
([#479](#479))
([7b58a50](7b58a50))
* **core:** DinD issues
[#141](#141),
[#329](#329)
([#368](#368))
([b10d916](b10d916))
* **core:** raise an exception when docker compose fails to start
[#258](#258)
([#485](#485))
([d61af38](d61af38))
* **core:** use auto_remove=True with reaper instance
([#499](#499))
([274a400](274a400))
* **docs:** update the non-existent main.yml badge
([#493](#493))
([1d10c1c](1d10c1c))
* Fix the return type of `DockerContainer.get_logs`
([#487](#487))
([cd72f68](cd72f68))
* **keycloak:** tests on aarch64, use image from [jboss -> quay],
change supported version [16+ -> 18+]
([#480](#480))
([5758310](5758310))
* **postgres:** doctest
([#473](#473))
([c9c6f92](c9c6f92))
* read the docs build works again
([#496](#496))
([dfd1781](dfd1781))
* readthedocs build - take 1
([#495](#495))
([b3b9901](b3b9901))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
bearrito pushed a commit to bearrito/testcontainers-python that referenced this pull request Mar 30, 2024
@max-pfeiffer
fix(arangodb): tests to pass on ARM CPUs - change default image to 3.…
b74e1c8 
…11.x where ARM image is published (testcontainers#479)

- updated arangodb image version which now supports ARM CPUs
- skipping test for legacy version on ARM CPU

fixes testcontainers#449

![Screenshot 2024-03-15 at 11 43
35](https://github.com/testcontainers/testcontainers-python/assets/13573675/9fe5edfc-9a83-44d2-beb5-9c917d41438c)
bearrito pushed a commit to bearrito/testcontainers-python that referenced this pull request Mar 30, 2024
@github-actions
chore(main): release testcontainers 4.2.0 (testcontainers#484)
f83b8f1 
🤖 I have created a release *beep* *boop*
---


##
[4.2.0](testcontainers/testcontainers-python@testcontainers-v4.1.0...testcontainers-v4.2.0)
(2024-03-24)


### Features

* support influxdb
([testcontainers#413](testcontainers#413))
([13742a5](testcontainers@13742a5))


### Bug Fixes

* **arangodb:** tests to pass on ARM CPUs - change default image to
3.11.x where ARM image is published
([testcontainers#479](testcontainers#479))
([7b58a50](testcontainers@7b58a50))
* **core:** DinD issues
[testcontainers#141](testcontainers#141),
[testcontainers#329](testcontainers#329)
([testcontainers#368](testcontainers#368))
([b10d916](testcontainers@b10d916))
* **core:** raise an exception when docker compose fails to start
[testcontainers#258](testcontainers#258)
([testcontainers#485](testcontainers#485))
([d61af38](testcontainers@d61af38))
* **core:** use auto_remove=True with reaper instance
([testcontainers#499](testcontainers#499))
([274a400](testcontainers@274a400))
* **docs:** update the non-existent main.yml badge
([testcontainers#493](testcontainers#493))
([1d10c1c](testcontainers@1d10c1c))
* Fix the return type of `DockerContainer.get_logs`
([testcontainers#487](testcontainers#487))
([cd72f68](testcontainers@cd72f68))
* **keycloak:** tests on aarch64, use image from [jboss -&gt; quay],
change supported version [16+ -> 18+]
([testcontainers#480](testcontainers#480))
([5758310](testcontainers@5758310))
* **postgres:** doctest
([testcontainers#473](testcontainers#473))
([c9c6f92](testcontainers@c9c6f92))
* read the docs build works again
([testcontainers#496](testcontainers#496))
([dfd1781](testcontainers@dfd1781))
* readthedocs build - take 1
([testcontainers#495](testcontainers#495))
([b3b9901](testcontainers@b3b9901))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderankin alexanderankin alexanderankin left review comments

@santi santi santi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Arangodb tests fail on machines with ARM CPU (Apple M1, M2)
3 participants
@max-pfeiffer @santi @alexanderankin