Skip to content

test (backend) Added containerization support in testing and replaced SQLite with MySQL in the api-server/run_store_test #11889

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ntny
Copy link
Contributor

@ntny ntny commented May 3, 2025

Description of your changes:

Motivation:

  1. While it's lightweight and fast, SQLite is less strict than MySQL in terms of data types, constraints, and SQL dialect compliance. This can lead to false positives — tests passing in SQLite but failing in production when using MySQL.
    For example, the attached test passes when using SQLite, but fails on MySQL due to stricter enforcement of constraints.
  2. Introducing Testcontainers simplifies testing when adding PostgreSQL support. Existing tests can be reused for both MySQL and PostgreSQL.
  3. Another nice thing is that you’ll be able to use tests for debugging the generated queries, since they now use the MySQL dialect — the same one as in production.

First approach implementation, applied only to run_details.

I ignored the test TestListRuns_Pagination_WithSortingOnMetrics because it generates an invalid query from the MySQL perspective (test was false-positive on SQLite).
Below are two SQLite queries that will not work using the MySQL dialect
query 1 without where by dummymetric contains the problem in the

SELECT
  ...
  rd.dummymetric
FROM (...)
GROUP BY rd.UUID

Here rd.dummymetric is not included in the GROUP BY clause and is not aggregated, but it is used in the SELECT statement. will only work if the sql_mode is lowered for MySQL and the GROUP_BY_FULL mode is removed, because there is no aggregation for the dummymetric field in the query. Without this adjustment, MySQL will throw an error
this query was generated here

mysql_and_where.txt doesn't work at all in MySQL. With exception ' Unknown column 'dummymetric' in 'where clause''
the second query was generated here

The SQLite alternatives to the above queries contain the same issues but still work because SQLite performs less strict query validation.

Also, just to double-check, I have reproduced the first query on my own Kubeflow Pipelines setup and it also threw the same error during the tests(dummymetric is just a random alias):
group by issue

Additionally, since I am creating a container and reusing it for performance reasons, I had to add logic for cleaning the database between tests, which complicated the initialization logic in run_details_test

Copy link

Hi @ntny. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link

github-actions bot commented May 3, 2025

🚫 This command cannot be processed. Only organization members or owners can use the commands.

@google-oss-prow google-oss-prow bot requested review from droctothorpe and HumairAK May 3, 2025 21:34
@ntny ntny force-pushed the test(backend)-add-integration-test-to-storage branch from 56a8536 to 5c4129c Compare May 3, 2025 22:02
@ntny ntny changed the title test (backend) Replace in-memory SQLite with MySQL using Testcontainers in api-server. test (backend) Added containerization support in testing: replaced SQLite with MySQL using Testcontainers in the api-server. May 4, 2025
@ntny ntny changed the title test (backend) Added containerization support in testing: replaced SQLite with MySQL using Testcontainers in the api-server. test (backend) Added containerization support in testing and replaced SQLite with MySQL using Testcontainers in the api-server. May 4, 2025
@ntny ntny changed the title test (backend) Added containerization support in testing and replaced SQLite with MySQL using Testcontainers in the api-server. test (backend) Added containerization support in testing and replaced SQLite with MySQL using Testcontainers in the api-server/run_store_test May 4, 2025
@ntny ntny changed the title test (backend) Added containerization support in testing and replaced SQLite with MySQL using Testcontainers in the api-server/run_store_test test (backend) Added containerization support in testing and replaced SQLite with MySQL in the api-server/run_store_test May 4, 2025
@ntny ntny force-pushed the test(backend)-add-integration-test-to-storage branch 7 times, most recently from 71362a2 to 1aa1937 Compare May 5, 2025 18:43
@ntny
Copy link
Contributor Author

ntny commented May 7, 2025

Hello @HumairAK,
I found the closed issue with a similar motivation described.
There are several merged commits mostly related to preparation for switching from SQLite testing to MySQL.

However, SQLite tests are still being used in api-server/storage, and it seems that #8851 is still relevant and the current PR partially closed it.
Could you please take a look and correct me if I'm wrong?

@HumairAK HumairAK moved this to In Review in KFP Project Tracker May 7, 2025
@ntny ntny force-pushed the test(backend)-add-integration-test-to-storage branch from 1aa1937 to db3aa28 Compare May 15, 2025 21:40
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign droctothorpe for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ners/mysql.

- replace in-memory SQLite with MySQL 8.0 using Testcontainers in run_store_test
- ignored false-positive test

Signed-off-by: ntny <[email protected]>
Signed-off-by: arpechenin <[email protected]>
@ntny ntny force-pushed the test(backend)-add-integration-test-to-storage branch from db3aa28 to 5d09c4d Compare May 15, 2025 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

1 participant