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.

Sign up for GitHub

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

Jump to bottom

Fix expectation of "Incompatible QoS" messages in unit test #496

Merged
merged 7 commits into from
May 4, 2020
Merged

Fix expectation of "Incompatible QoS" messages in unit test #496

merged 7 commits into from
May 4, 2020

Conversation

mm318
Copy link
Member

@mm318 mm318 commented Apr 23, 2020

Closes #492

The test_echo_pub.py unit test is failing due to interpreting newly added warning messages as received messages from a publisher. This pull request filters out those warning messages.

@dirk-thomas
Copy link
Member

Should the existence of such a warning really be ignored in the test? Should the test not either ensure that no such warning is present or that an excepted warning is present?

@mm318
Copy link
Member Author

mm318 commented Apr 23, 2020

That test's purpose is to test for the presence or non-presence of topic messages received from a publisher.

There could be another test that tests for the presence or non-presence of a warning, but that would be more reasonable if the default rmw implementation FastRTPS also supports it.

@hidmic
Copy link
Contributor

hidmic commented Apr 23, 2020

That test's purpose is to test for the presence or non-presence of topic messages received from a publisher.

Regardless, why does that warning pop up in the first place?

@mm318
Copy link
Member Author

mm318 commented Apr 23, 2020 •
edited
Loading

Regardless, why does that warning pop up in the first place?

The test is purposely using a publisher with a QoS policy that's incompatible with the custom set ros2 topic echo subscription QoS policy.

@dirk-thomas
Copy link
Member

The test is purposely using a publisher with a QoS policy that's incompatible with the custom set ros2 topic echo subscription QoS policy.

Then the test should also cover that the user see the desired warning.

@mm318 mm318 force-pushed the miaofei/fix-ros2topic-unit-test branch from 149ddd8 to bc2ea93 Compare April 24, 2020 03:45
@mm318 mm318 changed the title Filter out "Incompatible QoS" messages from unit test Fix expectation of "Incompatible QoS" messages in unit test Apr 24, 2020
@mm318
Copy link
Member Author

mm318 commented Apr 24, 2020

Then the test should also cover that the user see the desired warning.

Okay @dirk-thomas, I have updated the test to do so.

hidmic
hidmic reviewed Apr 24, 2020
View reviewed changes
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.

Overall LGTM

ros2topic/test/test_echo_pub.py
assert not command.output, (
'Echo CLI should not have received anything with incompatible QoS'
)
if 'rmw_fastrtps' in get_rmw_implementation_identifier():
Copy link
Contributor

Choose a reason for hiding this comment

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

@mm318 mind to add a TODO for removal once ros2/rmw_fastrtps#356 is addressed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

hidmic
hidmic approved these changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! CI up to ros2topic:

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

@hidmic
Copy link
Contributor

hidmic commented Apr 27, 2020 •
edited
Loading

@mm318 there're some flake8 issues.

@mm318
Copy link
Member Author

mm318 commented Apr 27, 2020

Hi @hidmic, I have resolved the flake8 issues. Thanks!

@mm318
Copy link
Member Author

mm318 commented Apr 28, 2020

Hi @hidmic, I think this pull request is ready to be merged. Thanks!

@mm318
Copy link
Member Author

mm318 commented Apr 29, 2020

Let's hold off on merging this pull request until we've reached a conclusion on #500 (comment), because if we were to revert the changes in #410, then the contents of the expected warning message will be different.

@mm318
fix unit test failure in test_echo_pub.py
1a22545 
Signed-off-by: Miaofei <miaofei@amazon.com>
@mm318
address PR comments
ba80509 
Signed-off-by: Miaofei <miaofei@amazon.com>
@mm318
add TODO regarding special case for rmw_fastrtps
77113ec 
Signed-off-by: Miaofei <miaofei@amazon.com>
@mm318
fix flake8 style violation
dd5b3c6 
Signed-off-by: Miaofei <miaofei@amazon.com>
@mm318
Revert "Added incompatible event support to ros2 topic echo and ros2 …
9e8bb33 
…topic pub (ros2#410)"

This reverts commit 02ae480.

Signed-off-by: Miaofei <miaofei@amazon.com>
@mm318
update expected output in unit test
550f257 
Signed-off-by: Miaofei <miaofei@amazon.com>
@mm318 mm318 force-pushed the miaofei/fix-ros2topic-unit-test branch from f01c183 to 550f257 Compare April 30, 2020 22:08
@mm318
remove unused imports
d4162fa 
Signed-off-by: Miaofei <miaofei@amazon.com>
This was referenced Apr 30, 2020
Closed
Merged
@ivanpauno
Copy link
Member

CI for both this PR and ros2/rclpy#553:

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

@ivanpauno
Copy link
Member

@hidmic is this ready to be merged?

@hidmic
Copy link
Contributor

hidmic commented May 4, 2020

It is, assuming the discussion in #500 (comment)
has settled. Are you planning on making more changes @mm318?

@ivanpauno
Copy link
Member

ivanpauno commented May 4, 2020 •
edited
Loading

Are you planning on making more changes @mm318?

PR ros2/rclpy#553 is enough to solve that discussion.
Reverting #410 is optional, and would modify the messages.

IMO, this PR is ready to be merged. @mm318 WDYT?

@mm318
Copy link
Member Author

mm318 commented May 4, 2020

I don't plan on making anymore changes to this pull request. It should be merged to also fix #492. Thanks!

@ivanpauno ivanpauno merged commit b0e43f6 into ros2:master May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidmic hidmic hidmic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ros2topic test_echo_pub can't handle new QoS mismatch warnings
4 participants
@mm318 @dirk-thomas @hidmic @ivanpauno