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

[ros2param] Provide tests for ros2param verbs #389

Open
wants to merge 7 commits into
base: rolling
Choose a base branch
from
Open

[ros2param] Provide tests for ros2param verbs #389

wants to merge 7 commits into from

Conversation

BMarchi
Copy link
Contributor

@BMarchi BMarchi commented Oct 30, 2019 •
edited
Loading

As the title says, this PR provides a set of tests covering the usage of all the verbs for ros2param

  • Linux Build Status
  • Arch Build Status
  • Windows Build Status
  • OSX Build Status

Connected to ros2/rclpy#455

@BMarchi BMarchi mentioned this pull request Oct 30, 2019
Merged
@BMarchi BMarchi changed the title Provide tests for ros2param verbs [ros2param] Provide tests for ros2param verbs Nov 1, 2019
hidmic
hidmic requested changes Nov 4, 2019
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.

First pass!

ros2param/test/test_cli.py Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
ros2param/test/fixtures/param_node.py Outdated Show resolved Hide resolved
ros2param/test/test_cli_delete.py Show resolved Hide resolved
@BMarchi BMarchi requested a review from hidmic November 5, 2019 12:08
hidmic
hidmic requested changes Nov 5, 2019
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.

Second pass!

ros2param/test/test_cli.py Show resolved Hide resolved
ros2param/test/param_node.py Outdated Show resolved Hide resolved
ros2param/test/test_cli.py Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
ros2param/test/test_cli_set.py Outdated Show resolved Hide resolved
ros2param/test/test_cli_set.py Outdated Show resolved Hide resolved
ros2param/test/test_cli_delete.py Outdated Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
@BMarchi
Make use of subtests and address pr comments
699aee4 
Signed-off-by: Brian Ezequiel Marchi <[email protected]>
@BMarchi
Move constants to their own files
636a6c0 
Signed-off-by: Brian Ezequiel Marchi <[email protected]>
@BMarchi BMarchi requested a review from hidmic November 6, 2019 18:11
@BMarchi BMarchi mentioned this pull request Nov 8, 2019
Open
hidmic
hidmic approved these changes Nov 11, 2019
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.

LGTM but for a few more comments and pending green CI

ros2param/test/test_cli.py Outdated Show resolved Hide resolved
ros2param/test/test_cli_delete.py Outdated Show resolved Hide resolved
ros2param/test/test_cli.py Outdated Show resolved Hide resolved
@BMarchi
Address PR comments
ded6028 
Signed-off-by: Brian Ezequiel Marchi <[email protected]>
hidmic
hidmic approved these changes Nov 11, 2019
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.

LGTM pending one last CI run

@BMarchi
Copy link
Contributor Author

BMarchi commented Nov 11, 2019

Yep, lets hope it doesn't hang

@BMarchi
Copy link
Contributor Author

BMarchi commented Nov 11, 2019

  • Linux Build Status
  • Arch Build Status
  • Windows Build Status
  • OSX Build Status

@hidmic hidmic mentioned this pull request Nov 19, 2019
Closed
@BMarchi
Copy link
Contributor Author

BMarchi commented Nov 25, 2019

Playing around with a Windows VM I was able to get all test running just fine if and only if I run them individually for each rmw_implementation, when I combine them (at least fastrtps, cyclone and connext), some tests hang and return exit code 1 after a sigterm is sent. I wasn't able yet to determine the root cause of this. Any thoughts?

@claireyywang
Copy link
Contributor

Playing around with a Windows VM I was able to get all test running just fine if and only if I run them individually for each rmw_implementation, when I combine them (at least fastrtps, cyclone and connext), some tests hang and return exit code 1 after a sigterm is sent. I wasn't able yet to determine the root cause of this. Any thoughts?

I've encountered similar situation when running CI on another ros2cli PR on windows. It seems like there's no definite fix for it yet according to @nuclearsandwich 's investigation ros2/rclpy#456 and ros2/build_farmer#248

@nuclearsandwich
Copy link
Member

I've encountered similar situation when running CI on another ros2cli PR on windows. It seems like there's no definite fix for it yet according to @nuclearsandwich 's investigation ros2/rclpy#456 and ros2/build_cop#248

The hangs in ros2/build_farmer#248 were resolved by eProsima/Fast-DDS#871 and the hung processes were deadlocked requiring a SIGKILL so it is likely something else going on here.

@ivanpauno
Copy link
Member

@hidmic I assigned you, so it doesn't appear more in our triage search, I also understand you're planning to follow-up on this.

@hidmic hidmic mentioned this pull request Apr 10, 2020
Closed
@clalancette clalancette mentioned this pull request Oct 29, 2020
Merged
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:23
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

@hidmic hidmic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BMarchi @claireyywang @nuclearsandwich @ivanpauno @hidmic