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 {drivertest fftest irtest monkey nand_sim pcitest sd_bench sd_stress sensortest} folders to the new driver folder #2961

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 driver folder and move {drivertest fftest irtest monkey nand_sim pcitest sd_bench sd_stress sensortest} folders to the new driver folder.

Impact

This PR only affects the apps/testing folder, changing the location of some subfolders under this folder.
new folder : driver
move : drivertest fftest irtest monkey nand_sim pcitest sd_bench sd_stress sensortest

Testing

CI test.

@nuttxpr
Copy link

nuttxpr commented Jan 21, 2025

[Experimental Bot, please feedback here]

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

Here's what's missing:

  • Summary: While the summary explains what is being done, it lacks the why. What problem does moving these test folders solve? How does this relate to the linked PR? Is this a cleanup effort? Is it preparing for future work? The summary needs to justify the change. It also needs the corresponding NuttX issue reference if one exists.
  • Impact: While the PR mentions affected files, it doesn't explicitly answer all the required impact questions with YES/NO followed by details if applicable. Even if the answer is NO, stating it explicitly ensures reviewers don't have to guess. For example:
    • Impact on user: NO
    • Impact on build: Potentially YES (if any build scripts relied on the old paths. Even if unlikely, this needs to be considered).
    • Impact on hardware: NO
    • Impact on documentation: YES (Any documentation referencing these test applications will need updating).
    • Impact on security: NO
    • Impact on compatibility: Potentially YES (if external scripts or tools rely on the old paths).
  • Testing: "CI test" is insufficient. The requirements explicitly request testing logs before and after the change. Even if the CI passes, providing snippets of relevant output demonstrating the tests still function as expected would greatly improve the PR. Also, details of the build host and target are missing.

In short, the PR needs more detail and explicit answers to the checklist items to be considered complete. While the changes themselves might be simple, demonstrating due diligence in the PR description is essential for proper review and acceptance.

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

@cederom
Copy link

cederom commented Jan 21, 2025

Didn't we want drivers/? :-P

@txy-21
Copy link
Contributor Author

txy-21 commented Jan 21, 2025

@cederom ok, change driver to dirvers

testing/driver/irtest/cmd.cxx Outdated Show resolved Hide resolved
testing/driver/sensortest/sensortest.c Outdated Show resolved Hide resolved
testing/driver/monkey/Make.defs Outdated Show resolved Hide resolved
testing/driver/sd_bench/Makefile Outdated Show resolved Hide resolved
@txy-21 txy-21 force-pushed the merge-driver-folder branch from a9c10b3 to 5af43ed Compare January 21, 2025 02:13
…d_bench sd_stress sensortest} folders to the new driver folder

Signed-off-by: tengshuangshuang <[email protected]>
@txy-21 txy-21 force-pushed the merge-driver-folder branch from 5af43ed to a13e928 Compare January 21, 2025 02:16
@txy-21
Copy link
Contributor Author

txy-21 commented Jan 21, 2025

@cederom Thanks for your suggestion.About irtest,sd_bench,monkey and sensortest folders, @xiaoxiang781216 has a different suggestion. His opinion is somewhat different from what we discussed in #2931. What do you think of his suggestion?

@cederom
Copy link

cederom commented Jan 21, 2025

@txy-21: @cederom Thanks for your suggestion.About irtest,sd_bench,monkey and sensortest folders, @xiaoxiang781216 has a different suggestion. His opinion is somewhat different from what we discussed in #2931. What do you think of his suggestion?

Always good to discuss :-) Maybe better solution can be found that way :-)

@cederom
Copy link

cederom commented Jan 21, 2025

Okay it seems @raiden00pl remarks on separating apps/testing into testing utilities (something to perform the tests) and apps/tests test scenarios (list of specific tests that returns PASS/FAIL) makes more sense now. This is also what @xiaoxiang781216 suggests right now :-)

What would be better / possible approach for now @txy-21 @xiaoxiang781216 ?

  1. apps/testing cleanup by organizing existing stuff by function first then identify test cases and move them next to apps/tests based on categories created in apps/testing?
  2. Use current time/work and split apps/testing/function and apps/tests/function right away?

Update: From the specific comments above it seems also some utilities in the apps/testing can be moved out into function specific location as these are more generic utilities / applications not really testing dedicated stuff?

@xiaoxiang781216 xiaoxiang781216 merged commit 6bc2b3c into apache:master Jan 21, 2025
37 checks passed
@xiaoxiang781216
Copy link
Contributor

@GUIDINGLI suggest we move the tools/utils to apps/system and benchmark to apps/benchmark.

@raiden00pl
Copy link
Member

@xiaoxiang781216 I thought the same - we can move some tools back to apps/system and some to apps/benchmark. But what about cmoka , unity and fff? These fits quite well in apps/testing. Maybe something like this:

  • programs that return pass/fail output - apps/tests
  • testing frameworks to be used by user- apps/testing
  • performance tests - apps/benchmark
  • small testing apps that doesn't fit above - apps/system

@cederom
Copy link

cederom commented Jan 21, 2025

I liked the @txy-21 idea that some testing utils for instance targeted for drivers tests could be placed in the apps/testing/drivers (or similar).. but looking at apps/system most programs there are testing utilities already.. the same with benchmarks :-) So apps/testing should be reserved for testing frameworks only according to original concept?

For sure @xiaoxiang781216 initial remarks here on what are "tests" in terms to return PASS/FAIL are in align with previous discussion @raiden00pl suggestions and I fully agree these should be separated and apps/tests seems best / standard / acclaimed location :-)

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Jan 21, 2025

@xiaoxiang781216 I thought the same - we can move some tools back to apps/system and some to apps/benchmark. But what about cmoka , unity and fff? These fits quite well in apps/testing. Maybe something like this:

  • programs that return pass/fail output - apps/tests
  • testing frameworks to be used by user- apps/testing

I prefer that testing frameworks and test case under one top level folder(either tests or testing is fine to me), we can move the testing testing/framework to testing/utils or frameworks etc.

  • performance tests - apps/benchmark
  • small testing apps that doesn't fit above - apps/system

LGTM.

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.

7 participants