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 himem_test,mtetest and x86-64-ABI folders to the new arch folder #2956

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 arch folder and move himem_test,mtetest and x86-64-ABI folders to the arch folder.

Impact

himem_test,mtetest and x86-64-ABI folders.
new arch folder.

Testing

local test.

@nuttxpr
Copy link

nuttxpr commented Jan 20, 2025

[Experimental Bot, please feedback here]

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

  • Insufficient Summary: "one step that merge test case" is vague. Explain why this restructuring is necessary. What problem does it solve? Is it part of a larger effort? What are the benefits of moving these folders? Be specific about the functional changes.

  • Missing Issue References: Are there related NuttX or NuttX Apps issues that this PR addresses? If so, provide links.

  • Incomplete Impact Assessment: Simply listing the changed folders isn't enough. Address all the impact points:

    • Is new feature added? Is existing feature changed? (Likely "No" in this case, but clarify.)
    • Impact on user: Will users need to change anything in their workflow or configuration?
    • Impact on build: Will build scripts or Makefiles need to be updated? Almost certainly yes, given the folder move. Describe the changes.
    • Impact on hardware: Likely no, but state it explicitly.
    • Impact on documentation: Does documentation need updating to reflect the new folder structure? If so, has it been updated in this PR or a separate one?
    • Impact on security/compatibility: Likely no, but explicitly state it.
  • Insufficient Testing Information: "local test" is far too vague.

    • Build Host Details: Specify the OS, CPU architecture, and compiler version used for testing.
    • Target Details: Which architecture and board configuration were used? Was this tested on real hardware or a simulator? Be specific (e.g., qemu-x86_64:nsh).
    • Missing Logs: Provide actual log output before and after the change. This demonstrates that the tests still pass after the restructuring. Show that the moved test cases still execute and produce the expected results.

In short, the PR needs to be much more thorough and explicit in addressing all the required points. Provide the missing context and details to make it clear why this change is beneficial and that it has been properly tested.

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

@txy-21 txy-21 force-pushed the merge-arch-folder branch from 727de12 to 57c56e5 Compare January 20, 2025 06:49
@xiaoxiang781216 xiaoxiang781216 merged commit bdb0fcd 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.

4 participants