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

testing/ostest/signest.c: fix the range of signals #2863

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Nov 22, 2024

Summary

Note that MAX_SIGNO is a valid signal number.

Impact

Testing

esp32s3-devkit:smp

Note that MAX_SIGNO is a valid signal number.
@nuttxpr
Copy link

nuttxpr commented Nov 22, 2024

[Experimental Bot, please feedback here]

This PR appears to be missing crucial information to meet the NuttX requirements. While it provides a skeletal structure, it lacks the necessary details in almost every section.

Missing Information:

  • Summary: The summary only mentions MAX_SIGNO being a valid signal number. This doesn't explain why this change is needed, what code is changed, or how the change works. Is this a bug fix? A new feature? What problem does it solve?
  • Impact: All impact sections are empty. This makes it impossible to assess the ramifications of the change. Even if the impact is minimal for some categories, explicitly stating "NO" with a brief justification is better than leaving it blank.
  • Testing: While a target is listed (esp32s3-devkit:smp), the critical "before" and "after" testing logs are empty. Without these logs, there's no evidence that the change works as intended or that the problem being addressed is actually fixed. The build host details are also missing.

To make this PR compliant, the author needs to provide:

  • A complete summary explaining the context, the change, and its purpose.
  • Fill out all impact sections, even if the answer is "NO." Justify "NO" answers briefly. If the answer is "YES," provide a detailed explanation.
  • Include actual testing logs demonstrating the behavior before and after the change. Specify the build host details (OS, CPU, compiler).

Without this information, reviewers cannot effectively evaluate the PR. It's likely to be rejected in its current state.

@xiaoxiang781216 xiaoxiang781216 merged commit 365869b into apache:master Nov 22, 2024
25 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.

3 participants