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

apps/testing:move arch_libc, fmemopen, scanftest folders to the new libc folder #2964

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

txy-21
Copy link
Contributor

@txy-21 txy-21 commented Jan 22, 2025

Summary

This is one step that merge test case as discussed in #2931.
Create libc folder and move arch_libc, fmemopen, scanftest folders to the new libc folder.

Impact

Only affect apps/testing folder

New folder :
libc

Move the following folder:
arch_libc, fmemopen, scanftest

Testing

CI test

@nuttxpr
Copy link

nuttxpr commented Jan 22, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, though it could be more thorough. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the "why" (part of a larger effort), "what" (moving test folders), and "how" (creating a new libc folder). The link to the related PR is excellent.
  • Impact Description: Clearly states the limited impact to the apps/testing folder. Lists the new and moved folders.
  • Testing Mention: Mentions CI testing.

Weaknesses:

  • Missing Detail in Impact: While it mentions affected folders, it doesn't discuss why this change is being made. What benefit does restructuring bring? This helps reviewers understand the motivation. Also, explicitly stating "NO" for the other impact categories would be clearer.
  • Insufficient Testing Detail: Simply saying "CI test" isn't enough. Which CI targets? Were any manual tests performed? Providing snippets of successful test output (even if just "pass") would strengthen the testing section significantly. The template requests "before" and "after" logs, which are missing. While not strictly required for pure restructuring, some evidence of functionality not being broken is good practice.
  • Missing Build Host Information: The template asks for build host details (OS, compiler, etc.). This is missing.

Recommendation:

While the PR seems to meet the letter of the requirements, adding the missing details, especially regarding testing and build environment, would significantly improve its quality and make review easier. Even a simple "Built and tested on Linux with GCC 12.x, CI passed on sim:nsh" would be a substantial improvement. Explaining the rationale behind the restructuring would also be beneficial.

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @txy-21 :-)

@xiaoxiang781216 xiaoxiang781216 merged commit 57c8a62 into apache:master Jan 22, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants