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

Test: add mysql ddl compability test #11424

Merged
merged 10 commits into from Aug 24, 2023
Merged

Conversation

aronchanisme
Copy link
Contributor

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #
#4374

What this PR does / why we need it:

Test: add mysql ddl compability test
case1: mysql_ddl_1.sql, covers comments, set variables, etc
case2: mysql_ddl_2.sql, covers engine, char set, key using, lock/unlock, auto_increment, row_format, etc
case3: mysql_ddl_3.sql, covers create view with definer, partition type and long sql definition, enum type, unused function, like "CONVERT , numeric type, etc
case4: mysql_ddl_4.sql, covers create view within a comment sql

@CLAassistant
Copy link

CLAassistant commented Aug 23, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ heni02
❌ aronchanisme
You have signed the CLA already but the status is still pending? Let us recheck it.

@matrix-meow matrix-meow added the size/XL Denotes a PR that changes [1000, 1999] lines label Aug 23, 2023
@mergify mergify bot added the kind/test-ci label Aug 23, 2023
@matrix-meow
Copy link
Contributor

@aronchanisme Thanks for your contributions!

Here are review comments for file test/distributed/cases/ddl/mysql_ddl_1.result:
Title: Test: add mysql ddl compatibility test

Body:

  • The PR is a Test and CI type.
  • It fixes issue [Feature Request]: DDL Transfer tool from MySQL to MO #4374.
  • The purpose of this PR is to add a mysql ddl compatibility test.
  • The test cases cover various aspects of mysql ddl, such as comments, set variables, engine, char set, key using, lock/unlock, auto_increment, row_format, create view with definer, partition type, long sql definition, enum type, unused function, and create view within a comment sql.

Changes made in the file test/distributed/cases/ddl/mysql_ddl_1.result:

  • The changes in this file seem to be the expected results of the test cases.
  • The diff shows the expected output of the test cases, including queries, comments, and variable values.

Overall, the pull request seems to add a comprehensive test suite for mysql ddl compatibility. However, there are a few areas that need addressing:

  1. Title: The title of the pull request could be more descriptive. Instead of just mentioning "Test," it could include a brief summary of what the test is for, such as "Add comprehensive mysql ddl compatibility test suite."

  2. Body: The body of the pull request is missing some important information. It would be helpful to include details about the specific mysql versions or features being tested, as well as any known limitations or issues with the test cases.

  3. Test Cases: The description of the test cases in the body is quite lengthy and could be condensed for better readability. It would be helpful to provide a high-level overview of the types of ddl operations being tested, rather than listing each individual case in detail.

  4. File Changes: The changes made in the file test/distributed/cases/ddl/mysql_ddl_1.result seem to be the expected results of the test cases. However, it would be beneficial to include comments or explanations for each change to provide more context for reviewers.

To optimize the changes made in the pull request, consider the following suggestions:

  1. Title: Update the title to be more descriptive, such as "Add comprehensive mysql ddl compatibility test suite for version X."

  2. Body: Provide a brief overview of the purpose of the test suite, the mysql versions or features being tested, and any known limitations or issues. This will help reviewers understand the scope and importance of the test suite.

  3. Test Cases: Condense the description of the test cases into a high-level overview, highlighting the main types of ddl operations being tested. This will make the description more concise and easier to understand.

  4. File Changes: Add comments or explanations for each change made in the file test/distributed/cases/ddl/mysql_ddl_1.result. This will provide more context for reviewers and make it easier to understand the expected results of the test cases.

By addressing these issues and optimizing the changes, the pull request will be more informative and easier to review, leading to a higher quality codebase.

Here are review comments for file test/distributed/cases/ddl/mysql_ddl_1.sql:
Review:

Title: Test: add mysql ddl compability test

The title of the pull request is clear and concise. It indicates that the changes being made are related to adding a test for MySQL DDL compatibility.

Body:

The body of the pull request provides some context and information about the changes being made. It mentions that the PR is a test and CI type, and it references an issue that this PR fixes. The body also provides a brief description of what the PR does, which is adding a MySQL DDL compatibility test. It further explains that the test covers various cases related to comments, set variables, engine, char set, key using, lock/unlock, auto_increment, row_format, create view with definer, partition type, long SQL definition, enum type, unused function, and create view within a comment SQL.

Changes:

The changes made in the pull request are focused on adding a new test file called mysql_ddl_1.sql under the test/distributed/cases/ddl directory. The file contains SQL statements that cover various scenarios related to MySQL DDL compatibility.

Suggestions for improvement:

  1. The body of the pull request could be more detailed and provide a clearer explanation of why this test is needed and how it will benefit the codebase. This will help reviewers understand the purpose and significance of the changes.

  2. The title of the pull request could be more specific by mentioning that it is adding a test for MySQL DDL compatibility for a specific feature or functionality.

  3. The body of the pull request could include a summary of the changes made in the mysql_ddl_1.sql file, highlighting the key scenarios being covered by the test.

  4. It would be helpful to include information about how the test cases were designed and what specific behaviors or functionalities they are testing.

  5. Consider providing more context about the issue being fixed and why it is important to address it.

Optimizations:

  1. Consider breaking down the changes into smaller, more focused commits. This will make it easier to review and understand the changes.

  2. Ensure that the test cases cover a wide range of scenarios and edge cases to thoroughly test the MySQL DDL compatibility.

  3. Consider adding comments within the test file to explain the purpose and expected outcome of each test case.

  4. Consider adding assertions or checks within the test cases to verify the correctness of the results.

Overall, the pull request is a good start in adding a test for MySQL DDL compatibility. However, there is room for improvement in terms of providing more detailed explanations and optimizing the test cases.

Here are review comments for file test/distributed/cases/ddl/mysql_ddl_2.result:

Review:

Title

The title of the pull request is clear and concise. It states that the pull request is adding a test for MySQL DDL compatibility.

Body

The body of the pull request provides some context and information about the changes made. It mentions that the pull request is related to issue #4374 and explains that the test cases cover various aspects of MySQL DDL compatibility. However, the body could be improved by providing more specific details about the test cases and their purpose.

Changes

The changes made in the pull request are focused on adding a new test case file (mysql_ddl_2.result) to the test/distributed/cases/ddl directory. The changes include the creation of a new database, the creation of multiple tables with different configurations, and the execution of show create table commands to verify the table definitions.

Problems

  1. Inconsistent naming: The file name mysql_ddl_2.result does not follow the naming convention used for other test case files in the same directory. It would be better to use a consistent naming convention to make it easier to identify and manage the test cases.

  2. Lack of comments: The changes made in the test case file lack comments to explain the purpose and expected outcome of each test case. Adding comments would make it easier for other developers to understand the intent of the test cases and would also serve as documentation for future reference.

  3. Redundant commands: The create database if not exists command is repeated for each table creation. It would be more efficient to move this command outside the loop and execute it only once.

  4. Inconsistent spacing: There are inconsistencies in the spacing used in the DEFAULT CHARSET and COLLATE clauses. Some have spaces before and after the equal sign, while others do not. It would be better to use consistent spacing for better readability.

Suggestions

  1. Rename the file mysql_ddl_2.result to follow the naming convention used for other test case files, such as mysql_ddl_2.sql.

  2. Add comments to each test case in the test case file to explain the purpose and expected outcome of the test.

  3. Move the create database if not exists command outside the loop and execute it only once.

  4. Use consistent spacing in the DEFAULT CHARSET and COLLATE clauses.

Optimization

To optimize the changes made in the pull request, consider the following suggestions:

  1. Combine similar test cases: Some of the test cases in the file have similar configurations, such as different CHARSET and COLLATE values. Instead of creating separate test cases for each combination, consider combining them into a single test case with multiple assertions.

  2. Use parameterized tests: Instead of hardcoding the table names and configurations in the test case file, consider using parameterized tests to generate multiple test cases with different configurations. This would make it easier to add new test cases and reduce code duplication.

  3. Use a test framework: Consider using a test framework, such as pytest or unittest, to organize and execute the test cases. This would provide better test reporting and make it easier to manage and maintain the test suite.

Here are review comments for file test/distributed/cases/ddl/mysql_ddl_2.sql:

Review:

Title

The title of the pull request is clear and concise. It accurately describes the purpose of the changes made in the pull request.

Body

The body of the pull request provides some context and explanation for the changes made. It includes a checklist for the type of pull request, a reference to the issue being fixed, and a description of what the pull request does and why it is needed. However, the body could be improved by providing more specific details about the changes made and their impact.

Changes

The changes made in the pull request are focused on adding a new test case for MySQL DDL compatibility. The changes include the addition of a new SQL file (mysql_ddl_2.sql) that contains a series of test cases for different MySQL DDL statements. The test cases cover various aspects of MySQL DDL, such as engine, character set, key using, lock/unlock, auto_increment, and row_format.

While the changes themselves seem to be fine, there are a few areas that could be improved:

  1. The comments in the SQL file (mysql_ddl_2.sql) are not consistent. Some comments start with -- testcase_2. while others do not have any prefix. It would be better to have consistent and descriptive comments for each test case.

  2. The database creation statement (create database if not exists mysql_ddl_test_db;) is not necessary for the test cases in this file. It can be removed to simplify the test cases.

  3. The show create table statements after each create table statement can be removed. They are not necessary for the test cases and add unnecessary clutter to the file.

  4. The LOCK TABLES and UNLOCK TABLES statements in the test case (mmysql_ddl_test_t210) are not necessary. They can be removed to simplify the test case.

  5. The test case (mmysql_ddl_test_t211) for AUTO_INCREMENT can be improved by adding more meaningful values for the id column.

  6. The test case (mmysql_ddl_test_t212) for AUTO_INCREMENT can be improved by using a different value for the initial AUTO_INCREMENT value.

  7. The test case (mmysql_ddl_test_t213) for AUTO_INCREMENT can be improved by using a different value for the initial AUTO_INCREMENT value.

  8. The test case (mmysql_ddl_test_t214) for ROW_FORMAT can be improved by using a different value for the ROW_FORMAT.

  9. The test case (mmysql_ddl_test_t215) for ROW_FORMAT can be improved by using a different value for the ROW_FORMAT.

  10. The test case (mmysql_ddl_test_t216) can be simplified by removing unnecessary options and using more meaningful values for the columns.

Overall, the changes made in the pull request seem to be fine, but there are some areas that could be improved for clarity and simplicity.

@aronchanisme
Copy link
Contributor Author

need to pass bvt tests first, fixing

@aronchanisme aronchanisme self-assigned this Aug 24, 2023
@mergify mergify bot merged commit b066547 into matrixorigin:main Aug 24, 2023
3 checks passed
sukki37 pushed a commit that referenced this pull request Aug 24, 2023
Test: add mysql ddl compability test
case1: mysql_ddl_1.sql, covers comments, set variables, etc
case2: mysql_ddl_2.sql, covers engine, char set, key using, lock/unlock, auto_increment, row_format, etc
case3: mysql_ddl_3.sql, covers create view with definer, partition type and long sql definition, enum type, unused function, like "CONVERT , numeric type, etc
case4: mysql_ddl_4.sql, covers create view within a comment sql

Approved by: @heni02
sukki37 pushed a commit that referenced this pull request Sep 11, 2023
Test: add mysql ddl compability test
case1: mysql_ddl_1.sql, covers comments, set variables, etc
case2: mysql_ddl_2.sql, covers engine, char set, key using, lock/unlock, auto_increment, row_format, etc
case3: mysql_ddl_3.sql, covers create view with definer, partition type and long sql definition, enum type, unused function, like "CONVERT , numeric type, etc
case4: mysql_ddl_4.sql, covers create view within a comment sql

Approved by: @heni02
sukki37 pushed a commit that referenced this pull request Sep 12, 2023
Test: add mysql ddl compability test
case1: mysql_ddl_1.sql, covers comments, set variables, etc
case2: mysql_ddl_2.sql, covers engine, char set, key using, lock/unlock, auto_increment, row_format, etc
case3: mysql_ddl_3.sql, covers create view with definer, partition type and long sql definition, enum type, unused function, like "CONVERT , numeric type, etc
case4: mysql_ddl_4.sql, covers create view within a comment sql

Approved by: @heni02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/test-ci size/XL Denotes a PR that changes [1000, 1999] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants