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

Add tests take sequence + take sequence with bad arguments #128

Merged
merged 1 commit into from
Sep 23, 2020
Merged

Add tests take sequence + take sequence with bad arguments #128

merged 1 commit into from
Sep 23, 2020

Conversation

Lobotuerk
Copy link
Contributor

@Lobotuerk Lobotuerk commented Sep 16, 2020 •
edited
Loading

Adds tests for checking returns on bad arguments for rmw_take_sequence
Signed-off-by: lobotuerk jtlorente@ekumenlabs.com

@Lobotuerk Lobotuerk changed the title Add tests take sequence bad arguments Add tests take sequence Sep 16, 2020
@Lobotuerk Lobotuerk marked this pull request as ready for review September 17, 2020 15:10
Blast545
Blast545 reviewed Sep 17, 2020
View reviewed changes
Copy link
Contributor

@Blast545 Blast545 left a comment

Choose a reason for hiding this comment

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

Minor suggestions/questions

test_rmw_implementation/test/test_subscription.cpp Outdated Show resolved Hide resolved
test_rmw_implementation/test/test_subscription.cpp Outdated Show resolved Hide resolved
test_rmw_implementation/test/test_subscription.cpp Outdated Show resolved Hide resolved
test_rmw_implementation/test/test_subscription.cpp Show resolved Hide resolved
test_rmw_implementation/test/test_subscription.cpp Outdated Show resolved Hide resolved
test_rmw_implementation/test/test_subscription.cpp Outdated Show resolved Hide resolved
hidmic
hidmic reviewed Sep 18, 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 with @Blast545 comments addressed.

test_rmw_implementation/test/test_subscription.cpp Show resolved Hide resolved
@hidmic
Copy link
Contributor

hidmic commented Sep 18, 2020

Also, please consider rewriting the PR title to read better.

@Lobotuerk Lobotuerk changed the title Add tests take sequence Add tests take sequence with bad arguments Sep 22, 2020
@Lobotuerk
Copy link
Contributor Author

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

@Lobotuerk Lobotuerk changed the title Add tests take sequence with bad arguments Add tests take sequence + take sequence with bad arguments Sep 23, 2020
@Lobotuerk
fixed merge
bfaea8d 
Signed-off-by: lobotuerk <jtlorente@ekumenlabs.com>
@Lobotuerk Lobotuerk force-pushed the lobotuerk/take_sequence_bad_arguments branch from 4a8441f to bfaea8d Compare September 23, 2020 19:57
@Lobotuerk
Copy link
Contributor Author

CI up to test_rmw_implementation after rebase:

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

@Lobotuerk
Copy link
Contributor Author

All CI green, merging

@Lobotuerk Lobotuerk merged commit 07bdde0 into master Sep 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the lobotuerk/take_sequence_bad_arguments branch September 23, 2020 20:29
ahcorde pushed a commit that referenced this pull request Oct 9, 2020
@Lobotuerk @ahcorde
Add tests take sequence + take sequence with bad arguments (#128)
c12f0ec 
Signed-off-by: lobotuerk <jtlorente@ekumenlabs.com>
ahcorde pushed a commit that referenced this pull request Oct 21, 2020
@Lobotuerk @ahcorde
Add tests take sequence + take sequence with bad arguments (#128)
32f83a1 
Signed-off-by: lobotuerk <jtlorente@ekumenlabs.com>
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

@Blast545 Blast545 Blast545 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Lobotuerk @hidmic @Blast545