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 expectation for exit code in play_end_to_end test since after… #763

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov commented Apr 30, 2021

We have failing PlayEndToEndTestFixture, play_fails_gracefully_if_needed_coverter_plugin_does_not_exist test on CI.

I have tried to make a brief analysis and come to conclusion that it should be expectation for EXIT_FAILURE at

EXPECT_THAT(exit_code, Eq(EXIT_SUCCESS));

https://github.com/ros2/rosbag2/blob/master/rosbag2_tests/test/rosbag2_tests/test_rosbag2_play_end_to_end.cpp#L118.

  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. 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.
The reason why we started to getting this test fail is because we made some redesign inside Player class and now we are calling in constructor reader_->open(storage_options_, {"", rmw_get_serialization_format()});
Before redesign we were calling this reader_->open(..) in Player::play(..) method only where it was wrapped with try-catch statement.

I think it will be incorrect to wrap call

reader_->open(storage_options_, {"", rmw_get_serialization_format()});

with try-catch statement in constructor. Therefore I am changing expectation in unit test.

… redesign we are getting exception in constructor.

Signed-off-by: Michael Orlov <morlovmr@gmail.com>
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner April 30, 2021 08:04
@MichaelOrlov MichaelOrlov requested review from emersonknapp and piraka9011 and removed request for a team April 30, 2021 08:04
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

lgtm with green CI

@MichaelOrlov
Copy link
Contributor Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@MichaelOrlov MichaelOrlov merged commit dd50cbb into ros2:master Apr 30, 2021
@MichaelOrlov MichaelOrlov deleted the michaelorlov/correct_exit_code_expectation_for_player branch April 30, 2021 19:26
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.

None yet

2 participants