nuttxpr commented on PR #2965: URL: https://github.com/apache/nuttx-apps/pull/2965#issuecomment-2606104440
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not fully meet the NuttX requirements. Here's why and how to fix it: **Missing Information in Summary:** * **Why is this change necessary?** The PR mentions a discussion in another PR (#2931), but doesn't explain the underlying reason for consolidating these tests. What problem does this solve or what improvement does it offer? Is it for organization, maintainability, or something else? This needs to be clearly stated. * **What functional part of the code is being changed?** While it mentions `apps/testing`, it should be more specific. It's changing the directory structure and organization of the memory management tests. * **How does the change *exactly* work?** While the steps are listed, it lacks detail. Does this involve any code changes beyond moving files and folders? Were any build scripts modified? **Missing Information in Impact:** * While it mentions affecting `apps/testing`, it needs to explicitly answer the YES/NO questions for *all* impact categories. Even if the answer is NO, state it explicitly. For example: * Impact on user: NO * Impact on build: YES (directory structure changed, build scripts may need updating) * Impact on hardware: NO * Impact on documentation: Possibly YES (if any documentation refers to the old paths, it needs updating) * Impact on security: NO * Impact on compatibility: Potentially YES (scripts or other tools relying on the old paths may break). * Justify the answers. Don't just say YES or NO. If there's no impact, briefly explain why. **Missing Information in Testing:** * **Insufficient Testing Detail:** "CI test" is not enough. Which CI tests were run? Provide links to the CI runs. * **Missing Logs:** The template specifically requests "Testing logs before change" and "Testing logs after change." Even if the changes are purely structural, provide some evidence that the tests still run and pass after the reorganization. This could be a snippet of the CI output showing successful execution of the moved tests. * **Build Host and Target Details:** Specify the build host OS, CPU, compiler, and target architecture/board used for testing. **Example of an Improved PR Description:** ``` ## Summary This PR reorganizes the memory management tests within `apps/testing` to improve organization and maintainability, as discussed in https://github.com/apache/nuttx-apps/pull/2931. This consolidation makes it easier to locate and manage memory-related tests. Specifically, a new `mm` directory is created within `apps/testing`. The existing `mm` directory is renamed to `mmtools` and moved into the new `mm` directory. Additionally, the following test directories are moved into the new `mm` directory: `cachetest`, `iob`, `kasantest`, `memstress`, `memtester`, `open-memstream`, `ramtest`, and `stressapptest`. The `apps/testing/Make.defs` file is updated to reflect the new directory structure. ## Impact * Is new feature added? NO * Is existing feature changed? NO (Functionality remains the same, only organization is changed) * Impact on user: NO (Users running these tests will need to update their paths if they were directly accessing them, but the standard testing framework should handle this automatically). * Impact on build: YES (The directory structure and `apps/testing/Make.defs` have been modified). * Impact on hardware: NO * Impact on documentation: YES (Documentation referencing the old test paths needs to be updated. This update is included in this PR.) * Impact on security: NO * Impact on compatibility: YES (Scripts or tools relying on the old directory structure may break. We will address these compatibilities issues in subsequent PRs as needed). * Anything else to consider: N/A ## Testing I confirm that changes are verified on a local setup and within CI. * Build Host: Linux (Ubuntu 20.04), x86_64, GCC 9.4.0 * Target: qemu-x86_64 CI Test Run: [Link to CI Run] Testing logs before change (Example - show a successful test run before the move): ``` ... output showing successful test run from apps/testing/mm/memtester ... ``` Testing logs after change (Example - show the same test now running successfully from the new location): ``` ... output showing successful test run from apps/testing/mm/memtester ... ``` ``` By providing more detail and explicitly addressing all the points in the template, the PR will be much clearer and easier for reviewers to evaluate. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
