Skip to content
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

chore: stable bvt/result_count.sql result check and enable load_data_LOCAL_infile case in mo1.2 #16478

Merged
merged 4 commits into from
May 30, 2024

Conversation

xzxiong
Copy link
Contributor

@xzxiong xzxiong commented May 29, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue ##14836, #16424

What this PR does / why we need it:

ref: #16474
changes:

  1. sepract check op from result_count/result_count.sql and put it into zz_statement_query_type/result_count.sql
  2. active bvt load data LOCAL infile

PR Type

Bug fix, Tests


Description

  • Refactored test cases for account operations by updating account names and moving result checks to a new file.
  • Added new test cases and result check operations for various SQL statements.
  • Improved the organization and readability of test files by separating concerns.

Changes walkthrough 📝

Relevant files
Tests
result_count.result
Update and streamline test cases for account operations   

test/distributed/cases/result_count/result_count.result

  • Removed multiple test cases related to account operations and SQL
    statements.
  • Added new test cases for creating and dropping accounts with a new
    naming convention.
  • +2/-62   
    result_count.sql
    Refactor account operation tests and move result checks   

    test/distributed/cases/result_count/result_count.sql

  • Updated account names in test cases to follow a new naming convention.
  • Removed result check operations and moved them to a new file.
  • +4/-10   
    result_count.result
    Add result check operations for SQL statements                     

    test/distributed/cases/zz_statement_query_type/result_count.result

  • Added result check operations for various SQL statements.
  • Included new test cases for verifying statement results.
  • +59/-0   
    result_count.sql
    Move and enhance result check operations                                 

    test/distributed/cases/zz_statement_query_type/result_count.sql

  • Moved result check operations from the original file.
  • Added conditions to filter statements based on length and content.
  • +15/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    … into zz_statement_query_type/result_count.sql
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the PR primarily involves refactoring and updating test cases, which are generally straightforward to review. The changes are well-documented and isolated to specific test files, reducing the complexity of the review.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The use of hardcoded passwords ('111' and '******') in test cases could be a concern if these patterns are replicated in production code or if the test environment is not properly isolated.

    🔒 Security concerns

    No

    @matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label May 29, 2024
    @mergify mergify bot requested a review from sukki37 May 29, 2024 07:31
    @mergify mergify bot added kind/bug Something isn't working kind/test-ci labels May 29, 2024
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Use a stronger password for the create account statement to enhance security

    Consider using a more secure password for the create account statement to avoid potential
    security risks associated with using a simple password like '111'.

    test/distributed/cases/result_count/result_count.result [137]

    -/* cloud_user */create account bvt_result_count_test_case2 admin_name 'test_account' identified by '111';
    +/* cloud_user */create account bvt_result_count_test_case2 admin_name 'test_account' identified by 'Str0ngP@ssw0rd!';
     
    Suggestion importance[1-10]: 8

    Why: Using a stronger password is crucial for security, especially for account creation statements in a database.

    8
    Best practice
    Use drop account if exists to prevent errors if the account does not exist

    To ensure that the drop account statement does not fail if the account does not exist,
    consider using drop account if exists instead.

    test/distributed/cases/result_count/result_count.sql [108]

    -/* cloud_user */drop account bvt_result_count_test_case2;
    +/* cloud_user */drop account if exists bvt_result_count_test_case2;
     
    Suggestion importance[1-10]: 7

    Why: Using drop account if exists is a best practice to avoid errors when the specified account does not exist, ensuring smoother operation and error handling in SQL scripts.

    7
    Enhancement
    Add comments to explain the purpose of setting case names for better readability

    To improve readability and maintainability, consider adding comments to explain the
    purpose of the set @case_name statements.

    test/distributed/cases/zz_statement_query_type/result_count.result [2-15]

    +-- Set case name for case 1
     set @case_name="case1";
     select statement, result_count from statement_info where account="bvt_result_count" and statement not like '%mo_ctl%' and length(statement) > 0 and status != 'Running' and aggr_count < 1 order by request_at desc limit 50;
     ...
    +-- Set case name for case 2
     set @case_name="case2";
     select statement, result_count from statement_info where user="dump" and sql_source_type="cloud_user_sql" and status != 'Running' and statement like '%bvt_result_count_test_case2%' and aggr_count < 1 order by request_at desc limit 2;
     
    Suggestion importance[1-10]: 6

    Why: Adding comments improves code readability and maintainability, which is beneficial for understanding the purpose of specific operations.

    6

    @xzxiong xzxiong changed the title chore: stable bvt/result_count.sql result check in mo1.2 chore: stable bvt/result_count.sql result check active load_data_LOCAL_infile case in mo1.2 May 29, 2024
    @xzxiong xzxiong changed the title chore: stable bvt/result_count.sql result check active load_data_LOCAL_infile case in mo1.2 chore: stable bvt/result_count.sql result check and enable load_data_LOCAL_infile case in mo1.2 May 29, 2024
    @mergify mergify bot merged commit 4dc7f89 into matrixorigin:1.2-dev May 30, 2024
    17 of 18 checks passed
    @aylei aylei mentioned this pull request Jun 5, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix kind/bug Something isn't working kind/test-ci Review effort [1-5]: 2 size/M Denotes a PR that changes [100,499] lines Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants