V3.0.6 fix reg test 5233 set warning t #5319
Conversation
Summary of ChangesHello @mirostauder, 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 addresses an issue in 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
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces admin-side stats flushes with a query-based digest reset, consumes all digest rows keeping the last, inverts Test 1 to expect no warning, gates digest validation to run only when a warning is present, and applies the same reset approach in Test 3. Adds trailing newline. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 effectively addresses the issues in reg_test_5233_set_warning-t.cpp by replacing the unimplemented PROXYSQL FLUSH STATS command with a functional SELECT statement to reset query digest statistics. It also correctly inverts the test logic and assertions to reflect the expected behavior after the bug fix, ensuring that the test accurately validates that UPDATE statements no longer trigger SET warnings. The changes improve the test's reliability and logical flow.
|
|
||
| // Clear stats to start fresh | ||
| MYSQL_QUERY(admin, "PROXYSQL FLUSH STATS"); | ||
| MYSQL_QUERY(admin, "SELECT * FROM stats_mysql_query_digest_reset"); |
There was a problem hiding this comment.
The replacement of the unimplemented PROXYSQL FLUSH STATS command with SELECT * FROM stats_mysql_query_digest_reset is a good fix. It ensures the test can properly clear statistics, making it more robust and functional.
| // But currently it does, so we expect match_count > 0 | ||
| bool warning_found = (match_count > 0); | ||
| ok(warning_found, "UPDATE statement triggers SET warning (bug confirmed) - match_count: %zu", match_count); | ||
| ok(!warning_found, "UPDATE statement does NOT trigger SET warning - match_count: %zu", match_count); |
There was a problem hiding this comment.
The inversion of the assertion logic to ok(!warning_found, ...) correctly reflects the expected behavior after the bug fix. However, there's a minor grammatical error in the assertion message: "does NOT starts" should be "does NOT start".
| ok(!warning_found, "UPDATE statement does NOT trigger SET warning - match_count: %zu", match_count); | |
| ok(!warning_found, "UPDATE statement does NOT start SET warning - match_count: %zu", match_count); |
|
|
||
| if (!digest_text.empty()) { | ||
| diag("Found digest text: %s", digest_text.c_str()); | ||
| if (warning_found) { |
There was a problem hiding this comment.
Encapsulating the digest text check within if (warning_found) is a logical improvement. This ensures that the digest is only analyzed if a warning was actually triggered, streamlining the test flow and preventing unnecessary checks.
| digest_text[3] == ' '); | ||
|
|
||
| ok(starts_with_set, "Digest text starts with 'SET ' (explains the bug)"); | ||
| ok(!starts_with_set, "Digest text does NOT starts with 'SET '"); |
There was a problem hiding this comment.
Similar to the previous comment, the assertion ok(!starts_with_set, ...) correctly reflects the expected outcome. Please correct the grammatical error in the message: "does NOT starts" should be "does NOT start".
| ok(!starts_with_set, "Digest text does NOT starts with 'SET '"); | |
| ok(!starts_with_set, "Digest text does NOT start with 'SET '"); |
| diag("UPDATE statement did NOT trigger SET warning, not checking digest"); | ||
| ok(1, "UPDATE statement did NOT trigger SET warning, not checking digest"); | ||
| } |
There was a problem hiding this comment.
The addition of this else block for warning_found is a good enhancement. It clearly handles the scenario where the UPDATE statement did not trigger a SET warning, providing a concise success message and avoiding redundant checks.
|
|
||
| // Clear stats again | ||
| MYSQL_QUERY(admin, "PROXYSQL FLUSH STATS"); | ||
| MYSQL_QUERY(admin, "SELECT * FROM stats_mysql_query_digest_reset"); |
There was a problem hiding this comment.
This change consistently replaces PROXYSQL FLUSH STATS with SELECT * FROM stats_mysql_query_digest_reset, ensuring that statistics are cleared correctly before the subsequent test steps.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
60-65: Test plan comment is now stale.The comment says "Test that warning appears (confirming bug exists)" but the test now verifies the opposite—that the warning does not appear after the fix.
📝 Suggested fix to update the plan comment
// Plan: 3 tests - // 1. Test that warning appears (confirming bug exists) - // 2. Check digest text in stats table + // 1. Test that UPDATE statement does NOT trigger SET warning (confirming bug is fixed) + // 2. Check digest text in stats table (only if warning was unexpectedly found) // 3. Test with actual SET statement for comparison plan(3);
🤖 Fix all issues with AI agents
In `@test/tap/tests/reg_test_5233_set_warning-t.cpp`:
- Around line 140-143: Update the stale comment that currently says "we expect
match_count > 0" to accurately reflect the new expectation that UPDATE
statements should NOT trigger the SET warning (i.e., match_count == 0); locate
the block using the variables match_count and warning_found and the test
assertion ok(!warning_found, ...), then change the comment to state that no
warning is expected for UPDATE statements and that match_count should be 0 to
avoid confusion for future maintainers.
🧹 Nitpick comments (2)
test/tap/tests/reg_test_5233_set_warning-t.cpp (2)
111-111: Result set from reset query is not consumed.The
stats_mysql_query_digest_resettable returns data when queried. While the reset side-effect occurs, the result set is not retrieved and freed, which could leave the connection in an unexpected state for subsequent queries.♻️ Suggested fix to consume and free the result
- MYSQL_QUERY(admin, "SELECT * FROM stats_mysql_query_digest_reset"); + MYSQL_QUERY(admin, "SELECT * FROM stats_mysql_query_digest_reset"); + { + MYSQL_RES* res = mysql_store_result(admin); + if (res) mysql_free_result(res); + }
192-192: Same unconsumed result issue as line 111.Apply the same fix here to consume and free the result set.
♻️ Suggested fix
- MYSQL_QUERY(admin, "SELECT * FROM stats_mysql_query_digest_reset"); + MYSQL_QUERY(admin, "SELECT * FROM stats_mysql_query_digest_reset"); + { + MYSQL_RES* res = mysql_store_result(admin); + if (res) mysql_free_result(res); + }
| // This is the bug: warning should NOT appear for UPDATE statements | ||
| // But currently it does, so we expect match_count > 0 | ||
| bool warning_found = (match_count > 0); | ||
| ok(warning_found, "UPDATE statement triggers SET warning (bug confirmed) - match_count: %zu", match_count); | ||
| ok(!warning_found, "UPDATE statement does NOT trigger SET warning - match_count: %zu", match_count); |
There was a problem hiding this comment.
Stale comment contradicts the new expectation.
The comment at lines 140-141 states "we expect match_count > 0", but the code on line 143 now expects !warning_found (i.e., no warning, match_count == 0). This inconsistency will confuse future maintainers.
📝 Suggested fix to update the comment
// This is the bug: warning should NOT appear for UPDATE statements
- // But currently it does, so we expect match_count > 0
+ // After the fix, UPDATE statements should not trigger this warning
bool warning_found = (match_count > 0);
ok(!warning_found, "UPDATE statement does NOT trigger SET warning - match_count: %zu", match_count);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This is the bug: warning should NOT appear for UPDATE statements | |
| // But currently it does, so we expect match_count > 0 | |
| bool warning_found = (match_count > 0); | |
| ok(warning_found, "UPDATE statement triggers SET warning (bug confirmed) - match_count: %zu", match_count); | |
| ok(!warning_found, "UPDATE statement does NOT trigger SET warning - match_count: %zu", match_count); | |
| // This is the bug: warning should NOT appear for UPDATE statements | |
| // After the fix, UPDATE statements should not trigger this warning | |
| bool warning_found = (match_count > 0); | |
| ok(!warning_found, "UPDATE statement does NOT trigger SET warning - match_count: %zu", match_count); |
🤖 Prompt for AI Agents
In `@test/tap/tests/reg_test_5233_set_warning-t.cpp` around lines 140 - 143,
Update the stale comment that currently says "we expect match_count > 0" to
accurately reflect the new expectation that UPDATE statements should NOT trigger
the SET warning (i.e., match_count == 0); locate the block using the variables
match_count and warning_found and the test assertion ok(!warning_found, ...),
then change the comment to state that no warning is expected for UPDATE
statements and that match_count should be 0 to avoid confusion for future
maintainers.
|
replace unimplemented admin command
PROXYSQL FLUSH STATSwith
SELECTon respectivestat_..._resettablefix inverted logic and test step dependency
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.