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:merge case folder to the new mm folder #2965

Merged
merged 1 commit into from
Jan 23, 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 mm folder and move folders to the new mm folder.

Impact

Only affect apps/testing. The changes are as follows:

1.rename original mm folder to mmtools and move it to the new mm folder.

2.move the following folders into the new mm folder:
cachetest, iob, kasantest, memstress, memtester, open-memstream, ramtest, stressapptest

Testing

CI test.
depends on apache/nuttx#15659

@nuttxpr
Copy link

nuttxpr commented Jan 22, 2025

[Experimental Bot, please feedback here]

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 (The placement of cases under testing is a bit messy, and cases of the same type need to be merged #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.

testing/mm/open_memstream/Make.defs Outdated Show resolved Hide resolved
testing/mm/resmonitor/Make.defs Outdated Show resolved Hide resolved
testing/mm/mmtool/mm_main.c Outdated Show resolved Hide resolved
testing/mm/mmtool/Kconfig Outdated Show resolved Hide resolved
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 :-) Here goes my conditional go assuming all @xiaoxiang781216 remarks are adressed :-)

@txy-21 txy-21 force-pushed the merge-mm-folder branch 5 times, most recently from e39c35f to e1eabc3 Compare January 23, 2025 06:07
@xiaoxiang781216
Copy link
Contributor

@txy-21 please update nuttx board's defconfig too

1.rename original  mm folder to heaptest and move it to mm folder

2.move the following folders into the new mm folder:
  cachetest, heaptest, iob, kasantest, memstress, memtester, ramtest, stressapptest

Signed-off-by: tengshuangshuang <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 077c346 into apache:master Jan 23, 2025
10 of 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