Fix: reg_test_5233_set_warning-t #5294
Conversation
Summary of ChangesHello @rahim-kanji, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughTest reg_test_5233_set_warning was updated to perform setup and cleanup SQL via the proxy connection (instead of admin), added an early exit when Changes
Sequence Diagram(s)(omitted — change is a test connection-target swap and minor control-flow tweak, not a multi-component new feature) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes the regression test reg_test_5233_set_warning-t by redirecting the database setup and cleanup queries from the admin connection to the proxy connection. This ensures that DDL and DML statements are properly handled by the proxy and executed on the backend database. I have included one suggestion to improve the robustness of the test setup, addressing a potential failure point in clean environments.
| MYSQL_QUERY(proxy, "CREATE DATABASE IF NOT EXISTS test"); | ||
| MYSQL_QUERY(proxy, "USE test"); |
There was a problem hiding this comment.
While correctly changing the connection from admin to proxy for these queries, there is a latent issue with the test's setup. The proxy connection is established on line 91 by connecting directly to the test database. If this database does not exist, mysql_real_connect will fail, and the test will terminate before this CREATE DATABASE statement can be executed.
To ensure the test is robust and can run in an environment where the test database doesn't already exist, you should consider modifying the connection logic on line 91 to connect without a specific database (using NULL for the database name). This would allow the CREATE DATABASE and USE statements here to function as intended.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/tap/tests/reg_test_5233_set_warning-t.cpp (1)
102-105: LGTM! Correct fix to use proxy for MySQL DDL/DML.Using the proxy connection instead of admin is the right approach—admin is for ProxySQL configuration, while proxy forwards queries to the MySQL backend.
Minor note: Line 103 (
USE test) is redundant since the proxy connection at line 91 already specifies"test"as the default database.Optional: Remove redundant USE statement
MYSQL_QUERY(proxy, "CREATE DATABASE IF NOT EXISTS test"); - MYSQL_QUERY(proxy, "USE test"); MYSQL_QUERY(proxy, "CREATE TABLE IF NOT EXISTS setting (setting_id VARCHAR(100) PRIMARY KEY, value VARCHAR(100))"); MYSQL_QUERY(proxy, "INSERT IGNORE INTO setting (setting_id, value) VALUES ('foo', '1.0')");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/tap/tests/reg_test_5233_set_warning-t.cpp
🔇 Additional comments (1)
test/tap/tests/reg_test_5233_set_warning-t.cpp (1)
210-210: LGTM!Cleanup correctly uses the proxy connection, consistent with the setup changes.
rahim-kanji
force-pushed
the
v3.0_fix_reg_test_5233_set_warning-t_test
branch
from
45a8aa3 to
b9175f8
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/tap/tests/reg_test_5233_set_warning-t.cpp (1)
94-98: Missingmysql_close(proxy)on connection failure.When
mysql_real_connectfails for the proxy connection, onlyadminis closed. Theproxyhandle was already initialized viamysql_initon line 86 and should also be closed to avoid a resource leak.Suggested fix
if (!mysql_real_connect(proxy, cl.host, cl.username, cl.password, "test", cl.port, NULL, CLIENT_MULTI_STATEMENTS)) { diag("Failed to connect to ProxySQL proxy: %s", mysql_error(proxy)); + mysql_close(proxy); mysql_close(admin); return exit_status(); }
🧹 Nitpick comments (1)
test/tap/tests/reg_test_5233_set_warning-t.cpp (1)
105-108: Minor:USE testis redundant.The proxy connection is already established with the "test" database on line 94, making the
USE teststatement on line 106 unnecessary.Suggested fix
MYSQL_QUERY(proxy, "CREATE DATABASE IF NOT EXISTS test"); - MYSQL_QUERY(proxy, "USE test"); MYSQL_QUERY(proxy, "CREATE TABLE IF NOT EXISTS setting (setting_id VARCHAR(100) PRIMARY KEY, value VARCHAR(100))");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/tap/tests/reg_test_5233_set_warning-t.cpp
🔇 Additional comments (2)
test/tap/tests/reg_test_5233_set_warning-t.cpp (2)
69-71: LGTM!The early exit when
cl.getEnv()fails is correctly placed before any resource allocation (connections), preventing potential resource leaks and following the typical pattern for ProxySQL test initialization.
213-216: LGTM!The cleanup correctly uses the proxy connection (consistent with setup) and properly closes both connections afterward.
rahim-kanji
changed the title
(same as title)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.