[PN-68] Refactor _robot_command_goal_complete

[abaker-bdai]

Merge #179 before merging this PR.

Adds unit test for _robot_command_goal_complete and refactors the function so that it is easier to read.

While writing the test, I spotted a few statuses that potentially return the incorrect value. I'll implement some of these test cases in a follow up PR.

[jbarry-bdai]

I've just made comments on the test because I suggest doing this PR in three pieces:

1. Only commit the test to reflect the current behavior of the function (but not the changes to the function included in this PR).
2. Commit a refactor to the function that behaves exactly as the old function did - confirmed by the test committed in 1
3. Commit changes to the refactor that fix the bugs discovered by the test.

This PR combines 1 and 2 - I suggest splitting it into two PRs.

Of the comments I made here, only the ones relevant to the actual test file (one typo, one place where I think a test is missing) should be updated in this PR. The others should wait for the third step where we fix the implementation.

Thanks for this!

[abaker-bdai]

I've just made comments on the test because I suggest doing this PR in three pieces:

1. Only commit the test to reflect the current behavior of the function (but not the changes to the function included in this PR).
2. Commit a refactor to the function that behaves exactly as the old function did - confirmed by the test committed in 1
3. Commit changes to the refactor that fix the bugs discovered by the test.

This PR combines 1 and 2 - I suggest splitting it into two PRs.

Of the comments I made here, only the ones relevant to the actual test file (one typo, one place where I think a test is missing) should be updated in this PR. The others should wait for the third step where we fix the implementation.

Thanks for this!

So the idea is that if you return None, the synchronous commands will return GoalResponse.SUCCESS later on, as long as no other commands return GoalResponse.IN_PROGRESS or GoalResponse.FAILED. What I can do is revert the test refactor to the initial test that I wrote to capture the pre-refactor state of the function. In a follow up PR, I'll address the bugs and potentially refactor the tests so that they are more thorough.

[jbarry-bdai]

I think we should commit the test first and separately from changes to spot ros2.

[abaker-bdai]

@jbarry-bdai Now that #179 has been merged into this branch, this is ready for review/stamp.

[jbarry-bdai]

Not all the way through but sending what I have so far.

[jbarry-bdai]

I think my only substantive comment is the confusion over _process functions not returning success.

[abaker-bdai]

@amessing-bdai so it would be easier to review, I added the new feedback statuses and updated the tests here: #204

[jbarry-bdai]

I think a little doc string on these functions to just say they return IN_PROGRESS, FAILURE, or NONE would be helpful