Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Fix remaining rosmsg tests#734

Merged
gerkey merged 4 commits intoros:enable_testsfrom
dhood:enable_tests
Feb 23, 2016
Merged

Fix remaining rosmsg tests#734
gerkey merged 4 commits intoros:enable_testsfrom
dhood:enable_tests

Conversation

@dhood
Copy link
Copy Markdown
Contributor

@dhood dhood commented Jan 29, 2016

Building on work related to issue #709

======================================================================
FAIL: test_get_msg_text (test_rosmsg.TestRosmsg)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/gerkey/code/rosmsg-tests/src/ros_comm/tools/rosmsg/test/test_rosmsg.py", line 74, in test_get_msg_text
    self.assertEquals(text, rosmsg.get_msg_text(type_, raw=True))
AssertionError: 'test_rosmaster/Empty empty\n' != 'Empty empty\n'

From what I can see, rosmsg.get_msg_text() used to have a 'fulltext' option which would refer to the msg types with their package name included ('test_rosmaster/Empty empty\n' vs 'Empty empty\n', as in the error output). This was removed in ef2beb7, but now having the parameter raw=False returns msgs in fulltext, while raw=True doesn't.

The tests are failing because they don't expect this discrepancy.

The current behaviour is:
With raw=False, messages are: recursive, whitespace removed, fulltext.
with raw=True, messages are not recursive, with whitespace not removed, and not fulltext.

I updated the tests to match the current behaviour of get_msg_text/get_srv_text, but it's possible that the appropriate fix is actually the other way around, as, judging from this comment in the previously failing test, the behaviour when raw=False vs True used to be different (with respect to fulltext).

@dhood dhood mentioned this pull request Jan 29, 2016
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider using the test_message_package variable to format this string instead of the raw string here and elsewhere.

@jacquelinekay
Copy link
Copy Markdown

Ran tests manually and the rosmsg tests pass.

This was the output of catkin_test_results:

roswtf/nosetests-test.xml: 3 tests, 0 errors, 1 failures
roswtf/rosunit-roswtf_command_line_online.xml: 2 tests, 0 errors, 1 failures
test_rosbag/rosunit-r02random_play.xml: 1 tests, 0 errors, 1 failures
test_rosbag/rosunit-random_play05.xml: 1 tests, 0 errors, 1 failures
test_rospy/rosunit-sub_to_multiple_pubs.xml: 1 tests, 0 errors, 1 failures

On origin/enable_tests, rosmsg has 3 failures.

Changing from std_msgs/String to test_rosmaster/String also looks good to me--why doesn't RosmsgC.msg use a raw string type? Is it important that we test a nested message here for some reason?

I'm not a maintainer on this package, will defer to @gerkey and @dirk-thomas.

@dhood
Copy link
Copy Markdown
Contributor Author

dhood commented Jan 30, 2016

Thanks for running the tests @jacquelinekay
Yeah, RosmsgC.msg is being used to test the recursive printing of message types.

@gerkey
Copy link
Copy Markdown
Contributor

gerkey commented Feb 23, 2016

Thanks for the fixes!

Given how long ago the behavior change was made, I'd prefer to stick with the current behavior, however imperfect it might be. It's more dangerous to "fix" the behavior to match the old tests, which could break somebody's system.

gerkey added a commit that referenced this pull request Feb 23, 2016
@gerkey gerkey merged commit abc0791 into ros:enable_tests Feb 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants