test: test start taosBenchmark #33390
Conversation
Summary of ChangesHello @menshibin, 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 focuses on improving the reliability and robustness of a specific benchmark test case. The core change involves migrating the external command execution from the less controlled Highlights
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
|
![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 refactors the benchmark test to use subprocess.run instead of os.system, which is a good improvement for security and process control. It also adds a loop to run the test multiple times. My review includes suggestions to improve the test's effectiveness by making each loop iteration unique, to enhance code consistency by translating Chinese comments and log messages to English, and to reduce log noise.
| self.executeAndCheck('stmt2_tag_order_1', 'stmt2', 1) | ||
| self.executeAndCheck('stmt2_tag_order_2', 'stmt2', 0) |
There was a problem hiding this comment.
These calls use hardcoded database names inside a loop. This means the exact same test is run 99 times, which is redundant. To make this stability test more effective, use the loop variable i to create unique database names for each iteration. This will better test for state leakage between runs.
| self.executeAndCheck('stmt2_tag_order_1', 'stmt2', 1) | |
| self.executeAndCheck('stmt2_tag_order_2', 'stmt2', 0) | |
| self.executeAndCheck(f'stmt2_tag_order_1_{i}', 'stmt2', 1) | |
| self.executeAndCheck(f'stmt2_tag_order_2_{i}', 'stmt2', 0) |
| try: | ||
| result = subprocess.run( | ||
| cmd, | ||
| capture_output=True, # 捕获输出 | ||
| text=True, # 文本模式 | ||
| timeout=300, # 设置超时 | ||
| check=True # 非零返回码时抛异常 | ||
| ) | ||
| tdLog.info(f"Benchmark执行成功: {result.stdout}") | ||
| self.checkDataCorrect(dbname, 1000, 100) | ||
| tdSql.execute(f"drop database {dbname};") | ||
| except subprocess.CalledProcessError as e: | ||
| tdLog.error(f"Benchmark执行失败，返回码: {e.returncode}") | ||
| tdLog.error(f"错误输出: {e.stderr}") | ||
| raise | ||
| except subprocess.TimeoutExpired: | ||
| tdLog.error("Benchmark执行超时") | ||
| raise | ||
| except Exception as e: | ||
| tdLog.error(f"执行过程中发生异常: {str(e)}") | ||
| raise |
There was a problem hiding this comment.
The inline comments and log messages in this try...except block are in Chinese, while the rest of the codebase is in English. To maintain consistency and improve readability for all contributors, please translate these to English.
try:
result = subprocess.run(
cmd,
capture_output=True, # Capture output
text=True, # Text mode
timeout=300, # Set timeout
check=True # Raise exception on non-zero return code
)
tdLog.info(f"Benchmark executed successfully: {result.stdout}")
self.checkDataCorrect(dbname, 1000, 100)
tdSql.execute(f"drop database {dbname};")
except subprocess.CalledProcessError as e:
tdLog.error(f"Benchmark execution failed with return code: {e.returncode}")
tdLog.error(f"Error output: {e.stderr}")
raise
except subprocess.TimeoutExpired:
tdLog.error("Benchmark execution timed out")
raise
except Exception as e:
tdLog.error(f"An exception occurred during execution: {str(e)}")
raise| for i in range(1, 100): | ||
| self.executeAndCheck('stmt2_tag_order_1', 'stmt2', 1) | ||
| self.executeAndCheck('stmt2_tag_order_2', 'stmt2', 0) | ||
| tdLog.success("Successfully executed") |
There was a problem hiding this comment.
Logging a success message on every iteration creates a lot of log noise. It would be better to move this log message outside the loop to be printed only once after all iterations complete successfully. This makes the test logs cleaner and easier to read.
Description
Please briefly describe the code changes in this pull request.
Checklist
Please check the items in the checklist if applicable.