Skip to content

Conversation

@shunping
Copy link
Collaborator

@shunping shunping commented Jul 1, 2025

A few tests are failing (https://github.com/apache/beam/actions/runs/15985918137) due to testcontainers upgrade in (#35216).

It seems that after 4.x, testcontainers deprecated port_to_expose (testcontainers/testcontainers-python#296).

This PR attempts to forward fix this by replacing port_to_expose with port in our tests.

@shunping shunping self-assigned this Jul 1, 2025
@shunping shunping requested a review from claudevdm July 1, 2025 20:04
{
"comment": "Modify this file in a trivial way to cause this test suite to run",
"modification": 11
"modification": 12
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont think this test suite is necessary, it runs on dataflow.

@shunping
Copy link
Collaborator Author

shunping commented Jul 1, 2025

Getting the following error:

E       ModuleNotFoundError: No module named 'MySQLdb'

../../build/gradleenv/1398941893/lib/python3.9/site-packages/sqlalchemy/dialects/mysql/mysqldb.py:149: ModuleNotFoundError

After searching on the web, it seems we will need to install mysqlclient.

@shunping
Copy link
Collaborator Author

shunping commented Jul 2, 2025

I am trying to find out the root cause of why we need MySQLdb after testcontainers upgrade.

I followed the failed test code(

'mysql': DbData(
lambda: MySqlContainer(), ['mysql:mysql-connector-java:8.0.28'],
'mysql',
'com.mysql.cj.jdbc.Driver')
}
@classmethod
def setUpClass(cls):
cls.containers = {}
cls.engines = {}
cls.jdbc_configs = {}
for db_type, db_data in cls.DB_CONTAINER_CLASSPATH_STRING.items():
container = cls.start_container(db_data.container_fn)
cls.containers[db_type] = container
cls.engines[db_type] = sqlalchemy.create_engine(
container.get_connection_url())
), extracted the following code block and ran it on version 3.7.1 and 4.10.0.

from testcontainers.mysql import MySqlContainer
container = MySqlContainer()
container.start()
print(container.get_connection_url())

Connection url from version 3.7.1

mysql+pymysql://test:test@localhost:32768/test

Connection url from version 4.10.0

mysql://test:test@localhost:32770/test

In version 3.7.1, it uses pymysql driver:
https://github.com/testcontainers/testcontainers-python/blob/28e3a471c32c1036dd5e37df13cdde3b1ba91000/testcontainers/mysql.py#L67

In version 4.10.0, if we do not specify dialect, it will not contain a driver in the url.

When we call sqlalchemy.create_engine(), the two urls are routed to different code paths

@shunping
Copy link
Collaborator Author

shunping commented Jul 2, 2025

By adding dialect when initializing MySqlContainer, i.e. container = MySqlContainer(dialect='pymysql'), I am able to get the same connection url in 4.10.0 as 3.7.1.

@shunping shunping force-pushed the fix-port-to-expose branch from 240df60 to e340e50 Compare July 2, 2025 02:14
@github-actions github-actions bot removed the build label Jul 2, 2025
@shunping
Copy link
Collaborator Author

shunping commented Jul 2, 2025

Looks like the previous failed test is green now.
https://github.com/apache/beam/actions/runs/16014666896/job/45178934425?pr=35501

@shunping shunping marked this pull request as ready for review July 2, 2025 03:12
@shunping
Copy link
Collaborator Author

shunping commented Jul 2, 2025

cc'ed @Amar3tto @damccorm

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2025

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2025

Assigning reviewers:

R: @jrmccluskey for label python.

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

LGTM

@shunping shunping merged commit 02c587b into apache:master Jul 2, 2025
92 of 93 checks passed
@shunping shunping deleted the fix-port-to-expose branch July 2, 2025 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants