Conversation
83e4830 to
1dbe6bb
Compare
|
Force pushed white-space fix for uncrustify error. |
|
I'm still looking over |
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
1dbe6bb to
5a3ffba
Compare
|
Addressed PR feedback and rerunning tests:
|
|
Look like |
8c08084 to
99787a6
Compare
|
I think I took care of the memory error. Checking again:
|
Signed-off-by: Stephen Brawner <brawner@gmail.com>
99787a6 to
4b0497b
Compare
|
About 20 lines short (out of 1030) to reach 95% coverage. Guess that will have to do for now. The automated build.ros2.org check failed when I pushed the most recent commit because the nodes in rcl-action interfered with rcl's tests. Not sure why since they are run sequentially. I force pushed again and it passed. Hopefully this doesn't show up regularly in nightlies. |
|
This introduced 11 new build warnings in nightlies https://ci.ros2.org/view/nightly/job/nightly_linux_release/1557/gcc/new/ |
| return GOAL_STATE_UNKNOWN; | ||
| } | ||
| assert(GOAL_STATE_ACCEPTED == state || GOAL_STATE_EXECUTING == state); | ||
| assert(GOAL_EVENT_CANCEL_GOAL == event); |
There was a problem hiding this comment.
assert is compiled out in release mode, that's why the new warnings.
why a PR titled "Increase test coverage ..." is changing behavior?
If a behavior change was needed, that should've been part of a different PR (or at least, commits shouldn't have been squashed)
|
I’ll submit a patch soon. This doesn’t actually change behavior, the if statements were checking conditions which were impossible based on logic elsewhere.
… On May 29, 2020, at 7:07 AM, Ivan Santiago Paunovic ***@***.***> wrote:
@ivanpauno commented on this pull request.
In rcl_action/src/rcl_action/goal_state_machine.c:
> return GOAL_STATE_EXECUTING;
}
rcl_action_goal_state_t
_cancel_goal_event_handler(rcl_action_goal_state_t state, rcl_action_goal_event_t event)
{
- if ((GOAL_STATE_ACCEPTED != state && GOAL_STATE_EXECUTING != state) ||
- GOAL_EVENT_CANCEL_GOAL != event)
- {
- return GOAL_STATE_UNKNOWN;
- }
+ assert(GOAL_STATE_ACCEPTED == state || GOAL_STATE_EXECUTING == state);
+ assert(GOAL_EVENT_CANCEL_GOAL == event);
assert is compiled out in release mode, that's why the new warnings.
why a PR titled "Increase test coverage ..." is changing behavior?
If a behavior change was needed, that should've been part of a different PR (or at least, commits shouldn't have been squashed)
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Sounds good to me then! |
|
Addressed in #666 |
This boosts the coverage of rcl_action tests.