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

remove blocks and workarounds on service tests #162

Closed
wants to merge 2 commits into from

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Sep 19, 2016

This should not be merged with ros2/rmw_fastrtps#56, but can be used for testing it, if Connext is not tested at the same time.

This can be merged once Connext has been updated to support wait_for_service complete as well (in progress).

Connects to ros2/rmw_fastrtps#56

@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Sep 19, 2016
@dhood
Copy link
Member

dhood commented Sep 19, 2016

This might be ready to be removed as well, right?

std::this_thread::sleep_for(std::chrono::seconds(2));
(couldn't add a line comment)

@wjwwood
Copy link
Member Author

wjwwood commented Sep 19, 2016

Yeah, but it would need to be replaced with a wait_for_service call. I just started CI, but if that's looking ok, I'll try to make this change and rerun them.

@dhood
Copy link
Member

dhood commented Sep 19, 2016

is this wait_for_service call not sufficient?

if (!requester->wait_for_service(20_s)) {

@wjwwood
Copy link
Member Author

wjwwood commented Sep 19, 2016

Ah, yeah it is. Sorry, should have read more of the context.

@wjwwood
Copy link
Member Author

wjwwood commented Sep 19, 2016

Removed it in 2bc1097.

@wjwwood
Copy link
Member Author

wjwwood commented Sep 20, 2016

I'm going to close this pr and push the changes to a different branch name, that way I can test the other changes on this branch name without these because they break Connext.

@wjwwood wjwwood closed this Sep 20, 2016
@wjwwood wjwwood removed the in progress Actively being worked on (Kanban column) label Sep 20, 2016
@wjwwood
Copy link
Member Author

wjwwood commented Sep 20, 2016

New branch name is local_graph_changes to match the upcoming Connext changes which should make these changes work.

@wjwwood wjwwood deleted the feature/service_is_available branch September 20, 2016 16:51
wjwwood pushed a commit that referenced this pull request Sep 21, 2016
* Add regression test for different behaviour between first and second client (#156)

* Add regression test for different behaviour between first and second client

* lint

* Fix compiler warnings

* Spelling fixup

* remove blocks and workarounds on new service test
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.

None yet

2 participants