nuttxpr commented on PR #15285: URL: https://github.com/apache/nuttx/pull/15285#issuecomment-2553081160
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements. While it provides a decent summary and impact assessment, it severely lacks in the testing section. Here's a breakdown of what's missing: * **Insufficient Summary Detail:** While the summary explains the "why" and the "what," it lacks detail on *how* the buffer pool works. More information on the implementation details is needed. Mention the specific functions added/modified. * **Missing Issue References:** The PR summary should reference any related NuttX or NuttX Apps issues. Even if there isn't a directly related issue, consider creating one to track the motivation and discussion for this change. * **Incomplete Impact Assessment:** Several "Impact" fields are left blank. Explicitly state "NO" or provide details for *all* impact categories. For example: * **Impact on user:** Does this change the API? Do users need to configure the buffer pool? * **Impact on build:** Are there any new Kconfig options? * **Impact on hardware:** Does this affect any specific architectures or boards? Even if it's intended to be generic, stating that explicitly is important. * **Impact on documentation:** Does this require updates to the NuttX documentation? If so, were these updates included in the PR? * **Impact on security:** Even if the answer is no, explicitly state it. * **Impact on compatibility:** Does this break any existing applications? * **Inadequate Testing Description:** "Mainly on SIM" is not enough. Specify the *exact* SIM configuration used (e.g., `nsh`). What "different configs" were tested for tcp/udp/icmp/devif_callback? Provide concrete examples. "Other pools share exactly the same logic, not tested very carefully" is unacceptable. All affected code paths should be thoroughly tested. * **Missing Test Logs:** The template explicitly requests "Testing logs before change" and "Testing logs after change." These are crucial for demonstrating the effectiveness of the change and for regression testing. Include the actual command-line output and relevant log messages. Even for simple changes, showing the output before and after helps reviewers understand the impact. In short, the PR needs significant improvement in the testing and impact assessment sections before it can be properly reviewed. A more detailed summary and inclusion of issue references would also improve the PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
