utest/msh: supports autocomplete of utest cases for utest_run #10701
Conversation
📌 Code Review Assignment🏷️ Tag: componentsReviewers: Maihuanyi Changed Files (Click to expand)
🏷️ Tag: documentationReviewers: CXSforHPU GorrayLi lianux-mm unicornx Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-09-19 16:20 CST)
📝 Review Instructions
|
kurisaW
force-pushed
the
utest_cmd
branch
2 times, most recently
from
ff92a41 to
a1836d1
Compare
|
我建议是拆分成两个commit，一个是格式化，一个是新增功能，这样好review下 |
已按要求修改 |
|
LGTM |
There was a problem hiding this comment.
Pull Request Overview
This PR adds autocomplete functionality for utest cases when using the utest_run command in the MSH shell. It allows users to see available test cases and complete test case names with tab completion.
Key changes:
- Adds API functions to retrieve test case count and names from the utest framework
- Implements autocomplete logic for the
utest_runcommand with tabular display of matching test cases - Updates code formatting to improve consistency with RT-Thread coding standards
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| components/utilities/utest/utest.h | Adds function declarations for getting test case count and names, updates copyright year |
| components/utilities/utest/utest.c | Implements new API functions and fixes pointer initialization logic in utest_init |
| components/finsh/msh.c | Adds utest autocomplete functionality and improves code formatting consistency |
There was a problem hiding this comment.
先提三个 general 的问题
问题1：
建议 msh_utest_list_complete 这个函数应该放到 utest 模块中而不是放在 msh 中，对于 msh 来说，实际上不应该看到 utest 的内部太多逻辑，否则如果按照这个设计思路做下去如果 msh 中要加入更多的其他模块中类似 utest 这样的需求，那 msh 中将变成一个六边形战士了。
将 msh_utest_list_complete 的实现移到 utest 中后，utest 也不需要暴露 utest_get_tc_num 和 utest_get_tc_name 这些借口和概念出来，msh 不应该关心 utest 中的内部细节。
更好的做法应该尝试将目前 msh 调用 utest 的 msh_utest_list_complete 做成一种 callback 方式就更好了，utest 在初始化时将 msh_utest_list_complete 作为回调函数注册到 msh 中，msh 在恰当的时间点上回调这些 callback（可能来自 utest 也可能来自其他的模块）。
更细节来说 utest 这种外部模块在注册时其实可以注册两个关键数据，一个是需要自动匹配的命令名字，譬如 "utest_run", 另一个是这个名字的自动完成回调，即 msh_utest_list_complete。而对于 msh 来说，在目前调用 msh_utest_list_complete 的地方实际上可以扩展为检查一个注册的表，并将当前的 prefix 和需要进一步 autocomplete 的注册的命令名字进行匹配，匹配成功则调用相应的回调即可。只不过目前的需要扩展的模块只有一个 utest，可以按照现在的简单做法用 if 匹配而已。
问题2：
建议代码同时，请完善一下文档，可以在 documentation/6.components/utest/utest.md 中的 "Running Test Cases" 部分添加对 TAB 自动补丁的描述。
问题3：
如果要分 commit 提交，建议先 format 再改逻辑，另外 format 部分最好是分成两个 format，一个是 msh 模块，一个是 utest 模块，这样会比较清晰。
问题4：
format 的 commit 的 subject 过于简单了，至少要有模块的前缀，譬如： "msh: format code"
其他
代码细节问题我看 Copilot 已经提了不少，请先看看，上面几个问题确认好了我再仔细看你的代码修改吧。
|
@unicornx 对于第一个问题，我看了下RT-Thread Finsh组件似乎并不支持这种两个函数的回调吧，因为是要支持执行utest_run这个函数时自动补齐（这里执行msh_utest_list_complete()），我注意到msh是支持msh_auto_complete的，但是像这块各模块也是单拎出来去实现对应的自动补齐逻辑，例如posix file：msh_auto_complete_path 如果要支持自动补齐，目前来看最好的方案就是集成在msh中了，也就是我目前的方案 如果有可行的方案，能否给出更加具体的说明及参考？ |
这个问题我们不争论了吧。如果 msh 有原作者或者 maintainer 可以让他发表一下意见。 其他问题你看看能改了就行，其他我没意见了。 |
这么修改是有些麻烦了，这个PR实现了针对MSH的自动补全子选项特性。可以使用这类接口去实现此功能，我这边针对utest.c稍做修改实现了补全功能，供作者参考： +#define MAX_UTEST_OPTIONS 128 /* 支持更多测试用例，可根据需要调整 */
+static struct msh_cmd_opt utest_testcase_run_msh_options[MAX_UTEST_OPTIONS];
+
+static void utest_build_options(void)
+{
+ rt_size_t i;
+ rt_size_t option_index = 0;
+
+ if (tc_num >= MAX_UTEST_OPTIONS - 1)
+ {
+ LOG_W("Too many test cases (%d), only first %d will have completion", tc_num, MAX_UTEST_OPTIONS - 1);
+ }
+
+ rt_memset(utest_testcase_run_msh_options, 0, sizeof(utest_testcase_run_msh_options));
+
+ rt_size_t max_cases = (tc_num < MAX_UTEST_OPTIONS - 1) ? tc_num : MAX_UTEST_OPTIONS - 1;
+ for (i = 0; i < max_cases; i++)
+ {
+ utest_testcase_run_msh_options[option_index].id = i + 1;
+ utest_testcase_run_msh_options[option_index].name = tc_table[i].name;
+ utest_testcase_run_msh_options[option_index].des = RT_NULL;
+ option_index++;
+ }
+
+ utest_testcase_run_msh_options[option_index].id = 0;
+ utest_testcase_run_msh_options[option_index].name = RT_NULL;
+ utest_testcase_run_msh_options[option_index].des = RT_NULL;
+}
+
+static void utest_build_options(void);
+
int utest_init(void)
{
/* initialize the utest commands table.*/
@@ -123,6 +154,9 @@ int utest_init(void)
LOG_E("no memory, tc_fail_list init failed!");
}
}
+
+ utest_build_options();
+
return tc_num;
}
INIT_COMPONENT_EXPORT(utest_init);
@@ -335,6 +369,7 @@ INIT_APP_EXPORT(utest_auto_run);
int utest_testcase_run(int argc, char** argv)
{
static char utest_name[UTEST_NAME_MAX_LEN];
+ int opt_id;
rt_memset(utest_name, 0x0, sizeof(utest_name));
tc_loop = 1;
@@ -343,32 +378,47 @@ int utest_testcase_run(int argc, char** argv)
{
utest_thread_create(RT_NULL);
}
- else if (argc == 2 || argc == 3 || argc == 4)
+ else if (argc >= 2)
{
- if (rt_strcmp(argv[1], "-thread") == 0)
+ opt_id = msh_cmd_opt_id_get(argc, argv, (void*)utest_testcase_run_msh_options);
+
+ if (opt_id >= 1 && opt_id <= tc_num)
{
- if (argc == 3 || argc == 4)
+ rt_size_t test_index = opt_id - 1;
+ rt_strncpy(utest_name, tc_table[test_index].name, sizeof(utest_name) -1);
+ if (argc == 3)
{
- rt_strncpy(utest_name, argv[2], sizeof(utest_name) -1);
- if (argc == 4)
- {
- tc_loop = atoi(argv[3]);
- }
+ tc_loop = atoi(argv[2]);
}
- utest_thread_create(utest_name);
- }
- else if (rt_strcmp(argv[1], "-help") == 0)
- {
- utest_help();
+ utest_do_run(utest_name);
}
else
{
- rt_strncpy(utest_name, argv[1], sizeof(utest_name) -1);
- if (argc == 3)
+ if (rt_strcmp(argv[1], "-thread") == 0)
{
- tc_loop = atoi(argv[2]);
+ if (argc == 3 || argc == 4)
+ {
+ rt_strncpy(utest_name, argv[2], sizeof(utest_name) -1);
+ if (argc == 4)
+ {
+ tc_loop = atoi(argv[3]);
+ }
+ }
+ utest_thread_create(utest_name);
+ }
+ else if (rt_strcmp(argv[1], "-help") == 0)
+ {
+ utest_help();
+ }
+ else
+ {
+ rt_strncpy(utest_name, argv[1], sizeof(utest_name) -1);
+ if (argc == 3)
+ {
+ tc_loop = atoi(argv[2]);
+ }
+ utest_do_run(utest_name);
}
- utest_do_run(utest_name);
}
}
else
@@ -379,7 +429,7 @@ int utest_testcase_run(int argc, char** argv)
return RT_EOK;
}
-MSH_CMD_EXPORT_ALIAS(utest_testcase_run, utest_run, utest_run [-thread or -help] [testcase name] [loop num]);
+MSH_CMD_EXPORT_ALIAS(utest_testcase_run, utest_run, utest_run [-thread or -help] [testcase name] [loop num], optenable);
|
|
看起来这个方法可行，原本是有这么个接口的 |
components/utilities/utest/utest.c
Outdated
| } | ||
| } | ||
|
|
||
| static struct msh_cmd_opt utest_testcase_run_msh_options[MAX_UTEST_OPTIONS]; |
There was a problem hiding this comment.
这个限制case 数量的做法感觉扩展性不好，而且不好操作，容易引入问题，一个原因是 utest cases 的数量以后可能会膨胀的厉害；而来用户实际操作时很难准确估算出实际的个数。
所以我建议用堆 malloc 出来这个utest_testcase_run_msh_options，因为此时我们是知道 cases 的实际大小的。至于释放，我觉得应该也不用释放的。
There was a problem hiding this comment.
动态的应该不行，因为使用MSH_CMD_EXPORT_ALIAS 中的 optenable 参数需要编译时常量作为选项参数。
There was a problem hiding this comment.
宏展开是这样的：
static struct msh_cmd_opt utest_testcase_run_msh_options[MAX_UTEST_OPTIONS];
const char __fsym_utest_run_name[] __attribute__((section(".rodata.name"))) = "utest_run";
const char __fsym_utest_run_desc[] __attribute__((section(".rodata.name"))) = "utest_run [-thread or -help] [testcase name] [loop num]";
__attribute__((used)) const struct finsh_syscall __fsym_utest_run __attribute__((section("FSymTab")))=
{
__fsym_utest_run_name,
__fsym_utest_run_desc,
utest_testcase_run_msh_options,
(syscall_func)&utest_testcase_run
};There was a problem hiding this comment.
哦，那看上去 malloc 的确不行。
如果一定要上这个功能的话，我觉得按照目前的做法，有几点需要注意一下：
- 如果初始化过程中出现实际 tc_num 超过了我们分配的 MAX_UTEST_OPTIONS，目前我看代码只是告警，但我建议直接退出并告警提示用户重新配置 MAX_UTEST_OPTIONS 为实际 tc_num 的个数 + 1。如果只是处理一部分感觉这个feature 做了也是鸡肋。
- MAX_UTEST_OPTIONS 这个宏名称建议按规范，譬如加上前缀
RT_UTEST_MAXNUM_CASES. - 目前默认值太小，目前 RTT 源码中 UTEST_TC_EXPORT 的示例搜索一下至少 159 个，我建议翻倍 320 个至少。因为一旦
RT_UTEST_USING_ALL_CASES打开很容易超过 64。
There was a problem hiding this comment.
64的大小应该都算多了，正常来说，用户一次性启用的测试用例本身也不会很多（这对utest线程栈大小也有很高的要求），并且已经预留了打印提示
MAX_UTEST_OPTIONS 已修改为RT_UTEST_MAX_OPTIONS
如果原来有机制支持，那最好了。这样改动只在 utest 可见也是符合我的最初想法的。 |
components/utilities/utest/utest.c
Outdated
| else if (argc >= 2) | ||
| { | ||
| if (rt_strcmp(argv[1], "-thread") == 0) | ||
| opt_id = msh_cmd_opt_id_get(argc, argv, (void*)utest_testcase_run_msh_options); | ||
|
|
||
| if (opt_id >= 1 && opt_id <= tc_num) | ||
| { | ||
| if (argc == 3 || argc == 4) | ||
| rt_size_t test_index = opt_id - 1; | ||
| rt_strncpy(utest_name, tc_table[test_index].name, sizeof(utest_name) -1); | ||
| if (argc == 3) | ||
| { | ||
| rt_strncpy(utest_name, argv[2], sizeof(utest_name) -1); | ||
| if (argc == 4) | ||
| { | ||
| tc_loop = atoi(argv[3]); | ||
| } | ||
| tc_loop = atoi(argv[2]); | ||
| } | ||
| utest_thread_create(utest_name); | ||
| } | ||
| else if (rt_strcmp(argv[1], "-help") == 0) | ||
| { | ||
| utest_help(); | ||
| utest_do_run(utest_name); | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
[nitpick] Complex control flow with nested conditions / 复杂的控制流嵌套条件. The new autocomplete logic is embedded within the existing parameter parsing, making the function harder to follow. Consider extracting the autocomplete handling into a separate function or restructuring the condition hierarchy for better readability. / 新的自动完成逻辑嵌入在现有的参数解析中，使函数更难理解。建议将自动完成处理提取到单独的函数中，或重构条件层次结构以提高可读性。
Copilot uses AI. Check for mistakes.
components/utilities/utest/utest.c
Outdated
| { | ||
| if (argc == 3 || argc == 4) | ||
| rt_size_t test_index = opt_id - 1; | ||
| rt_strncpy(utest_name, tc_table[test_index].name, sizeof(utest_name) -1); |
There was a problem hiding this comment.
Missing null termination in string copy / 字符串复制缺少空终止符. The rt_strncpy call doesn't guarantee null termination if the source string is exactly sizeof(utest_name) - 1 characters long. Should null-terminate the destination buffer explicitly: utest_name[sizeof(utest_name) - 1] = '\0'; / rt_strncpy 调用不能保证如果源字符串恰好是 sizeof(utest_name) - 1 个字符长时的空终止。应该显式地空终止目标缓冲区：utest_name[sizeof(utest_name) - 1] = '\0';
Copilot uses AI. Check for mistakes.
|
另外CI问题：
|
这么怎么说？ |
|
这部分超出带来的问题，其实也就是超出部分的utest没法显示而已，然后提供警告提示，这个处理应该是合理的吧 |
或许可以写一个脚本，在链接后获取并统计下 map 文件中已经注册进 ut 的列表，然后生成对应 MAX_UTEST_OPTIONS 的宏配置 |
这个搞复杂了，难道要编译两遍？没必要吧。 我之所以提出用告警还是出错退出的考虑主要是觉得如果只是告警，万一用户没注意继续使用，会产生很奇怪的感觉，因为明明配置里enable 了某个测试项，但是 命令行里 tab 后看不到，徒增烦恼。 如果维持告警不退出，还有一种解决思路就是一旦发现case 个数超出了我们的配置值，那在每次 tab 自动补齐过程中，至少要有所提示，譬如显示 ”......." 这种，如果没有超出则正常显示。 大家可以从从用户体验角度考虑一下看看还有什么好的处理方式？ |
后面这种也行，可以在启动 ut 的时候动态的判断下 ut 数量和 MAX_UTEST_OPTIONS 进行比较，然后告警下用户需要增加数组长度 |
@kurisaW 这个还是抽空修改下吧 |
📌 Code Review Assignment🏷️ Tag: componentsReviewers: Maihuanyi Changed Files (Click to expand)
🏷️ Tag: documentationReviewers: CXSforHPU GorrayLi lianux-mm unicornx Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-09-25 14:59 CST)
📝 Review Instructions
|
拉取/合并请求描述：(PR description)
[
支持utest_run自动补齐测试用例，下面是一个运行截图：
为什么提交这份PR (why to submit this PR)
你的解决方案是什么 (what is your solution)
请提供验证的bsp和config (provide the config and bsp)
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up