-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[add][testcase]add memory pool and file system testcases. #10670
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
Conversation
📌 Code Review Assignment🏷️ Tag: componentsReviewers: Maihuanyi Changed Files (Click to expand)
🏷️ Tag: workflowReviewers: Rbb666 kurisaW supperthomas Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-09-06 13:50 CST)
📝 Review Instructions
|
a9fb6be
to
6b650e9
Compare
@unicornx 汪老师可以帮忙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.
除了一些针对代码的 comments 外,还有以下哎general 的意见。
-
我只看了 DFS 的部分,因为 DFS 部分是新增的,而且使用了新的 utest 组织方式。而且我主要 review 了新增部分的组织方式,对于实际的 DFS 测试内容,我建议请 DFS 的 maintainer review,这也是我们引入 maintainer 机制以及 utestcase 按照模块组织的最终目的。
-
对于 DFS 的 utestcase 的组织,请再读一下 How-to add utest cases into RT-Thread for your module.. 目前看上去存在以下问题:
- 没有遵循配置项命名规范:RT_UTEST_TC_USING_XXXX
- 缺少对 RT_UTEST_USING_ALL_CASES 的依赖
详细的例子可以参考一下:https://github.com/RT-Thread/rt-thread/blob/master/src/klibc/utest/ 下的代码。
-
建议将这个 pr 拆分成两个,或者在一个 pr 中作为两个 commit 分开提交,因为改动明显涉及两个 模块,一个是 DFS,一个是 core 部分的 mempool。
components/dfs/utest/Kconfig
Outdated
string "Filesystem type for test" | ||
default "elm" | ||
help | ||
The filesystem type (e.g., elm, fat) |
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.
help 部分应该和 RT_UTEST_DFS_MNT_PATH 一样说清楚这个配置项是为了 filesystem test 而用的。而且最好写成:Configure the xxxx which will be used in unit test. 不要一上来给个名字解释,但用户不知道这是干什么的。我们其实最好说清楚这是干什么的。
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.
已修改
|
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.
仅 review 了 DFS 部分的和 utest 框架相关部分。具体的测试用例建议请 DFS 的 maintainer (如果有的话)检查。
拉取/合并请求描述:(PR description)
[
为什么提交这份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