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 epoll, fatutf8 ... folders to the new fs folder #2963

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

txy-21
Copy link
Contributor

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

Summary

This is one step that merge test case as discussed in #2931.
Create fs folder and move {epoll, fatutf8, fasantest, fopencookie, fstest, mtd_config_fs, nxffs, smart, smart_test} folders to the new fs folder.

Impact

Only affect apps/testing folder

New folder :
fs

Move the following folder:
epoll
fatutf8
fasantest
fopencookie
fstest
mtd_config_fs
nxffs
smart
smart_test

Testing

CI test

Copy link

@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 :-)

@nuttxpr
Copy link

nuttxpr commented Jan 21, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. While it provides a summary and impact assessment, it lacks crucial information.

Here's what's missing:

  • Summary: Lacks clarity on why this change is necessary. Simply stating it's part of a larger effort isn't sufficient. What problem does this restructuring solve? How does it benefit the project? What functionality is being changed (even if it's just directory structure)? How exactly does the move work (e.g., are symbolic links used? Is it a pure move?). The linked PR provides context, but the PR itself should be self-contained.

  • Impact: While it mentions the affected folders, it lacks crucial "YES/NO" answers for most impact categories. Even if the answer is "NO," it should be explicitly stated. For example:

    • Impact on user: NO
    • Impact on build: Potentially YES (if any build scripts rely on the old paths). This needs clarification.
    • Impact on hardware: NO
    • Impact on documentation: Potentially YES (if any documentation refers to the old paths). This needs investigation and explicit statement.
    • Impact on security: NO
    • Impact on compatibility: Potentially YES. This change could break existing user setups or scripts that rely on the old paths. This needs investigation.
  • Testing: "CI test" is insufficient. While CI is important, the PR should provide specific examples of tests run locally to validate the change. What were the results before and after the move? Show concrete evidence that the restructuring didn't break anything. Including the build host and target information is also necessary.

In short, the PR needs to be more thorough and explicit in addressing all the requirements. It relies too heavily on the linked PR for context. Each PR should stand on its own.

@xiaoxiang781216 xiaoxiang781216 merged commit 8141e35 into apache:master Jan 21, 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