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

drivertest_uart: Pass in the specified parameters to test #3022

Merged
merged 1 commit into from
Mar 26, 2025

Conversation

Zhangshoukui
Copy link
Contributor

Summary

drivertest_uart: Pass in the specified parameters to test

https://github.com/apache/nuttx/blob/master/tools/ci/testrun/script/test_framework/test_cmocka.py
cmocka --skip test_case_posix_timer|test_case_oneshot|write_default|read_default|burst_test|gpiotest01

In the CI of the community, this test is skipped because this test will block the terminal, and I think that the test added to cmocka should not be designed this way

Impact

drivertest_uart

Testing

local test

@nuttxpr
Copy link

nuttxpr commented Mar 7, 2025

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. Here's why and how to fix it:

Missing Information:

  • Summary:

    • Why is the change necessary? You mention the test blocks the terminal in CI, but why is passing parameters the solution? Explain the root problem and how this change addresses it. Is it to allow control over blocking behavior? To enable more specific testing scenarios?
    • What functional part of the code is being changed? You mention drivertest_uart and a Python test script. Be more specific. Which C files are modified? Are you changing the uart driver itself, the test application, or the test runner script?
    • How does the change exactly work? What parameters are being passed? How are they used within the drivertest_uart code? Provide examples.
  • Impact:

    • You only mention drivertest_uart. Address all the impact points:
      • New/Changed Feature: Describe the change in functionality.
      • Impact on User: Will users of the drivertest_uart tool need to do anything differently?
      • Impact on Build: Will the build process change (new dependencies, configuration options, etc.)?
      • Impact on Hardware: Does this change affect specific architectures, boards, or drivers?
      • Impact on Documentation: Does the documentation need updating? If so, have you included the updates in the PR?
      • Impact on Security: Are there any security implications?
      • Impact on Compatibility: Are there backward compatibility concerns? Will this break existing tests or user code?
  • Testing:

    • Build Host(s): Provide details about your development environment (OS, CPU architecture, compiler version).
    • Target(s): Specify the architecture(s) and board/configuration(s) you tested on (e.g., sim:qemu-x86_64, arm:stm32f4discovery).
    • Testing logs: You say "local test," which is insufficient. Include actual log output from the tests, both before and after your change. Demonstrate that the change fixes the problem and doesn't introduce new issues. If the output is extensive, consider using a service like gist.github.com and linking to it.

Example of Improved PR Description:

## Summary

The `drivertest_uart` test currently blocks the terminal during CI execution, causing it to be skipped in the automated test suite. This PR addresses this issue by passing parameters to `drivertest_uart` to control its blocking behavior.  Specifically, a new `--non-blocking` option is added to the test application, allowing it to run without halting the CI process. This change modifies the `drivers/serial/drivertest_uart.c` file and the CI script located at `tools/ci/testrun/script/test_framework/test_cmocka.py`.

The `test_cmocka.py` script is updated to pass the `--non-blocking` parameter to `drivertest_uart` when running under CI. Within `drivertest_uart.c`, this parameter disables the blocking read calls, allowing the test to complete without user interaction.

Related NuttX Issue: #1234 (if applicable)

## Impact

* **New/Changed Feature:** Adds a `--non-blocking` option to `drivertest_uart` to prevent blocking in automated testing environments.
* **Impact on User:** Users running `drivertest_uart` manually will not be affected unless they explicitly use the new `--non-blocking` option.
* **Impact on Build:** No changes to the build process.
* **Impact on Hardware:** No impact on specific hardware.
* **Impact on Documentation:** The `drivertest_uart` help text is updated to document the new `--non-blocking` option. The updated help text is included in this PR.
* **Impact on Security:** No security implications.
* **Impact on Compatibility:** Backward compatible. Existing usage of `drivertest_uart` is unaffected.

## Testing

I confirm that changes are verified on a local setup and work as intended:

* **Build Host(s):** Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
* **Target(s):** sim:qemu-x86_64, arm:stm32f4discovery:default

Testing logs before change:

(Output showing the test being skipped or hanging)


Testing logs after change:

(Output showing the test completing successfully in non-blocking mode)

By providing more detail and addressing all the required points, you significantly increase the chances of your PR being reviewed and merged quickly. Remember to replace the example information with the specifics of your actual change.

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

@lupyuen
Copy link
Member

lupyuen commented Mar 24, 2025

Sorry @Zhangshoukui could you fill in the Commit Message for this commit? Just copy from the PR Summary. Thanks :-)

https://github.com/apache/nuttx/blob/master/tools/ci/testrun/script/test_framework/test_cmocka.py
cmocka --skip test_case_posix_timer|test_case_oneshot|write_default|read_default|burst_test|gpiotest01

In the CI of the community, this test is skipped because this test will block the terminal, and I think that the test added to cmocka should not be designed this way

Signed-off-by: zhangshoukui <[email protected]>
@Zhangshoukui
Copy link
Contributor Author

Sorry @Zhangshoukui could you fill in the Commit Message for this commit? Just copy from the PR Summary. Thanks :-)

done

@lupyuen lupyuen merged commit 7b65b2b into apache:master Mar 26, 2025
39 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.

6 participants