Skip to content

Add stomp target link to moveit_planners_stomp tests #3437

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

Merged
merged 1 commit into from
Apr 14, 2025

Conversation

Bckempa
Copy link
Contributor

@Bckempa Bckempa commented Apr 14, 2025

Description

Fix missing linkage in moveit_planners_stomp tests via target_link_libraries declaration as suggested in #3436 (comment)

Fixes #3436

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@Bckempa
Copy link
Contributor Author

Bckempa commented Apr 14, 2025

Running clang-format on my machine totally mangled the existing CMakeLists.txt which I presume is a configuration error in my environment so I skipped that step of the checklist and the rest don't seem relevant to this PR.

I am testing that this resolves the Space ROS build issue and request a backport to jazzy if approved.

@sjahr
Copy link
Contributor

sjahr commented Apr 14, 2025

Running clang-format on my machine totally mangled the existing CMakeLists.txt which I presume is a configuration error in my environment so I skipped that step of the checklist and the rest don't seem relevant to this PR.

I am testing that this resolves the Space ROS build issue and request a backport to jazzy if approved.

Perfect, thanks!

Copy link

codecov bot commented Apr 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.07%. Comparing base (64e934f) to head (c8a9b32).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3437      +/-   ##
==========================================
- Coverage   45.87%   45.07%   -0.79%     
==========================================
  Files         717      717              
  Lines       62636    62639       +3     
  Branches     7581     7586       +5     
==========================================
- Hits        28728    28230     -498     
- Misses      33742    34240     +498     
- Partials      166      169       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Bckempa Bckempa marked this pull request as ready for review April 14, 2025 16:18
@Bckempa
Copy link
Contributor Author

Bckempa commented Apr 14, 2025

Confirmed this fixes the issue with the Space ROS build space-ros/docker#224

@EzraBrooks EzraBrooks merged commit ec5b368 into moveit:main Apr 14, 2025
10 checks passed
@github-project-automation github-project-automation bot moved this to ✅ Done in MoveIt Apr 14, 2025
@Bckempa Bckempa deleted the bugfix/stomp-test-include branch April 14, 2025 16:47
@Bckempa
Copy link
Contributor Author

Bckempa commented Apr 14, 2025

Thanks for the quick-turnaround! Can we expect this change to be back ported to the jazzy branch or do we need to retarget our build to accommodate?

@EzraBrooks EzraBrooks added the backport-jazzy Mergify label that triggers a PR backport to Jazzy label Apr 14, 2025
mergify bot pushed a commit that referenced this pull request Apr 14, 2025
sea-bass pushed a commit that referenced this pull request Apr 14, 2025
sussybot5258 pushed a commit to GreyCatAI/moveit2 that referenced this pull request Apr 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-jazzy Mergify label that triggers a PR backport to Jazzy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Dependency in Stomp Test Build
3 participants