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

apps/testing: move cxxsize, cxxtest and uclibcxx_test folders to the new cxx folder #2959

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

txy-21
Copy link
Contributor

@txy-21 txy-21 commented Jan 20, 2025

Summary

This is one step that merge test case as discussed in #2931.
Create cxx folder and move cxxsize, cxxtest and uclibcxx_test folders to the new cxx folder.

Impact

new cxx folder.
move three folders : cxxsize, cxxtest and uclibcxx_test

Testing

ci test.

@nuttxpr
Copy link

nuttxpr commented Jan 20, 2025

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. While it provides a basic summary and mentions testing, it lacks crucial details. Here's a breakdown of what's missing:

  • Summary: While it states the "what," it lacks the "why." Why is this restructuring necessary? Is it for better organization, future expansion, or some other reason? How exactly does moving these folders achieve that goal?

  • Impact: The impact section is too brief. It mentions the new folder and moved folders, but fails to address the specific points required. For all of the "Impact on..." items, explicit "YES" or "NO" answers are needed followed by descriptions where applicable. Even if the answer is "NO," explicitly stating so is essential for clarity. For example:

    • Impact on user: NO
    • Impact on build: YES (Build scripts and paths may need to be updated to reflect the new directory structure. This could affect users who have custom build configurations.)
    • Impact on hardware: NO
    • ... and so on for all other impact categories.
  • Testing: Saying "ci test" is insufficient. Provide specific details:

    • Build Host(s): Specify the operating system, CPU architecture, and compiler version used for testing. (e.g., Linux x86_64, GCC 12.2)
    • Target(s): Specify the target architecture, board, and configuration used for testing. (e.g., sim:nsh, stm32f4discovery:nsh)
    • Testing logs: Provide actual log snippets demonstrating the functionality before and after the change. Even if there's no functional change, show some output proving that the affected tests still run successfully in the new location. If there is expected output, show that.

In short, the PR description needs to be significantly more detailed and address all the required points explicitly. Providing more context and concrete evidence will make the review process much smoother and ensure the change is well-understood.

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 @txy-21 :-)

@xiaoxiang781216 xiaoxiang781216 merged commit 903fbe4 into apache:master Jan 20, 2025
37 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.

5 participants