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

testing: move cmocka_fs to the new folder /basic #3011

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

txy-21
Copy link
Contributor

@txy-21 txy-21 commented Feb 26, 2025

Summary

testing: move cmocka_fs to the new folder /basic

Move the cmocka case folder under testsuites to testing/fs/basic

Impact

apps/testing/testsuites and apps/testing/fs/basic

Testing

ci test.

@nuttxpr
Copy link

nuttxpr commented Feb 26, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. While it provides some information, it lacks crucial details. Here's a breakdown:

  • Insufficient Summary: While it states what is changed, it doesn't explain why. What problem does moving cmocka_fs solve? What are the benefits? How does the move work (e.g., just moving files, or code changes within the tests themselves)? There's no mention of related issues.

  • Incomplete Impact Assessment: Simply listing affected directories isn't enough. Consider these questions:

    • New/Changed Feature: Is this just a reorganization or does it affect functionality?
    • User Impact: Likely no, but explicitly state it.
    • Build Impact: Will anything need to be changed in Makefiles or configuration?
    • Hardware Impact: Likely no, but explicitly state it.
    • Documentation Impact: Does this change require updating any documentation (e.g., test suite organization)?
    • Security Impact: Likely no, but explicitly state it.
    • Compatibility Impact: Likely no, but explicitly state it.
  • Missing Testing Details: "ci test" isn't sufficient. Provide specific details:

    • Build Host: OS, CPU architecture, compiler version.
    • Target: Architecture (e.g., simulator, specific hardware), board and configuration.
    • Logs: Actual before and after logs demonstrating the change's effect (or explain why logs are not applicable, if that's the case). Just saying "ci test" doesn't show what was tested or the results of the test.

The PR description needs to be much more thorough to meet the NuttX requirements. Provide the missing context and details to make it clear why the change is necessary and that it has been properly validated.

@cederom
Copy link

cederom commented Feb 26, 2025

CI failed, restarted.

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

  • Please try to use relative paths (i.e. apps/testing/fs/basic) as /basic means a location in root directory so totally different place :-P
  • I think the agreements was made before that test scenarios should be located in apps/tests/ while apps/testing/ was supposed to hold utilities and frameworks. Therefore target location should be apps/tests/cmocka/fs instead. Other cmocka tests could be placed there in analog way with functional location names. I have asked for confirmation in the original tests reorganization thread The placement of cases under testing is a bit messy, and cases of the same type need to be merged #2931.

Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

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

please add a commit message

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