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: inconsistent test runs for community modules #497

Merged
merged 1 commit into from
Mar 30, 2024
Merged

fix: inconsistent test runs for community modules #497

merged 1 commit into from
Mar 30, 2024

Conversation

max-pfeiffer
Copy link
Contributor

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

  • Fixed inconsistencies for community module test runs: using all supported Python versions
  • Pinned runner version (best practice)

fixes #482

@max-pfeiffer max-pfeiffer changed the title fix: Module tests are now run on all supported Python versions fix: inconsistent test runs for community modules Mar 24, 2024
alexanderankin
alexanderankin reviewed Mar 24, 2024
View reviewed changes
Copy link
Collaborator

@alexanderankin alexanderankin left a comment

Choose a reason for hiding this comment

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

this is not arguing for or against, im just documenting the previous rationale.

i think making it the ubuntu version explicit may be justifiable. we have a follow up in the short term, as ubuntu 24 is just around the corner.

.github/workflows/ci-community.yml
needs: [track-modules]
if: ${{ needs.track-modules.outputs.changed_modules != '[]' }}
strategy:
fail-fast: false
matrix:
python-version: [ "3.11" ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

im prett sure we did this as a cost saving measure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexanderankin Please check what the job track-modules does: it tracks code changes in the modules and then runs the test job conditionally. So this is only executed when we actually have relevant code changes.

So I would say that job is already quite optimised with regard to pipeline usage time.

Testing with the Python versions which we actually want to support is kind of mandatory if you want to avoid our package breaking.

What would you suggest doing to cover these test cases?

Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit late to the party here, but fully support testing on all Python versions to be sure about compatibility. If we ever run into GA minutes becoming scarce, we can have a conversation about cost savings then

.github/workflows/ci-core.yml
python-version: ["3.9", "3.10", "3.11", "3.12"]
runs-on: ${{ matrix.os }}-latest
steps:
- uses: actions/checkout@v4
- name: Set up Python
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this is also aspirationally there for win/mac OSs, but again they are missing as a cost saving measure.

Copy link
Contributor Author

@max-pfeiffer max-pfeiffer Mar 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@alexanderankin Please be aware of the findings from @bearrito and me concerning ARM runners in this thread: #446

It looks like these runners might be usable mid- or long term. Maybe end of this year. We don't know.

Currently this matrix does not serve any purpose. So I consider this as dead code which can be removed.

@max-pfeiffer
Copy link
Contributor Author

this is not arguing for or against, im just documenting the previous rationale.

i think making it the ubuntu version explicit may be justifiable. we have a follow up in the short term, as ubuntu 24 is just around the corner.

@alexanderankin Thanks for your feedback. Much appreciated.

Let me please elaborate a bit why I consider pinning versions (also in this case) a good practice: as you just mentioned a new Ubuntu runner version will become available soon. If this happens our pipeline will start running on it when we use the "latest" version. If we do not test this new runner version before introducing it in our pipeline, we risk breaking our pipeline because some change that new runner introduces.

So consciously deciding about new versions and testing them prior to use them in production is avoiding this risk and keep things running.

@alexanderankin
Copy link
Collaborator

ok I'm going to merge because i guess it seems reasonable enough.

pinging for FYI/attention in case any issues come up with this change:

@alexanderankin alexanderankin merged commit 914f1e5 into testcontainers:main Mar 30, 2024
9 checks passed
@github-actions github-actions bot mentioned this pull request Mar 30, 2024
Merged
alexanderankin pushed a commit that referenced this pull request Apr 1, 2024
@github-actions
chore(main): release testcontainers 4.3.0 (#500)
f819c7a 
🤖 I have created a release *beep* *boop*
---


##
[4.3.0](testcontainers-v4.2.0...testcontainers-v4.3.0)
(2024-04-01)


### Features

* **client:** Add custom User-Agent in Docker client as
`tc-python/<version>`
([#507](#507))
([dd55082](dd55082))


### Bug Fixes

* Add CassandraContainer
([#476](#476))
([507e466](507e466))
* add chroma container
([#515](#515))
([0729bf4](0729bf4))
* Add Weaviate module
([#492](#492))
([90762e8](90762e8))
* **cassandra:** make cassandra dependency optional/test-only
([#518](#518))
([bddbaeb](bddbaeb))
* **core:** allow setting docker command path for docker compose
([#512](#512))
([63fcd52](63fcd52))
* **google:** add support for Datastore emulator
([#508](#508))
([3d891a5](3d891a5))
* Improved Oracle DB module
([#363](#363))
([6e6d8e3](6e6d8e3))
* inconsistent test runs for community modules
([#497](#497))
([914f1e5](914f1e5))
* **kafka:** Add redpanda testcontainer module
([#441](#441))
([451d278](451d278))
* **kafka:** wait_for_logs in kafka container to reduce lib requirement
([#377](#377))
([909107b](909107b))
* **keycloak:** container should use dedicated API endpoints to
determine container readiness
([#490](#490))
([2e27225](2e27225))
* **nats:** Client-Free(ish) NATS container
([#462](#462))
([302c73d](302c73d))
* **new:** add a new Docker Registry test container
([#389](#389))
([0f554fb](0f554fb))
* pass doctests, s/doctest/doctests/, run them in gha,
s/asyncpg/psycopg/ in doctest, fix keycloak flakiness: wait for first
user
([#505](#505))
([545240d](545240d))
* pass updated keyword args to Publisher/Subscriber client in
google/pubsub
[#161](#161)
([#164](#164))
([8addc11](8addc11))
* Qdrant module
([#463](#463))
([e8876f4](e8876f4))
* remove accidentally added pip in dev dependencies
([#516](#516))
([dee20a7](dee20a7))
* **ryuk:** Enable Ryuk test suite. Ryuk image 0.5.1 -> 0.7.0. Add
RYUK_RECONNECTION_TIMEOUT env variable
([#509](#509))
([472b2c2](472b2c2))

---
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: tests are run inconsistently in CI pipeline
3 participants
@max-pfeiffer @alexanderankin @santi