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

correct exit code assertion #747

Merged
merged 1 commit into from
Apr 22, 2021
Merged

correct exit code assertion #747

merged 1 commit into from
Apr 22, 2021

Conversation

Karsten1987
Copy link
Collaborator

fixes #746

as described in the linked issue, the exit code should rather be EXIT_FAILURE than EXIT_SUCCESS.

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987 Karsten1987 requested a review from a team as a code owner April 21, 2021 22:25
@Karsten1987 Karsten1987 requested review from mjeronimo and david-prody and removed request for a team April 21, 2021 22:25
@Karsten1987
Copy link
Collaborator Author

once ci is available again, I want to run the rosbag2_tests package without the xfail label to see if that fix recovers already the record_end_to_end test.

@Karsten1987 Karsten1987 merged commit 06bc383 into master Apr 22, 2021
@delete-merged-branch delete-merged-branch bot deleted the kk/fix_746 branch April 22, 2021 15:08
@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Apr 30, 2021

@Karsten1987 Sorry that I am writing in closed PR.
Could you please clarify how or why you come to the conclusion that it should be EXIT_SUCCESS rather than EXIT_FAILURE?

The reason why I am asking is because I am getting failure on my development branch in this test exactly at the same place where you changed expectation.
I have tried to make a brief analysis and come to conclusion that it should be EXIT_FAILURE.
I might be overlook something, but here is my thoughts:

  1. In this test we are expecting to observe message "Could not find converter for format some_rmw" in the output.
  2. This message only appears as text in throwing exception in https://github.com/ros2/rosbag2/blob/master/rosbag2_cpp/src/rosbag2_cpp/converter.cpp#L49-L56
  3. I've tried to trace this exception up to the stack and seems we are not handling it anywhere. Please correct me if I am wrong. But if we are not handling this exception we should expect EXIT_FAILURE result in this case.

It seems first time we are creating format converter in void SequentialReader::check_converter_serialization_format
which we are calling from void SequentialReader::open(..)
First time we are calling reader_->open(storage_options_, {"", rmw_get_serialization_format()}); in constructor of the player class which we invoke in rosbag2_py::Player::play() which is finally invokes from ros2bag verb play.py interface

None of them have try catch clause on the back trace.

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Apr 30, 2021

@Karsten1987 I apologies, I messed up. You made it right - you changed EXIT_SUCCESS to the EXIT_FAILURE
and this is exactly what I am expecting to observe. Sorry my bad.

Update: I have created PR #763 for my failing test. Apparently it turns out that we have two tests with similar names play_fails_gracefully_if_needed_coverter_plugin_does_not_exist and both of them has to be fixed for return code expectation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

record end to end test has wrong expectancy for wrong rmw format.
3 participants