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

wait_for_ouput() repr includes actual text #408

Merged
merged 2 commits into from
Apr 27, 2020

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Apr 27, 2020

This makes wait_for_output() return an object that still evaluates as
a bool, but when repr()'d it includes the actual text. This will help
debug a few test failures on the build farm caused by expected output
not being seen.

@jacobperron review-requested because I would like to get this into foxy to help debug test failures on the build farm, though I could see how this could be considered changing an API. I think it's safe because the returned object still evaluates True as False. Thoughts?

@sloretz sloretz self-assigned this Apr 27, 2020
This makes `wait_for_output()` return an object that still evaluates as
a bool, but when `repr()`'d it includes the actual text. This will help
debug a few test failures on the build farm caused by expected output
not being seeing.

Signed-off-by: Shane Loretz<sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz force-pushed the sloretz/wait_for_output_repr branch from 54a2b39 to 0d077f6 Compare April 27, 2020 18:27
@sloretz
Copy link
Contributor Author

sloretz commented Apr 27, 2020

Here's what this looks like in the pytest output. It shows most of the actual output in the traceback, not all because pytest truncates strings, but enough to be useful.

======================================================================
FAIL: test_topic_echo[rmw_fastrtps_cpp] (test_cli.TestROS2TopicCLI)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/sloretz/ws/ros2/build/launch_testing/launch_testing/markers.py", line 57, in _wrapper
    return func(self, *args, **kwargs)
  File "/home/sloretz/ws/ros2/src/ros2/ros2cli/ros2topic/test/test_cli.py", line 367, in test_topic_echo
    assert topic_command.wait_for_output(functools.partial(
AssertionError: assert <BoolWithText(False): "data: 'Hello World: 90'\n---\ndata: 'Hello World: 91'\n---\ndata: 'Hello World: 92'\n---\ndata:...a: 'Hello World: 96'\n---\ndata: 'Hello World: 97'\n---\ndata: 'Hello World: 98'\n---\ndata: 'Hello World: 99'\n---\n">
 +  where <BoolWithText(False): "data: 'Hello World: 90'\n---\ndata: 'Hello World: 91'\n---\ndata: 'Hello World: 92'\n---\ndata:...a: 'Hello World: 96'\n---\ndata: 'Hello World: 97'\n---\ndata: 'Hello World: 98'\n---\ndata: 'Hello World: 99'\n---\n"> = <bound method ProcessProxy.wait_for_output of <launch_testing.tools.process.ProcessProxy object at 0x7fc77c9ebf70>>(functools.partial(<function expect_output at 0x7fc7801b24c0>, expected_lines=[re.compile("data: 'Helloxxxx World: \\d+'"), '---'], strict=True), timeout=10)
 +    where <bound method ProcessProxy.wait_for_output of <launch_testing.tools.process.ProcessProxy object at 0x7fc77c9ebf70>> = <launch_testing.tools.process.ProcessProxy object at 0x7fc77c9ebf70>.wait_for_output
 +    and   functools.partial(<function expect_output at 0x7fc7801b24c0>, expected_lines=[re.compile("data: 'Helloxxxx World: \\d+'"), '---'], strict=True) = <class 'functools.partial'>(<function expect_output at 0x7fc7801b24c0>, expected_lines=[re.compile("data: 'Helloxxxx World: \\d+'"), '---'], strict=True)
 +      where <class 'functools.partial'> = functools.partial
 +      and   <function expect_output at 0x7fc7801b24c0> = <module 'launch_testing.tools' from '/home/sloretz/ws/ros2/build/launch_testing/launch_testing/tools/__init__.py'>.expect_output
 +        where <module 'launch_testing.tools' from '/home/sloretz/ws/ros2/build/launch_testing/launch_testing/tools/__init__.py'> = launch_testing.tools

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I guess this should be unlikely to break downstream usage, and the benefit outweighs the risk IMO.

@hidmic can you take a look?

@sloretz
Copy link
Contributor Author

sloretz commented Apr 27, 2020

CI (Testing launch_testing test_launch_testing ros2topic just fastrtps_static)

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

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

The assert message still needs work, but this looks like an improvement to me too. LGTM

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Contributor Author

sloretz commented Apr 27, 2020

Second run of CI linux test to check flake8 fix Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Apr 27, 2020

CI looks ok. Linux, Linux-aarch64, OSX only had a flake-8 failure, but the re-run of linux shows that was fixed by 9ce33de. Windows showed a flake8 failure plus build warning, but that warning also exists in the nightly. Merging.

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.

3 participants