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

Fix Topic Info Test with "Infinite" printing #616

Merged
merged 2 commits into from
May 6, 2021

Conversation

emersonknapp
Copy link
Contributor

Connected to ros2/rclpy#722

Fixes the output tests given the new printing info.

@emersonknapp
Copy link
Contributor Author

@ivanpauno One a side note - do you know a better way to debug when launch_testing.tools.expect_output is failing? I ended up having to write my own little loop in the test to figure out what line was the problem, it looked like:

actual_lines = topic_command.output.splitlines()
for expected_line, actual_line in zip(expected_lines, actual_lines):
    if hasattr(expected_line, 'match'):
        assert expected_line.match(actual_line)
    else:
        assert expected_line == actual_line

(
additionally, do you know if it is possible to isolate these tests? the following selects nothing

colcon test --packages-select ros2topic --event-handlers console_direct+ --pytest-args -k test_topic_endpoint_info_verbose

The only option I could get to work was -k test_cli which only narrows it down to that whole source file, which still takes potentially several minutes to run
)

None of the above is important - this should fix the tests, but if you have insights on the development workflow I would be really happy to hear it.

@emersonknapp
Copy link
Contributor Author

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

@ivanpauno
Copy link
Member

@ivanpauno One a side note - do you know a better way to debug when launch_testing.tools.expect_output is failing? I ended up having to write my own little loop in the test to figure out what line was the problem, it looked like:

No, I always modify the test to manually capture the output and print it, or sth like that.
It would be good to improve launch_testing.tools.expect_output so it provides a more useful error message.

colcon test --packages-select ros2topic --event-handlers console_direct+ --pytest-args -k test_topic_endpoint_info_verbose

For cmake packages I use --cmake-args -R <regex_expression_tests_to_run>.
For python I typically go to the test folder and use python -m pytest -s <test_name> ...

Unfortunately for launch_testing tests, I don't think there's a way to only run some of the test cases.
Based on the pytest output:

collected 1 item / 1 deselected

it seems that launch_testing collects everything in one test case (?).

@hidmic may know better.

Copy link
Member

@ivanpauno ivanpauno 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

@ivanpauno
Copy link
Member

CI results are here.
Rpr checker is expected to fail until ros2/rclpy#722 gets in (and a new rclpy release is done).

@emersonknapp emersonknapp force-pushed the emersonknapp/fix-topic-info-test branch from 9290b44 to 34ff19a Compare April 5, 2021 18:28
@hidmic
Copy link
Contributor

hidmic commented Apr 16, 2021

It would be good to improve launch_testing.tools.expect_output so it provides a more useful error message.

Agreed. It's not the first time someone has complained about this --> ros2/system_tests#404

Unfortunately for launch_testing tests, I don't think there's a way to only run some of the test cases.

There isn't. launch_testing doesn't fully integrate with pytest. I cannot: ros2/launch#405

Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@audrow audrow self-assigned this Apr 16, 2021
Emerson Knapp added 2 commits April 16, 2021 11:19
Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp emersonknapp force-pushed the emersonknapp/fix-topic-info-test branch from 34ff19a to ecb4143 Compare April 16, 2021 18:20
@ivanpauno ivanpauno merged commit 04397d9 into ros2:master May 6, 2021
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.

4 participants