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

boards: Rename CONFIG_TESTING_SENSORTEST to CONFIG_SYSTEM_SENSORTEST #15698

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

xiaoxiang781216
Copy link
Contributor

Summary

follow the change from: apache/nuttx-apps#2976

Impact

Testing

ci

@lupyuen lupyuen changed the title boards: Renanme CONFIG_TESTING_SENSORTEST to CONFIG_SYSTEM_SENSORTEST boards: Rename CONFIG_TESTING_SENSORTEST to CONFIG_SYSTEM_SENSORTEST Jan 26, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 26, 2025

[Experimental Bot, please feedback here]

No, this PR description does not meet the NuttX requirements. It's far too brief and lacks crucial information. Here's a breakdown of what's missing and how to fix it:

Summary:

  • Insufficient Detail: Simply referencing another PR isn't enough. You must summarize the change within this PR's description. Explain the why, what, and how of the change independently. Copy-pasting from the other PR is acceptable, but ensure the context is clear for someone reviewing this PR specifically.
  • Missing Issue References: If this PR addresses a specific issue in either the NuttX or NuttX Apps repositories, provide the full issue URL (e.g., https://github.com/apache/nuttx/issues/1234).

Impact:

  • Completely Empty: This section is crucial. You must address each point, even if the answer is "NO." For "NO" answers, a simple "NO" is sufficient. However, for "YES" answers, provide a concise explanation of the impact. For example:
    • Impact on user: YES (Users will now need to configure option XYZ in their .config file to enable this feature.)
    • Impact on hardware: YES (This change adds support for the ABC sensor on the DEF board.)
  • Consider all points: Don't skip any of the impact categories. Even seemingly minor changes can have unforeseen consequences.

Testing:

  • Insufficient Information: "ci" is not enough. While CI testing is important, you also need to provide details about your local testing.
  • Missing Details: Specify the exact build host OS, CPU architecture, compiler version, target architecture, board, and configuration. Be precise. For example:
    • Build Host: macOS Ventura (Intel Core i7), GCC 12.2
    • Target: sim:nsh
  • No Logs: The "Testing logs before change" and "Testing logs after change" sections are empty. You must provide relevant logs demonstrating the issue before the change and the correct behavior after the change. These logs should clearly show the impact of your changes. Even if the change is small, include some output demonstrating the difference.

Example of a better Testing section:

Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host(s): Linux (Ubuntu 22.04, x86_64), GCC 11.3.0
* Target(s): sim:nsh

Testing logs before change:

nsh> help ls
ls: not found


Testing logs after change:

nsh> help ls
ls: list directory contents
usage: ls [-l] []

By providing more complete information, reviewers can understand your changes, assess their impact, and verify that they work as intended. A well-written PR description significantly speeds up the review process and increases the likelihood of your contribution being accepted.

@cederom
Copy link
Contributor

cederom commented Jan 26, 2025

Isnt this commit part of apache/nuttx-apps#2976? Why separate PR? :-)

@xiaoxiang781216
Copy link
Contributor Author

Isnt this commit part of apache/nuttx-apps#2976? Why separate PR? :-)

it's in the different git, can't in one pr.

@cederom
Copy link
Contributor

cederom commented Jan 27, 2025

Braindead, sorry, long time no sleep!!! :D :D

Copy link
Contributor

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

Sorry for my faux pas :D Merging now together with apache/nuttx-apps#2976 as instructed :-)

@cederom cederom merged commit 4ef36cf into apache:master Jan 27, 2025
17 of 39 checks passed
@xiaoxiang781216 xiaoxiang781216 deleted the testing branch January 27, 2025 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants