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 irtest/sensortest/resmonitor/monkey to apps/system #2976

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 commented Jan 26, 2025

Summary

System utils relocation from apps/testing to apps/system as discussed in #2931 and #2961.

Impact

System wide testing utils relocation (apps/testing-> apps/system).

Testing

pass ci

@nuttxpr
Copy link

nuttxpr commented Jan 26, 2025

[Experimental Bot, please feedback here]

Summary

This PR appears to be missing crucial information required by the NuttX guidelines. The summary doesn't explain the why, what, or how of the change. It only mentions "testing folder restructure," which is insufficient. There's no mention of related issues or pull requests either.

Impact

While the impact section mentions "testing folder restructure," it fails to address the specific impact questions. Does this change add/modify features? Does it affect users, the build process, hardware, documentation, security, or compatibility? Simply saying "testing folder restructure" isn't helpful without context.

Testing

"Pass CI" is also insufficient. While CI passing is good, the PR needs to specify the build hosts and targets tested. It also needs to provide actual testing logs from before and after the change, not just a statement that the CI passed. Without these logs, reviewers cannot verify the impact of the changes.

In short, no, this PR does not meet the NuttX requirements. It needs significantly more detail in all sections to be considered complete.

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 @xiaoxiang781216 you are right these tools belong to apps/system :-)

Git probably wrongly detected filename changes, but the content is okay :-)

  • testing/sensortest/Make.defs -> system/resmonitor/Make.defs
  • testing/resmonitor/Make.defs -> system/sensortest/Make.defs

Lets just wait for the CI build test to complete :-)

@cederom
Copy link

cederom commented Jan 26, 2025

Build error, restarted failed tasks..

xiaoxiang781216 added a commit to xiaoxiang781216/incubator-nuttx that referenced this pull request Jan 26, 2025
@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Jan 26, 2025

Build error, restarted failed tasks..

@cederom need merge with apache/nuttx#15698 together.

@cederom cederom requested a review from lupyuen January 26, 2025 09:09
@cederom
Copy link

cederom commented Jan 26, 2025

@cederom: Build error, restarted failed tasks..

@xiaoxiang781216: @cederom need merge with apache/nuttx#15698 together.

Hmm, can we please add commit from apache/nuttx#15698 here so we have clean build here? It seems to be part of this PR :-)

@xiaoxiang781216
Copy link
Contributor Author

@cederom: Build error, restarted failed tasks..

@xiaoxiang781216: @cederom need merge with apache/nuttx#15698 together.

Hmm, can we please add commit from apache/nuttx#15698 here so we have clean build here? It seems to be part of this PR :-)

it's impossible to include patch from the different git in one pr.

@cederom
Copy link

cederom commented Jan 27, 2025

Braindead, sorry, long time no sleep!!! :D :D

cederom pushed a commit to apache/nuttx that referenced this pull request Jan 27, 2025
@cederom
Copy link

cederom commented Jan 27, 2025

Merging together with apache/nuttx#15698 as instructed :-)

@cederom cederom merged commit 85a6aaa into apache:master Jan 27, 2025
16 of 37 checks passed
@xiaoxiang781216 xiaoxiang781216 deleted the reorg2 branch January 27, 2025 02:59
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