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

feat(autoware_test_utils): porting autoware_test_utils from universe to core #172

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

JianKangEgon
Copy link

Description

We are porting autoware_test_utils to autoware.core, and this PR adds the package to core.

Related links

Parent Issue:

  • Link

How was this PR tested?

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

Copy link

github-actions bot commented Jan 18, 2025

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@JianKangEgon
Copy link
Author

This pr depends on the new ported package "autoware_utils": autowarefoundation/autoware_utils#23 and "autoware_internal_msgs": autowarefoundation/autoware_internal_msgs#45

Copy link

@mounireom1 mounireom1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jncr gen

Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this package is only used during testing, as its name suggests, it shouldn’t be in the common directory. I think it would be better to create a tests directory and move it there just like the demos directory.

common/autoware_test_utils/CHANGELOG.rst Outdated Show resolved Hide resolved
common/autoware_test_utils/package.xml Outdated Show resolved Hide resolved
<depend>autoware_map_msgs</depend>
<depend>autoware_perception_msgs</depend>
<depend>autoware_planning_msgs</depend>
<depend>autoware_pyplot</depend>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't migrate this package yet and I think we don't need to do it because it's for the visualization. cc @mitsudome-r

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Kondo san @youtalk

Thanks for reviewing this pr
Already revised the package according to your comment,

Best regards

心刚

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#172 (comment)

Please remove the usages too.

Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 37.78071% with 471 lines in your changes missing coverage. Please review.

Project coverage is 46.10%. Comparing base (4cb18f5) to head (2e54b04).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
...on/autoware_test_utils/src/autoware_test_utils.cpp 4.73% 161 Missing ⚠️
...autoware_test_utils/test/test_mock_data_parser.cpp 11.11% 3 Missing and 157 partials ⚠️
...n/autoware_test_utils/src/topic_snapshot_saver.cpp 0.00% 83 Missing ⚠️
...ommon/autoware_test_utils/src/mock_data_parser.cpp 78.18% 47 Missing and 13 partials ⚠️
...nclude/autoware_test_utils/autoware_test_utils.hpp 83.33% 1 Missing and 4 partials ⚠️
...are_test_utils/test/test_autoware_test_manager.cpp 90.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #172       +/-   ##
===========================================
- Coverage   78.75%   46.10%   -32.66%     
===========================================
  Files          11       17        +6     
  Lines         193      950      +757     
  Branches       73      529      +456     
===========================================
+ Hits          152      438      +286     
- Misses         11      306      +295     
- Partials       30      206      +176     
Flag Coverage Δ *Carryforward flag
differential 37.78% <37.78%> (?)
total 78.75% <ø> (ø) Carriedforward from 4cb18f5

*This pull request uses carry forward flags. Click here to find out more.

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

Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#172 (review)

If this package is only used during testing, as its name suggests, it shouldn’t be in the common directory. I think it would be better to create a tests directory and move it there just like the demos directory.

@JianKangEgon Please move directory according to my comment.

Comment on lines +22 to +29
autoware_lanelet2_extension:
type: git
url: https://github.com/autowarefoundation/autoware_lanelet2_extension
version: main
autoware_msgs:
type: git
url: https://github.com/autowarefoundation/autoware_msgs
version: main
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are duplicated.

Suggested change
autoware_lanelet2_extension:
type: git
url: https://github.com/autowarefoundation/autoware_lanelet2_extension
version: main
autoware_msgs:
type: git
url: https://github.com/autowarefoundation/autoware_msgs
version: main

Comment on lines +20 to +21
#include <autoware/pyplot/patches.hpp>
#include <autoware/pyplot/pyplot.hpp>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to remove them and the usages too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: To Triage
Development

Successfully merging this pull request may close these issues.

6 participants