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: Add a cmocka framework for network testing #2935

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

zs39
Copy link
Contributor

@zs39 zs39 commented Jan 9, 2025

Summary

Provides a network automated testing framework, including the main directory structure and Makefile, CMakeLists, Kconfig and other files such as tcp.

Impact

N/A

Testing

Local qemu environment verification

Provides a network automated testing framework, including the main directory structure and Makefile, CMakeLists, Kconfig and other files such as tcp.

Signed-off-by: zhangshuai39 <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Jan 9, 2025

[Experimental Bot, please feedback here]

No, this PR summary does not adequately meet the NuttX requirements. While it provides a basic outline, it lacks crucial detail. Here's why:

  • Summary: "Provides a network automated testing framework" is too vague. What kind of network tests? TCP only, as mentioned later? What aspects of networking are being tested? The mention of tcp.c implies a specific test, not a framework. This section needs to explain the purpose and scope of the framework. It should also clarify the relation between the framework and the seemingly specific tcp.c file. Furthermore, the relationship to existing testing infrastructure (if any) should be discussed.

  • Impact: Claiming "N/A" (Not Applicable) for all impact categories is highly unlikely for a new testing framework. Consider the following:

    • New Feature: This is clearly a new feature and should be stated as such.
    • Impact on User: Does this impact how users run existing tests? Will they need to learn a new testing interface?
    • Impact on Build: Adding a new framework undoubtedly changes the build process, even minimally. At least the build target for the test framework itself needs description. Specify any new build options or dependencies.
    • Impact on Hardware: While the framework itself might not directly impact hardware, its usage might. Does it require specific hardware configurations or features?
    • Impact on Documentation: A new feature requires documentation. This PR should either include documentation updates or explicitly state why they are not yet included (with a plan for adding them).
    • Impact on Security/Compatibility: While these might genuinely be N/A, it's better to explicitly state that these aspects have been considered and no impact is expected, rather than just using a blanket "N/A".
  • Testing: "Local qemu environment verification" is insufficient. What specific tests were run? What were the expected outcomes? The provided log sections are empty. Include actual log output demonstrating the tests and their results. Specify the QEMU configuration (e.g., the machine type). If only tcp.c is being tested, that should be explicitly stated. The build host details are also missing, violating the requirement.

In short, the PR description needs significant expansion to meet the NuttX requirements. More details are needed in every section to make it a complete and informative description of the changes.

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

@xiaoxiang781216
Copy link
Contributor

Thank you @zs39 :-)

  • This is just the initial template with no tests that you will add soon right?

Yes.

  • Please let us know how this testing work at current stage, can you provide a test log?

will provide in the next patchset.

yes, that's why we want upstreaming nettest first to avoid the conflct with the structure layout.

@xiaoxiang781216 xiaoxiang781216 merged commit f2b0437 into apache:master Jan 9, 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.

4 participants