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 sd_bench to apps/bechmarks #2977

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 commented Jan 26, 2025

Summary

Benchmark utils relocation from apps/testing to apps/benchmarks as discussed in #2931 and #2961.

Impact

Benchmarks relocation (apps/testing -> apps/benchmarks).

Testing

pass ci

@nuttxpr
Copy link

nuttxpr commented Jan 26, 2025

[Experimental Bot, please feedback here]

This PR likely does NOT meet the NuttX requirements, despite claiming to pass CI.

Summary

The provided summary is insufficient. It lacks crucial information like: why the apps/testing reorganization is necessary, what exactly changed within the apps/testing directory, and how the reorganization works. Simply stating "apps/testing reorg" doesn't explain the purpose or the technical details of the change. Issue/PR references are also missing.

Impact

While "apps/testing reorg" hints at an impact on the apps, it doesn't adequately address the required details. The PR author needs to specify YES/NO for each impact category and provide descriptions where applicable. For example, even if user impact is NO, they should explicitly state NO. For a reorganization, documentation impact is likely YES, requiring explanation of the new structure. Build impact might also be YES if build scripts or dependencies changed. The current response ignores these details.

Testing

"pass ci" is inadequate. While CI passing is good, the requirement is to demonstrate local verification before submitting the PR. This means specifying the host and target environments and providing actual log output demonstrating the issue before the change and the successful operation after the change. Just stating "pass ci" avoids demonstrating personal due diligence and makes it difficult to understand what was actually tested.

In short, this PR needs substantial improvement in providing the necessary information before it can be considered for merging.

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

@cederom cederom requested review from raiden00pl and lupyuen January 26, 2025 08:21
@cederom cederom merged commit cf46792 into apache:master Jan 27, 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