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

Modifying test for failing on getting an empty node name #374

Merged
merged 2 commits into from
Jan 10, 2020
Merged

Modifying test for failing on getting an empty node name #374

merged 2 commits into from
Jan 10, 2020

Conversation

CaptainTrunky
Copy link
Contributor

@CaptainTrunky CaptainTrunky commented Jun 29, 2019

Related to ros2/rmw_connext#362 and ros2/ros2#489.

PR modifies existing node_name_list test that it would fail on getting an empty node name.

Connects to ros2/ros2#489

@CaptainTrunky CaptainTrunky changed the title 489 Modifing test for failing on getting an empty node name Modifying test for failing on getting an empty node name Jun 29, 2019
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.

Running the test without ros2/rmw_connext#362 passes normally. Unless there's a way to produce an empty node name in the test, I wonder if this change is necessary.

We can't mock an empty node name with rclcpp, since it's not allowed.

On the other hand, perhaps it's not hurting to add this additional check.

@nuclearsandwich nuclearsandwich added the in review Waiting for review (Kanban column) label Aug 15, 2019
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Oct 24, 2019
@jacobperron
Copy link
Member

Taking another look at this, I think I misinterpreted the test. If we want to add a test checking for no empty names it seems more semantically meaningful to add it to node_check_names.cpp or put it in it's own test. The test currently being modified is meant to check that an expected node name appears in the ROS graph.

@CaptainTrunky Let me know if you're still able to iterate on this.

@jacobperron jacobperron self-assigned this Dec 11, 2019
@jacobperron
Copy link
Member

jacobperron commented Jan 10, 2020

I've rebased on master to bring the branch up-to-date.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
    • test_parameter_server_cpp failure also occurs on nightly builds.
    • test_local_parameters (OpenSplice) failure not occurring on nightly, but might not occur with release builds. In any case, the test is unrelated to the test modified in this PR.
  • Windows Build Status

@jacobperron jacobperron merged commit 7f018b2 into ros2:master Jan 10, 2020
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.

4 participants