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 atomic,cpuload,getprime,smp and timerjitter folders to the new sched folder #2974

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

txy-21
Copy link
Contributor

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

Summary

This is one step that merge test case as discussed in #2931.
Create new sched folder, move atomic,cpuload,getprime,smp and timerjitter folders to the new sched folder

Impact

This PR only affects the apps/testing folder, changing the location of some subfolders under this folder.
new folder : sched
move : atomic,cpuload,getprime,smp and timerjitter

Testing

CI test.

@nuttxpr
Copy link

nuttxpr commented Jan 24, 2025

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. While it provides a summary and mentions testing, it lacks crucial details. Here's a breakdown of what's missing and how to improve it:

Summary:

  • Missing "Why": The summary states what the change is but not why. Is this for better organization? Does it fix a problem or enable something new? Explain the rationale behind grouping these specific tests under "sched".
  • Missing "How": The summary mentions moving folders, but not how this was done. Were files simply moved? Were any build scripts or configurations modified?

Impact:

While the impact section addresses some points, it lacks detail and omits others:

  • Impact on user: While the user is likely a developer, the impact should still be described. Will any existing test scripts need to be updated to reflect the new paths?
  • Impact on build: The impact on the build process should be clarified. Does simply moving the folders affect anything in the build system (e.g., Makefiles, CMakeLists.txt)? Even if the answer is NO, explicitly state it and briefly explain why (e.g., "No, because the build system uses relative paths").
  • Impact on hardware: This is likely NO, but should be explicitly stated.
  • Impact on documentation: If the move requires any documentation updates (e.g., tutorials, READMEs), mention them here. If no updates are needed, state that explicitly.
  • Impact on security: This is likely NO, but should be explicitly stated.
  • Impact on compatibility: This is likely NO, but should be explicitly stated.

Testing:

  • Insufficient Detail: "CI test" is not enough. Specify the CI environment (e.g., GitHub Actions, Travis CI). Which targets were tested? Were all tests under these moved folders run successfully?
  • Missing Logs (or explanation): The template requests "Testing logs before change" and "Testing logs after change". Since this is a structural change, full logs might be excessive. However, you should at least explain what was tested and how success was determined. For example: "All tests under atomic, cpuload, getprime, smp, and timerjitter were run successfully on the qemu-x86_64 target before and after the folder move, confirming that the relocation did not break the tests."

Example of an improved PR description:

## Summary

This change moves several test folders related to scheduling concepts (atomic operations, CPU load, prime number calculation, SMP, and timer jitter) into a new "sched" subdirectory within the `apps/testing` folder. This improves organization and makes it easier to locate tests related to scheduling functionality, as discussed in https://github.com/apache/nuttx-apps/pull/2931.  The folders were moved using standard file system commands, and build scripts were updated to reflect the new paths.

## Impact

* **Impact on user:** YES. Developers running these tests will need to update their scripts to reflect the new paths.
* **Impact on build:** YES. Makefiles within the `apps/testing` directory were updated to reflect the new folder structure.
* **Impact on hardware:** NO.  This change is purely organizational within the test suite.
* **Impact on documentation:** YES. The README file for the `apps/testing` directory will be updated to reflect the new folder structure.
* **Impact on security:** NO. This change has no impact on security.
* **Impact on compatibility:** NO.  This change maintains backward compatibility.  Existing test scripts can be easily updated to accommodate the new paths.


## Testing

I confirm that changes are verified on a local setup and work as intended:

* Build Host: Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
* Target: qemu-x86_64

Testing Description:

All tests under the `atomic`, `cpuload`, `getprime`, `smp`, and `timerjitter` folders were executed successfully on the `qemu-x86_64` target both before and after moving the folders.  This confirms that the tests remain functional after the reorganization.  Specific test logs are omitted as they are extensive and unchanged functionally.  The CI system will also run these tests to ensure broader platform coverage.

This improved description provides a much clearer and more complete picture of the changes and their impact, fulfilling the requirements of a NuttX PR. Remember to tailor the specifics to your actual changes and testing.

testing/sched/atomic/Makefile Outdated Show resolved Hide resolved
testing/sched/cpuload/Make.defs Outdated Show resolved Hide resolved
@cederom
Copy link

cederom commented Jan 24, 2025

Thank you @txy-21 :-) Would you like to move selected applications to apps/system right now or you have patch set ready and prefer to move them after apps/testing is reorganized based on remarks in PRs? :-)

… to the new sched folder

Signed-off-by: tengshuangshuang <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 0406e67 into apache:master Jan 25, 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.

5 participants