-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[UT] Remove potentially buggy SQL test case for mem_pool property of resource groups #66104
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
[UT] Remove potentially buggy SQL test case for mem_pool property of resource groups #66104
Conversation
0e32622 to
712eeb3
Compare
|
The failing test case seems to be unrelated to my changes. |
|
@mergify rebase |
☑️ Nothing to do, the required conditions are not metDetails
|
..._resource_group_mem_pool/R/test_create_two_resource_groups_with_mem_pool_different_mem_limit
Show resolved
Hide resolved
|
@cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: T file not updated to match R file changes
The R (result) file had a show verbose resource groups all; command and its result block removed, but the corresponding T (template) file at test/sql/test_resource_group_mem_pool/T/test_create_two_resource_groups_with_mem_pool_different_mem_limit still contains two show verbose resource groups all; commands at lines 22-23. This creates a mismatch where the T file expects two executions of this command but the R file only defines expected results for one, which will likely cause test failures or incorrect test behavior.
test/sql/test_resource_group_mem_pool/R/test_create_two_resource_groups_with_mem_pool_different_mem_limit#L24-L28
Lines 24 to 28 in 712eeb3
| show verbose resource groups all; | |
| -- result: | |
| [REGEX]shared_resource_group_for_alex.+7.+55(\.\d+)?%.+NORMAL.+\(id=\d+,.+user=alex\)\s*(shared_pool_for_alex_and_brad) | |
| -- !result |
🧪 CI InsightsHere's what we observed from your CI run for eef6213. 🟢 All jobs passed!But CI Insights is watching 👀 |
Signed-off-by: arin-mirza <[email protected]>
Signed-off-by: arin-mirza <[email protected]>
f6c99e1 to
eef6213
Compare
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
|
@cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no bugs!
|
@Mergifyio backport branch-4.0 |
✅ Backports have been createdDetails
|
…resource groups (#66104) Signed-off-by: arin-mirza <[email protected]> (cherry picked from commit d36d837)
|
ignore backport check: 4.0.3 |
…resource groups (backport #66104) (#66531) Signed-off-by: arin-mirza <[email protected]> Co-authored-by: Arın Mirza <[email protected]>
…resource groups (StarRocks#66104) Signed-off-by: arin-mirza <[email protected]>
Why I'm doing:
I recently added E2E SQL test cases for testing the mem_pool property.
One of the test cases has an incorrect regex.
The intention was to assert that the expression cannot be matched in the result.
It instead tries to match a single character that is not present in the brackets.
The correct regex should use a negative lookahead to implement the intended behavior.
The test case, however, does not fail, as the result coincidentally does not start with any of these characters.
What I'm doing:
The check is actually redundant, as we already assert that the resource group was not created in the previous check.
Instead of adding another complicated regex with negative lookaheads, I am simply removing it.
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check:
Note
Removes an incorrect regex-based assertion and a duplicate
show verbose resource groups allcall from the mem_pool resource group test.R/test_create_two_resource_groups_with_mem_pool_different_mem_limitthat attempted to verify absence inshow verbose resource groups alloutput.T/test_create_two_resource_groups_with_mem_pool_different_mem_limitby removing a duplicateshow verbose resource groups allcall, leaving a single invocation.Written by Cursor Bugbot for commit eef6213. This will update automatically on new commits. Configure here.