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

Update test_security and test_rclcpp after one Participant per Context change #406

Merged
merged 2 commits into from
Apr 3, 2020
Merged

Update test_security and test_rclcpp after one Participant per Context change #406

merged 2 commits into from
Apr 3, 2020

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Mar 13, 2020 •
edited
Loading

Updates:

@ivanpauno ivanpauno self-assigned this Mar 13, 2020
This was referenced Mar 13, 2020
Merged
Merged
@ivanpauno ivanpauno force-pushed the ivanpauno/one-participant-per-context branch from f052189 to 983e863 Compare March 19, 2020 21:38
hidmic
hidmic reviewed Mar 27, 2020
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.

Overall LGTM, pending green CI and a few questions.

test_security/test/test_invalid_secure_node_creation_c.cpp Outdated Show resolved Hide resolved
test_security/test/test_invalid_secure_node_creation_c.cpp Outdated Show resolved Hide resolved
test_security/test/test_invalid_secure_node_creation_c.cpp Outdated Show resolved Hide resolved
test_security/test/test_secure_publisher.cpp Outdated Show resolved Hide resolved
test_security/test/test_secure_subscriber.cpp Outdated Show resolved Hide resolved
@ivanpauno ivanpauno changed the title Update test_security tests Update test_security and test_rclcpp after one Participant per Context change Mar 27, 2020
@ivanpauno ivanpauno requested a review from hidmic March 27, 2020 16:45
@ivanpauno
Copy link
Member Author

@hidmic I will consolidate this in two commits before merging:

  • 76b4f49
  • The ones modifying test_security.

mikaelarguedas
mikaelarguedas approved these changes Mar 27, 2020
View reviewed changes
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

looks great! (with green CI)

test_security/test/test_invalid_secure_node_creation_c.cpp Outdated Show resolved Hide resolved
test_security/test/test_secure_publisher_subscriber.py.in Outdated Show resolved Hide resolved
test_rclcpp/CMakeLists.txt Outdated Show resolved Hide resolved
test_rclcpp/CMakeLists.txt Show resolved Hide resolved
hidmic
hidmic approved these changes Mar 27, 2020
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 too!

test_security/test/test_invalid_secure_node_creation_c.cpp Show resolved Hide resolved
test_security/test/test_invalid_secure_node_creation_c.cpp Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Member Author

@mikaelarguedas @hidmic If everything looks good, I will consolidate commits.

hidmic
hidmic approved these changes Mar 27, 2020
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

mikaelarguedas
mikaelarguedas approved these changes Mar 28, 2020
View reviewed changes
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm

@ivanpauno ivanpauno force-pushed the ivanpauno/one-participant-per-context branch from 5e707e3 to 1eae267 Compare March 31, 2020 14:09
@ivanpauno
Copy link
Member Author

I've consolidated the PR in two commits:

  • Changes in test_security.
  • Disabling cross-vendor tests that are expected to fail now.

I've double checked that I didn't change nothing in the rebasing process, by doing a git diff against the non-squashed version.

@ivanpauno ivanpauno requested a review from hidmic March 31, 2020 14:12
@ivanpauno ivanpauno mentioned this pull request Apr 2, 2020
Merged
ivanpauno added 2 commits April 3, 2020 10:18
@ivanpauno
Update test_security tests to use security contexts
988ad01 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Skip failing cross vendor tests after Fast-RTPS based rmw implementat…
1623fbb 
…ions are using one Participant per Context

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno force-pushed the ivanpauno/one-participant-per-context branch from 1d93d97 to 1623fbb Compare April 3, 2020 13:18
@ivanpauno ivanpauno merged commit d489da7 into master Apr 3, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/one-participant-per-context branch April 3, 2020 13:19
@ivanpauno
Copy link
Member Author

For the record, the last force push just squashed commits, without introducing changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikaelarguedas mikaelarguedas mikaelarguedas approved these changes

@hidmic hidmic hidmic approved these changes

Assignees

@ivanpauno ivanpauno

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ivanpauno @mikaelarguedas @hidmic