Skip to content

fix(velocity_smoother): missing unit tests for resampling and invalid subscribed input topic#562

Merged
go-sakayori merged 9 commits into
autowarefoundation:mainfrom
ralwing:test-velocity-smoother
Nov 17, 2025
Merged

fix(velocity_smoother): missing unit tests for resampling and invalid subscribed input topic#562
go-sakayori merged 9 commits into
autowarefoundation:mainfrom
ralwing:test-velocity-smoother

Conversation

@ralwing

@ralwing ralwing commented Jul 2, 2025

Copy link
Copy Markdown
Contributor

Description

A few error has been reported regarding the backwards driving in autoware. We'd like to contribute by fixing them.

Firstly to allow proceeding on these errors we need to fix and add some unit tests to confirm that the changes doesn't brake any other functionallties.

This PR fixes

  • the invalid topic monitored in node
  • missing resample unit tests

Related links

Parent Issue:
autowarefoundation/autoware_universe#5938

autowarefoundation/autoware_universe#5976

  • Link

How was this PR tested?

(ADDED by @sasakisasaki) Unit tests passed in CI

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

Copilot AI review requested due to automatic review settings July 2, 2025 10:14
@github-actions

github-actions Bot commented Jul 2, 2025

Copy link
Copy Markdown

Thank you for contributing to the Autoware project!

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

Please ensure:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an incorrect topic in the smoother node interface tests and adds comprehensive unit tests for trajectory resampling to prevent regressions.

  • Updated test topic strings from output/trajectory to input/trajectory
  • Introduced test_resample.cpp with extensive resampling scenarios
  • Ensures new tests cover velocity smoother functionality without breaking existing behavior

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
planning/autoware_velocity_smoother/test/test_velocity_smoother_node_interface.cpp Corrected the input trajectory topic name
planning/autoware_velocity_smoother/test/test_resample.cpp Added new unit tests for trajectory resampling
Comments suppressed due to low confidence (1)

planning/autoware_velocity_smoother/test/test_resample.cpp:126

  • Add a test case for backward (negative) velocities to verify that the resampler correctly handles reverse driving scenarios, since the PR aims to address backwards driving errors.
class TrajectoryResampleTest : public ::testing::Test

Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp Outdated
@ralwing ralwing changed the title Test-velocity-smoother fix(velocity_smoother): missing unit tests for resampling and invalid subscribed input topic Jul 2, 2025
@ralwing ralwing force-pushed the test-velocity-smoother branch 2 times, most recently from 69244d5 to 22e4111 Compare July 2, 2025 10:38

publishMandatoryTopics(test_manager, test_target_node);

const std::string input_trajectory_topic = "velocity_smoother/output/trajectory";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

here is the fix, the input topic published to the output

@ralwing ralwing force-pushed the test-velocity-smoother branch from 2942908 to c122748 Compare July 4, 2025 10:57
@sasakisasaki sasakisasaki added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jul 6, 2025
@codecov

codecov Bot commented Jul 6, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.86%. Comparing base (6dd42d4) to head (aa298b4).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #562      +/-   ##
==========================================
- Coverage   49.60%   48.86%   -0.75%     
==========================================
  Files         338      338              
  Lines       21271    23726    +2455     
  Branches     9694    10850    +1156     
==========================================
+ Hits        10552    11594    +1042     
- Misses       9743    11031    +1288     
- Partials      976     1101     +125     
Flag Coverage Δ *Carryforward flag
differential 33.40% <ø> (?)
total 49.62% <ø> (+0.02%) ⬆️ Carriedforward from a70136d

*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.

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

@stale

stale Bot commented Sep 5, 2025

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale Bot added the status:stale Inactive or outdated issues. (auto-assigned) label Sep 5, 2025
@ralwing

ralwing commented Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

@sasakisasaki friendly ping

@stale stale Bot removed the status:stale Inactive or outdated issues. (auto-assigned) label Sep 29, 2025
@sasakisasaki

Copy link
Copy Markdown
Contributor

@ralwing Sorry for being late to reply, let me do review soon (within a few hours) 🙏

@sasakisasaki sasakisasaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you very much for providing the tests. Nice PR 👍 Not only the code, and also the enriched comments with ASCII diagrams are super helpful for understanding.

First of all, let me put a quick comment from my side. Please feel free to let me know if there is any my misunderstanding and so on.

Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp
Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp
Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp
Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp
const double ds = std::max(
current_velocity * resample_param_.dense_resample_dt,
resample_param_.dense_min_interval_distance);
const int expected_min_points = static_cast<int>(input.back().pose.position.x / ds);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(Note for myself) ds would be > 0.5 as this line: resample_param_.dense_min_interval_distance = 0.5;

Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp Outdated
Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp
Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp Outdated
ralwing added a commit to ralwing/autoware.core that referenced this pull request Oct 6, 2025
@ralwing ralwing force-pushed the test-velocity-smoother branch from 7419cc5 to 9d5a66d Compare October 6, 2025 08:38
ralwing added a commit to ralwing/autoware.core that referenced this pull request Oct 6, 2025
Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
@ralwing ralwing force-pushed the test-velocity-smoother branch from cdca5b3 to bff625d Compare October 6, 2025 08:42
@ralwing ralwing requested a review from Copilot October 6, 2025 09:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
pre-commit-ci Bot and others added 4 commits October 6, 2025 11:12
Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
@ralwing ralwing force-pushed the test-velocity-smoother branch from 63b4afc to fc436af Compare October 6, 2025 09:12
@ralwing ralwing requested a review from Copilot October 6, 2025 09:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@ralwing

ralwing commented Oct 9, 2025

Copy link
Copy Markdown
Contributor Author

@sasakisasaki Could I kindly ask you to spare some time to review this PR? I believe it is now ready to be merged. I think it's good to go.

@sasakisasaki sasakisasaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks fine! I agree with merging this PR while I put a minor comment 👍

Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp
@sasakisasaki

Copy link
Copy Markdown
Contributor

@autowarefoundation/autoware-core-global-codeowners Review request 😉

Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp
Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp Outdated
Comment thread planning/autoware_velocity_smoother/test/test_resample.cpp Outdated
Signed-off-by: Grzegorz Głowacki <gglowacki@autonomous-systems.pl>
@ralwing

ralwing commented Nov 17, 2025

Copy link
Copy Markdown
Contributor Author

@go-sakayori kindly ping :-)

@go-sakayori go-sakayori enabled auto-merge (squash) November 17, 2025 09:12
@go-sakayori go-sakayori merged commit 23c9806 into autowarefoundation:main Nov 17, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants