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

The placement of cases under testing is a bit messy, and cases of the same type need to be merged #2931

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

Conversation

txy-21
Copy link

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

Summary

Merge the test cases under testing into the same folder.

Impact

The cases under apps/testing will be separated and placed in different folders.

Testing

Local self-test can be done through.

@nuttxpr
Copy link

nuttxpr commented Jan 7, 2025

[Experimental Bot, please feedback here]

No, this PR description does not adequately meet the NuttX requirements. Here's why and how to fix it:

Missing Information in Summary:

  • Why is this change necessary? Just saying "merge test cases" isn't enough. Explain the benefits (e.g., better organization, easier to find tests, avoids naming conflicts, improves maintainability).
  • What functional part of the code is being changed? Be specific. "apps/testing" is a start, but mention the affected subdirectories or modules.
  • How does the change exactly work? Don't just say they'll be "separated and placed in different folders." Describe the new folder structure. What criteria are used to separate the tests? Provide examples.
  • Related Issues: If this addresses a specific issue in NuttX or NuttX Apps, link to it.

Missing Information in Impact:

  • All the "Impact" questions need more specific answers than "NO" or a vague "YES." Even if the answer is "NO," briefly explain why (e.g., "Impact on build: NO, this only affects the organization of source files").
  • Impact on user: How will this reorganization affect users who run these tests? Will they need to change any commands or scripts?
  • Impact on documentation: If the organization of tests changes, the documentation probably needs to be updated. State that explicitly.

Insufficient Testing Information:

  • Build Host(s): Specify your development environment (OS, CPU architecture, compiler version).
  • Target(s): List the specific architectures and boards you tested on (e.g., sim:nsh, arm:stm32f4discovery:hello).
  • Testing logs: Provide actual logs, not just "Local self-test can be done through." Show the test output before and after the change to demonstrate that the tests still pass and that the reorganization hasn't broken anything. If the change is about fixing tests, show how the output has improved.

Example of a Better PR Description:

Summary

This PR reorganizes the test cases under apps/testing to improve maintainability and findability. Currently, all test cases are in a single directory, making it difficult to navigate and potentially leading to naming conflicts. This change groups related tests into subdirectories based on their functionality (e.g., networking tests, file system tests, driver tests). This addresses [NuttX Apps Issue #XYZ] (if applicable).

Impact

  • Is new feature added? NO
  • Is existing feature changed? YES, the organization of test cases is changed.
  • Impact on user: YES. Users running these tests will need to update their commands or scripts to reflect the new directory structure. The README in apps/testing will be updated with the new organization.
  • Impact on build: NO, this only affects the organization of source files, not the build process itself.
  • Impact on hardware: NO, this change does not affect any hardware or drivers.
  • Impact on documentation: YES, the README in apps/testing will be updated to describe the new directory structure.
  • Impact on security: NO
  • Impact on compatibility: NO, existing tests should continue to function as before.
  • Anything else to consider: N/A

Testing

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

  • Build Host: macOS Ventura, Intel x86_64, GCC 12.2
  • Target(s): sim:nsh, arm:stm32f4discovery:hello

Testing logs before change:

ls apps/testing
test_case1.c test_case2.c test_case3.c ...
./apps/testing/test_case1
... test output ...

Testing logs after change:

ls apps/testing
networking/ filesystem/ drivers/
ls apps/testing/networking
test_case1.c test_case2.c
./apps/testing/networking/test_case1
... test output ...

By providing more details and specific information, you make it much easier for reviewers to understand and approve your PR. Remember to replace the placeholders with your actual information.

@xiaoxiang781216
Copy link
Contributor

This patch want to change the folder layout under testing, @cederom and @acassis please review whether this patch is acceptable?

testing/mm/cmocka/Kconfig Outdated Show resolved Hide resolved
@acassis
Copy link
Contributor

acassis commented Jan 7, 2025

@txy-21 please fix the conflicts

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 good idea, it should keep things clean, on the other hand folks may search for existing tests in subdirectories :-P PR needs a rebase. Then we can see the CI build results :-)

Some questions:

  • do we / should we want to resemble nuttx.git organization with these tests?
  • himem_test and memtest are moved to arch/ but mm is not, why? Maybe we want dedicated memory/ section for all memory related tests?
  • If we want to resemble nuttx/ organization do we want drivers/ for drivers test (in place of driver)?
  • cmocka does not really tell what category is tested under? Why it is placed under fs/? Is it the cmocka framework test or cmocka is used to test something else? If so the tests should be named by their function not a test tool used? See mm/cmocka comment below, if tests are using cmocka framework and tests inside are self-explanatory then okay :-)
  • before we had testuites/kernel/fs/* now it is renamed to fs/cmocka/*
  • what happens to fff? It is renamed to fs/cmocka/ but no reason why and what happens with fff. Anyone using fff? If so they will see things just disappeared with no info in the commit message.
  • what is the fdsantest test for? do we want to update description too?
  • do we want nand_sim in fs/ or drivers/? does it operate on driver or filesystem level?
  • do we want sd_bench and sd_stress in fs/ or drivers/? does it operate on driver or filesystem level?
  • do we want libc in libs/libc/libc_test to resemble nuttx layout?
  • do we want arch_libs in libs/arch_libs?
  • do we want fmemopen in libs/libc/stdio/fmemopen? libs/libc/fmemopen is also fine.
  • do we want scanftest in libs/libc/stdio/scanftest? libs/libc/scanftest is also fine.
  • mm/cmocka is similar to fs/cmocka.. I assume these are mm tests performed with use of cmocka framework and test inside will be self-explanaotory? Still it would be nice to know what tests are included and performed under cmocka?
  • should monkey go to drivers/monkey instead of mm/monkey?
  • should resmonitor go to os/resmonitor? is mm/ a good selection?
  • should stressapptest go to os/stressapptest?
  • I can see that fff again is removed and becomes part of net/. why?
  • testsuites/kernel/socket becomes net/cases.. shouldn't it become net/cmocka to align with other cmocka tests?
  • do we want rpmsg to go to fs/rpmsg or net/rpmsg?
  • do we want atomic to go to os/atomic or sched/atomic is okay?
  • why sched/cmocka_mutex has dedicated category not sched/cmocka as other cmocka tests?
  • the same as above for sched/cmocka_pthread why not just case for sched/cmocka?
  • the same as above for sched/cmocka_syscall?
  • the same as above for sched/cmocka_time?
  • do we want cpuload in os/cpuload or sched/ is better location?
  • do we want getprime in os/getprime or sched/ is better location?
  • do we want smp in os/smp or sched/ is better location?
  • do we want timerjitter in os/timerjitter or sched/ is better location?
  • Regarding the os/ test location it may contain generic (RT)OS tests while sched/ test would be dedicated to NuttX scheduler.. OS test may contain also other tests like os/ostest etc.
  • It seems that not all tests are moved?

@txy-21 txy-21 force-pushed the apps-case-dir-merge branch 7 times, most recently from 6c06aef to a5bf405 Compare January 8, 2025 14:11
@GUIDINGLI
Copy link
Contributor

Thank you @txy-21 good idea, it should keep things clean, on the other hand folks may search for existing tests in subdirectories :-P PR needs a rebase. Then we can see the CI build results :-)

Some questions:

  • do we / should we want to resemble nuttx.git organization with these tests?

Not exactly the same layout with nuttx.git, because something is not suitable, like nsh_test (if we have in the feature), lvgl...
So we prefer the feature category:
drivers libc fs rpmsg ...

  • himem_test and memtest are moved to arch/ but mm is not, why? Maybe we want dedicated memory/ section for all memory related tests?

Because himem_test not the common mm test, it depends on the specific arch chip.
And either 'mm' or 'arch' is ok for me, we can change to 'mm' if you insist.

  • If we want to resemble nuttx/ organization do we want drivers/ for drivers test (in place of driver)?
    yes, we will change it to 'drivers'
  • cmocka does not really tell what category is tested under? Why it is placed under fs/? Is it the cmocka framework test or cmocka is used to test something else? If so the tests should be named by their function not a test tool used? See mm/cmocka comment below, if tests are using cmocka framework and tests inside are self-explanatory then okay :-)

Yes, the 'mm/cmocka' will be confuse, we can change to 'mm/basic' ?
And we don't want to modify the test code in this PR, just move the folder and classify them.

  • before we had testuites/kernel/fs/* now it is renamed to fs/cmocka/*

same will 'mm/cmocka' change to fs/basic/*

  • what happens to fff? It is renamed to fs/cmocka/ but no reason why and what happens with fff. Anyone using fff? If so they will see things just disappeared with no info in the commit message.

We think the fff is none use it, so we decide remove it. Will split to another PR to discuss this.

  • what is the fdsantest test for? do we want to update description too?

The test case of tools fdsan, for the file descriptor used after free. The description will update in another PR.

  • do we want nand_sim in fs/ or drivers/? does it operate on driver or filesystem level?

Good find, should move to drivers.

  • do we want sd_bench and sd_stress in fs/ or drivers/? does it operate on driver or filesystem level?

Aslo move to drivers.

  • do we want libc in libs/libc/libc_test to resemble nuttx layout?
  • do we want arch_libs in libs/arch_libs?
  • do we want fmemopen in libs/libc/stdio/fmemopen? libs/libc/fmemopen is also fine.
  • do we want scanftest in libs/libc/stdio/scanftest? libs/libc/scanftest is also fine.

Probably not, we named the folder base on feature category, how do you think ?

  • mm/cmocka is similar to fs/cmocka.. I assume these are mm tests performed with use of cmocka framework and test inside will be self-explanaotory? Still it would be nice to know what tests are included and performed under cmocka?

'mm/cmocka' change to fs/basic will be OK ?
And the cmocka-based testcases are just cases.
We should be classify them with features not the case-based ?

  • should monkey go to drivers/monkey instead of mm/monkey?

Agree & Accept.

  • should resmonitor go to os/resmonitor? is mm/ a good selection?

So we start a 'os' folder, as the stress-likely test ? How about 'stress'

  • should stressapptest go to os/stressapptest?

OK,
But the 'monkey' should also the go to 'os/monkey' or 'stress/monkey' ? because they are all the stress test.

  • I can see that fff again is removed and becomes part of net/. why?

No, we remove it. As said before will start a new PR, to see opinions from others.

  • testsuites/kernel/socket becomes net/cases.. shouldn't it become net/cmocka to align with other cmocka tests?

Also change to net/basic

  • do we want rpmsg to go to fs/rpmsg or net/rpmsg?

No, rpmsg is a independent feature, is for the AMP IPC

  • do we want atomic to go to os/atomic or sched/atomic is okay?

Prefer sched/atomic

  • why sched/cmocka_mutex has dedicated category not sched/cmocka as other cmocka tests?
  • the same as above for sched/cmocka_pthread why not just case for sched/cmocka?
  • the same as above for sched/cmocka_syscall?
  • the same as above for sched/cmocka_time?

The folder name should a feature, not a 'cmocka-based' folder name I think.
somewhere named cmocka because we don't want the same name, so change to 'baisc'.
'sched/sched' change to 'sched/basic' @txy-21

  • do we want cpuload in os/cpuload or sched/ is better location?
  • do we want getprime in os/getprime or sched/ is better location?
  • do we want smp in os/smp or sched/ is better location?
  • do we want timerjitter in os/timerjitter or sched/ is better location?

Prefer sched

  • Regarding the os/ test location it may contain generic (RT)OS tests while sched/ test would be dedicated to NuttX scheduler.. OS test may contain also other tests like os/ostest etc.

'os' is too big, all the testcase can after os, because we are doing a OS ^_^

  • It seems that not all tests are moved?

The fist version only change the 'apps/testing'.

@raiden00pl
Copy link
Member

raiden00pl commented Jan 8, 2025

this page in the doc should be updated according to this PR https://nuttx.apache.org/docs/latest/applications/testing/index.html so we know were a given test app can be found.

@cederom
Copy link

cederom commented Jan 9, 2025

Thank you @GUIDINGLI for the feedback :-)

@cederom: Thank you @txy-21 good idea, it should keep things clean, on the other hand folks may search for existing tests in subdirectories :-P PR needs a rebase. Then we can see the CI build results :-)
Some questions:

  • do we / should we want to resemble nuttx.git organization with these tests?

@GUIDINGLI: Not exactly the same layout with nuttx.git, because something is not suitable, like nsh_test (if we have in the feature), lvgl... So we prefer the feature category: drivers libc fs rpmsg ...

okay :-)

  • himem_test and memtest are moved to arch/ but mm is not, why? Maybe we want dedicated memory/ section for all memory related tests?

Because himem_test not the common mm test, it depends on the specific arch chip. And either 'mm' or 'arch' is ok for me, we can change to 'mm' if you insist.

If its more architecture specific then arch/ is better location, thanks :-)

  • If we want to resemble nuttx/ organization do we want drivers/ for drivers test (in place of driver)?
    yes, we will change it to 'drivers'

thanks :-)

  • cmocka does not really tell what category is tested under? Why it is placed under fs/? Is it the cmocka framework test or cmocka is used to test something else? If so the tests should be named by their function not a test tool used? See mm/cmocka comment below, if tests are using cmocka framework and tests inside are self-explanatory then okay :-)

Yes, the 'mm/cmocka' will be confuse, we can change to 'mm/basic' ? And we don't want to modify the test code in this PR, just move the folder and classify them.

The mm/cmocka is also fine as it shows the Memory Management has dedicated tests performed with cmocka. It can be named mm/generic or mm/standard or whatever fits best, maybe mm/basic_tests is better than basic as it does not remind about the BASIC programming language :D What other folks think about this? :-)

  • before we had testuites/kernel/fs/* now it is renamed to fs/cmocka/*
    same will 'mm/cmocka' change to fs/basic/*

ACK

  • what happens to fff? It is renamed to fs/cmocka/ but no reason why and what happens with fff. Anyone using fff? If so they will see things just disappeared with no info in the commit message.

We think the fff is none use it, so we decide remove it. Will split to another PR to discuss this.

I think the mention of removing fff should be in this PR (commit mesage) because this PR and commit removes it.. or whatever place / PR touches the code. Just to let folks know what happened and why it is not here anymore :-)

  • what is the fdsantest test for? do we want to update description too?
    The test case of tools fdsan, for the file descriptor used after free. The description will update in another PR.

ACK

  • do we want nand_sim in fs/ or drivers/? does it operate on driver or filesystem level?
    Good find, should move to drivers.

thanks :-)

  • do we want sd_bench and sd_stress in fs/ or drivers/? does it operate on driver or filesystem level?
    Aslo move to drivers.

thanks :-)

  • do we want libc in libs/libc/libc_test to resemble nuttx layout?
  • do we want arch_libs in libs/arch_libs?
  • do we want fmemopen in libs/libc/stdio/fmemopen? libs/libc/fmemopen is also fine.
  • do we want scanftest in libs/libc/stdio/scanftest? libs/libc/scanftest is also fine.

Probably not, we named the folder base on feature category, how do you think ?

I think libs/libc will provide more space for other libs in libs/ too. What other people think about this? :-)

  • mm/cmocka is similar to fs/cmocka.. I assume these are mm tests performed with use of cmocka framework and test inside will be self-explanaotory? Still it would be nice to know what tests are included and performed under cmocka?

'mm/cmocka' change to fs/basic will be OK ? And the cmocka-based testcases are just cases. We should be classify them with features not the case-based ?

Yes, this is the point, cmocka name can be used to show cmocka framework is used for testing, but when looking inside tests should be self-explanatory to easily know what is tested under.. maybe some description like "generic feature tests performed with cmocka framework" would be helpful.. I think it works like this right now? :-)

  • should monkey go to drivers/monkey instead of mm/monkey?
    Agree & Accept.

thanks :-)

  • should resmonitor go to os/resmonitor? is mm/ a good selection?
    So we start a 'os' folder, as the stress-likely test ? How about 'stress'
  • should stressapptest go to os/stressapptest?
    OK, But the 'monkey' should also the go to 'os/monkey' or 'stress/monkey' ? because they are all the stress test.

I think that os/ is more generic and more OS specific tests can land there including stress-testing. If you like stresstesting can be a dedicated separate category why not :-)

Monkey test is undocumented and it seems to test UINPUT so probably drivers/ would be good category as agreed above before?

  • I can see that fff again is removed and becomes part of net/. why?
    No, we remove it. As said before will start a new PR, to see opinions from others.

Okay, it should not be touched here in that case :-)

  • testsuites/kernel/socket becomes net/cases.. shouldn't it become net/cmocka to align with other cmocka tests?
    Also change to net/basic

Or net/cmocka to be discussed with other devs, whatever name fits best :-)

  • do we want rpmsg to go to fs/rpmsg or net/rpmsg?

No, rpmsg is a independent feature, is for the AMP IPC

ACK :-)

  • do we want atomic to go to os/atomic or sched/atomic is okay?

Prefer sched/atomic

Okay :-) If the sched/ is better place then be it. This is why I considered dedicated os/ category for all generic OS features testing :-)

  • why sched/cmocka_mutex has dedicated category not sched/cmocka as other cmocka tests?
  • the same as above for sched/cmocka_pthread why not just case for sched/cmocka?
  • the same as above for sched/cmocka_syscall?
  • the same as above for sched/cmocka_time?

The folder name should a feature, not a 'cmocka-based' folder name I think. somewhere named cmocka because we don't want the same name, so change to 'baisc'. 'sched/sched' change to 'sched/basic' @txy-21

Okay so the pthread, syscall, time, etc, should be merged into one single sched/cmocka ?

  • do we want cpuload in os/cpuload or sched/ is better location?
  • do we want getprime in os/getprime or sched/ is better location?
  • do we want smp in os/smp or sched/ is better location?
  • do we want timerjitter in os/timerjitter or sched/ is better location?

Prefer sched

ACK :-) I am curious of other folks ideas here? shed vs os :-)

  • Regarding the os/ test location it may contain generic (RT)OS tests while sched/ test would be dedicated to NuttX scheduler.. OS test may contain also other tests like os/ostest etc.

'os' is too big, all the testcase can after os, because we are doing a OS ^_^

Yeah but it seems intutitive to have all generic tests over there.. but I am not enforcing anything.. if other folks also think that wat let it be sched/ :-)

  • It seems that not all tests are moved?

The fist version only change the 'apps/testing'.

ACK :-)

Thank you :-)

@txy-21 txy-21 force-pushed the apps-case-dir-merge branch from 4581cd3 to cd950ae Compare January 10, 2025 06:25
@txy-21
Copy link
Author

txy-21 commented Jan 10, 2025

According to this commenthttps://github.com//pull/2931#issuecomment-2577992183,
this PR does the following:
1.The existing fff framework is not used by anyone, so we decided to delete it.
2.The testsuites folder remains as is, changes to it will be made in the next PR.
3.move nand_sim,sd_bench and sd_stress from fs/ to driver/.
4.move monkey from mm/ to driver/.

.gitignore Outdated
@@ -47,3 +47,4 @@ build
.ccls-cache
compile_commands.json
.aider*
testing/*/Kconfig
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong. There is testing/unity/Kconfig which should not be ignored

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your reminder, I will fix this error.

@xiaoxiang781216
Copy link
Contributor

let's split the indpedent change to new pr, so we can progresss step by step.

Copy link
Member

@raiden00pl raiden00pl left a comment

Choose a reason for hiding this comment

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

I don't think that apps/testing is a good place to keep any tests logic. This directory was created for unity framework which is intended to use by user code to implement user application tests (TDD), but somehow it happened that more and more apps were migrated there.

It think that currently almost nothing fits there except Unity, Cmoka and fff. Why not create a new folder like tests which will hold all test applications, and leave testing for tools that can be used by user code? This directory should be reserved for things like testing frameworks integration and similar things.
It's more common in various projects to use tests directory to hold test logic, not testing.

Also removing fff doesn't seems right. It doesn't mater if its not used by any tests in apps, it still can be used to implement user specific tests. Adding a feature just to remove it a few months later is not ok. Maybe no one uses it, or maybe someone does, who knows?

@txy-21
Copy link
Author

txy-21 commented Jan 14, 2025

Thank @raiden00pl for your suggestion. This PR only integrates the cases in the testing folder. If you think it is a better choice to create a tests folder to store cases, you can create a PR to make the change.
Regarding the fff/ framework, I think what you said makes sense, so I kept the fff/ folder here.

@txy-21 txy-21 force-pushed the apps-case-dir-merge branch from cd950ae to a1f17a4 Compare January 14, 2025 07:17
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