Update controller tests to use configure/activate instead of on_configure/on_activate

[jsantoso91]

Fix for #1660 and follow up to #1658.

I created helper methods to call configure()/activate() and check expected state (for success and failure) afterwards.
Further replace instances of on_configure() and on_activate() with the said methods.

===

Contributions via pull requests are much appreciated. Before sending us a pull request, please ensure that:

1. Limited scope. Your PR should do one thing or one set of things. Avoid adding “random fixes” to PRs. Put those on separate PRs.
2. Give your PR a descriptive title. Add a short summary, if required.
3. Make sure the pipeline is green.
4. Don’t be afraid to request reviews from maintainers.
5. New code = new tests. If you are adding new functionality, always make sure to add some tests exercising the code and serving as live documentation of your original intention.

To send us a pull request, please:

• Fork the repository.
• Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
• Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
• Commit to your fork using clear commit messages.
• Send a pull request, answering any default questions in the pull request interface.
• Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

[mergify]

This pull request is in conflict. Could you fix it @jsantoso91?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.26%. Comparing base (30473a5) to head (6bb374f).
⚠️ Report is 54 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1682      +/-   ##
==========================================
+ Coverage   85.25%   85.26%   +0.01%     
==========================================
  Files         143      143              
  Lines       13794    13794              
  Branches     1194     1194              
==========================================
+ Hits        11760    11762       +2     
+ Misses       1636     1633       -3     
- Partials      398      399       +1     

Flag	Coverage Δ
unittests	85.26% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ive_controller/test/test_diff_drive_controller.cpp	94.49% <100.00%> (-0.10%)	⬇️
..._controllers/test/test_gpio_command_controller.cpp	99.02% <100.00%> (+0.01%)	⬆️
...r_broadcaster/test/test_gps_sensor_broadcaster.cpp	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jsantoso91]

I am looking for feedback before proceeding to edit other tests with similar pattern.

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

[jsantoso91]

Hello, the test results look clean, could I get this PR reviewed? @christophfroehlich @bmagyar
Thanks!

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

[mergify]

This pull request is in conflict. Could you fix it @jsantoso91?

[christophfroehlich]

First of all, sorry for the very late review.
Second, thanks for picking this :)

The changes itself look great, I just ask to refactor it a bit:

• now we have a mixture of your new helpers and other transitions, where still the state is saved into a variable and then state.id() is evaluated.
• I suggest using the same pattern for every transition. Either
  • ASSERT_EQ(controller->configure().id(), State::PRIMARY_STATE_INACTIVE); (do we need the helpers, this is simple enough?)
  • Or use only one helper per transition, and use something like ASSERT_TRUE(configure_succeeds()) or ASSERT_FALSE(configure_succeeds()).

Are you still working on OSS/ROS and could finish this?

This could be rolled out to other controllers as well, but let's change it for now only in those you already have touched.