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/drivertest_block.c: Fix cmoka driver block test error #2693

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

wyr-7
Copy link
Contributor

@wyr-7 wyr-7 commented Oct 11, 2024

Summary

testing/drivertest_block.c: Fix cmoka driver block test error

Impact

  1. Fix the problem that pre->source may have memory stampede
  2. Fix total block size larger than cache size condition

Testing

Test in Vela

@nuttxpr
Copy link

nuttxpr commented Oct 11, 2024

[Experimental Bot, please feedback here]

This pull request appears to partially meet the NuttX requirements. Here's a breakdown:

Strengths:

  • Summary: Provides a clear and concise description of the issue addressed (memory stampede, block size exceeding cache size) and the file affected (testing/drivertest_block.c).
  • Impact: Clearly outlines the functional impact of the changes (fixing errors).
  • Testing: Indicates the testing environment (Vela), but lacks specific details.

Areas for improvement:

  • Summary:
    • Lacks a related issue reference if this PR addresses a known bug.
  • Impact:
    • The impact section is very brief. While it mentions fixes, it lacks details about potential user impact, build process changes, hardware dependencies, or documentation requirements.
    • All other impact points are not addressed. While they may not be relevant, explicitly stating "NO" with a brief justification (e.g., "No impact on build process as changes are isolated to testing code") would provide a more thorough assessment.
  • Testing:
    • Crucially, it lacks testing logs before and after the change. These logs are essential to demonstrate the actual issue and the effectiveness of the fix.

Recommendation:

To fully meet the NuttX requirements, the PR should include:

  1. Related Issue: If applicable, link the corresponding NuttX issue that this PR addresses.
  2. Detailed Impact Assessment: Address all impact points, even if the answer is "NO." Briefly explain why there is no impact for each point.
  3. Complete Testing Information: Provide the complete testing logs before and after the change to clearly demonstrate the fix. This includes:
    • Build host details: OS, CPU architecture, compiler version.
    • Target details: Architecture (e.g., sim, RISC-V, ARM), board configuration.
    • Relevant log snippets showing the error before the fix and the corrected behavior after.

By addressing these points, the PR will be more comprehensive and easier for reviewers to assess.

Copy link

@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 @wyr-7 :-) Build errors reveal minor tweaks are required :-)

1. Fix the problem that pre->source may have memory stampede
2. Fix total block size larger than cache size condition

Signed-off-by: Yongrong Wang <wangyongrong@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit a486129 into apache:master Oct 15, 2024
36 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