Skip to content

skip test relying on source timestamps with Connext#615

Merged
dirk-thomas merged 1 commit intomasterfrom
dirk-thomas/skip-test-with-connext
Aug 11, 2020
Merged

skip test relying on source timestamps with Connext#615
dirk-thomas merged 1 commit intomasterfrom
dirk-thomas/skip-test-with-connext

Conversation

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas dirk-thomas added the bug Something isn't working label Aug 10, 2020
@dirk-thomas dirk-thomas self-assigned this Aug 10, 2020
@gbiggs
Copy link
Copy Markdown
Member

gbiggs commented Aug 10, 2020

The changes themselves LGTM.

Is there an alternative test for _rclpy.rclpy_take() available?

@dirk-thomas
Copy link
Copy Markdown
Member Author

Is there an alternative test for _rclpy.rclpy_take() available?

I don't understand the question - what for?

@gbiggs
Copy link
Copy Markdown
Member

gbiggs commented Aug 11, 2020

I'm probably misunderstanding something, but it looks like you're skipping the only tests here that call rclpy_take() for Connext. I was asking if that function is tested with Connext somewhere else or if it's just left untested with Connext. If the function relies on source timestamps and is completely unusable with Connext then that's fine, but it's probably worth being more specific about that in the skip comment.

@iluetkeb
Copy link
Copy Markdown
Contributor

One test is timestamp-specific (get_service_timestamps), but the other (test_take) isn't. In the C++ tests I've #ifdef'd out the check blocks only. Not sure if sth like that is possible for pyunit?

@dirk-thomas
Copy link
Copy Markdown
Member Author

I'm probably misunderstanding something, but it looks like you're skipping the only tests here that call rclpy_take() for Connext. I was asking if that function is tested with Connext somewhere else or if it's just left untested with Connext.

The specific test was added in the above referenced PR and has failed for Connext since then. This patch only skips the always failing test.

Adding a separate test to check rclpy_take() without considering the source timestamp would be an option as well as modifying the test to skip part of the logic for Connext.

Since this change doesn't reduce the previously covered (and passed) code I don't think either should happen in this PR.

@dirk-thomas dirk-thomas merged commit 2729bb8 into master Aug 11, 2020
@dirk-thomas dirk-thomas deleted the dirk-thomas/skip-test-with-connext branch August 11, 2020 05:13
crystaldust pushed a commit to crystaldust/rclpy that referenced this pull request Sep 10, 2020
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants