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

Ensure ros2doctor ROS nodes have valid names. #513

Merged
merged 2 commits into from
May 21, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented May 15, 2020

Precisely what the title says. Came across this while testing ros2 doctor hello on a VM that had dashes in its hostname. It's tricky one to test for regression, so I'm open to ideas.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic added the bug Something isn't working label May 15, 2020
Copy link
Contributor

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

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

LGTM! I think as long as we can enforce node name uniqueness this should be fine

@jacobperron
Copy link
Member

For a test, we could try using mock as described in this SO answer.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented May 18, 2020

@claireyywang @jacobperron PTAL. I changed tests' layout a bit as the existing ones where not testing anything. I'd think there was some confusion w.r.t pytest vs. unittest and their relationship w/ launch_testing.

On a side note, I find the hello verb implementation rather unusual. Particularly when it starts spawning short-lived daemon threads in a loop. IMHO a pool of threads would be cheaper and provide for tighter thread lifetime control.

@jacobperron
Copy link
Member

I'd think there was some confusion w.r.t pytest vs. unittest and their relationship w/ launch_testing.

Oof, I didn't realize that. I wonder if there are other places in our code base that make a similar mistake.

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.

LGTM with green CI

@hidmic
Copy link
Contributor Author

hidmic commented May 18, 2020

CI up to ros2doctor:

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

@hidmic
Copy link
Contributor Author

hidmic commented May 21, 2020

Focal PR job failure is unrelated. Have two approvals. Merging.

@hidmic hidmic merged commit e177e05 into master May 21, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/fix-ros2doctor-node-names branch May 21, 2020 16:21
craigh92 pushed a commit to craigh92/ros2cli that referenced this pull request Jun 11, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Craig <CraigUkaea@gmail.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.

None yet

3 participants