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

Revert "add test for longjump with 0 as return value" #2720

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

GUIDINGLI
Copy link
Contributor

Summary

Revert "add test for longjump with 0 as return value"

This reverts commit 63542f8.

user_main: setjmp test
setjmp_test: Initializing jmp_buf
setjmp_test: Try jump
setjmp_test: About to jump, longjmp with ret val: 123
setjmp_test: Jump succeed
setjmp_test: Try jump
setjmp_test: About to jump, longjmp with ret val: 0
setjmp_test: Try jump
dump_assert_info: Current Version: NuttX 10.4.0 11e0262ae8 Oct 14 2024 12:53:49 sim
dump_assert_info: Assertion failed : at file: setjmp.c:46 task: ostest process: ostest 0x7de654
sched_dumpstack: backtrace:
sched_dumpstack: [ 6] [<0xb211b1>] host_backtrace+0x2d/0x41
sched_dumpstack: [ 6] [<0x7a39b2>] up_backtrace+0x10a/0x189
sched_dumpstack: [ 6] [<0x78dc79>] sched_backtrace+0x4d/0x9a
sched_dumpstack: [ 6] [<0x7974e5>] sched_dumpstack+0x5e/0x141
sched_dumpstack: [ 6] [<0x78c679>] dump_running_task+0x2e/0x31
sched_dumpstack: [ 6] [<0x78c83d>] dump_assert_info+0x1c1/0x1dd
sched_dumpstack: [ 6] [<0x78c9df>] _assert+0x146/0x1bd
sched_dumpstack: [ 6] [<0x749df4>] __assert+0x2f/0x34
sched_dumpstack: [ 6] [<0x7df66c>] jump_with_retval+0x7f/0x11b
sched_dumpstack: [ 6] [<0x7df76c>] setjmp_test+0x64/0x82
sched_dumpstack: [ 6] [<0x7dea60>] user_main+0x40c/0x514
sched_dumpstack: [ 6] [<0x74a9ed>] nxtask_startup+0x57/0x5e
sched_dumpstack: [ 6] [<0x740db9>] nxtask_start+0xdf/0xe9
sched_dumpstack: [ 6] [<0x756757>] pre_start+0x2b/0x2e
ostest_main: Exiting with status 256
stdio_test: Standard I/O Check: fprintf to stderr

Impact

ostest

Testing

CI

@nuttxpr
Copy link

nuttxpr commented Oct 14, 2024

[Experimental Bot, please feedback here]

NuttX PR Requirements Review

This PR does not fully meet the NuttX requirements. Here's why:

Summary:

  • While the summary explains what the PR does (reverts a specific commit), it lacks a clear explanation of why the revert is necessary.
    • What issue did the original commit introduce?
    • Why is reverting it the best solution?

Impact:

  • Not enough detail. The PR states the impact is on "ostest", but it needs to be more specific.
    • Which specific functionality within ostest is affected?
    • How does reverting the commit address the issue?

Testing:

  • Insufficient information: "CI" is too vague.
    • Which CI systems were used?
    • What specific tests were run?
    • The provided logs are from the failing test case and don't demonstrate that the revert actually resolves the issue. Provide logs from the CI run showing that the tests now pass.

To improve this PR:

  1. Expand the Summary: Explain the rationale behind the revert. What problem did the original commit cause, and why is reverting the preferred solution?
  2. Detail the Impact: Specify which part of "ostest" is affected and how reverting the commit resolves the issue.
  3. Provide Complete Testing Information:
    • List the specific CI systems used.
    • Describe the tests that were run to validate the revert.
    • Crucially, include logs from a successful CI run after the revert to demonstrate the fix.

By addressing these points, the PR will be clearer, more informative, and better adhere to the NuttX contribution guidelines.

@GUIDINGLI GUIDINGLI merged commit 0e8a6ee into apache:master Oct 14, 2024
15 of 36 checks passed
@realprocrastinator
Copy link

realprocrastinator commented Oct 15, 2024

@GUIDINGLI Hi Guiding, the PR 63542f8 has dependency of the other one apache/nuttx#14152. If we merge the test case in the former before the bug fixes, it will result in a testing failure. So, it is expected that ostest is complaining.

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