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

Unit tests and bug fixes for XmlRpcSocket #1218

Merged
merged 4 commits into from Nov 7, 2017
Merged

Unit tests and bug fixes for XmlRpcSocket #1218

merged 4 commits into from Nov 7, 2017

Conversation

trainman419
Copy link
Contributor

@trainman419 trainman419 commented Nov 3, 2017 •
edited

  • Add system call mocks for socket functions, and use these to test the XmlRpcSocket class.
  • Improve the address resolution error messages to include a description the error generated by getaddrinfo.
  • Fix an existing TODO where EWOULDBLOCK was not handled as a fatal error on Linux.
  • Fix get_port so that it checks the return code from getsockname; this prevents it from using the socket data uninitialized if getsockname fails.
  • Make a few of the string arguments to XmlRpcSocket const so that they're compatible with string literals.

@trainman419
Unit tests and bug fixes for XmlRpcSocket
b2e3ab2 
Add system call mocks for socket functions, and use these to test the XmlRpcSocket class.
Improve the address resolution error messages to include a description the error generated by getaddrinfo.
Fix an existing TODO where EWOULDBLOCK was not handled as a fatal error on Linux.
Fix get_port so that it checks the return code from getsockname; this prevents it from using the socket data uninitialized if getsockname fails.
Make a few of the string arguments to XmlRpcSocket const so that they're compatible with string literals.
@trainman419 trainman419 mentioned this pull request Nov 3, 2017
Closed
mikepurvis
mikepurvis reviewed Nov 3, 2017
View reviewed changes
utilities/xmlrpcpp/test/CMakeLists.txt Outdated
)
set_target_properties(test_socket PROPERTIES
LINK_FLAGS
"-Wl,--wrap=accept -Wl,--wrap=bind -Wl,--wrap=close -Wl,--wrap=connect -Wl,--wrap=getaddrinfo -Wl,--wrap=getsockname -Wl,--wrap=listen -Wl,--wrap=read -Wl,--wrap=setsockopt -Wl,--wrap=select -Wl,--wrap=socket -Wl,--wrap=write -Wl,--wrap=fcntl -Wl,--wrap=freeaddrinfo"
Copy link
Member

Choose a reason for hiding this comment

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

Fancy. TIL.

Copy link
Member

Choose a reason for hiding this comment

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

I would expect these arguments to fail on Windows?

mikepurvis
mikepurvis reviewed Nov 3, 2017
View reviewed changes
utilities/xmlrpcpp/test/test_socket.cpp
@@ -0,0 +1,1368 @@

Copy link
Member

Choose a reason for hiding this comment

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

Copyright block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which license should I use here? I tend towards BSD but the rest of the library is licensed LGPL.

Copy link
Member

Choose a reason for hiding this comment

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

Either one would be fine. Embedding BSD inside an LGPL project is not a problem, though you should update the package.xml to declare both. If everything else is LGPL it's simpler/"cleaner" to follow the exisiting projects license. So I guess I'd suggest defaulting to BSD if you're ok with it and it's more liberal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok; added an LGPL copyright block to all of the files that I've added in this and the previous PR.

dirk-thomas
dirk-thomas reviewed Nov 6, 2017
View reviewed changes
utilities/xmlrpcpp/test/test_system_mocks.c Outdated

// The auto-generated mocks here don't use their parameters, so we disable
// that warning.
#pragma GCC diagnostic ignored "-Wunused-parameter"
Copy link
Member

Choose a reason for hiding this comment

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

Does this line need to be wrapped in a conditional in order to not fail on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. If this is built using gcc on windows it's probably fine, but the MSVC compiler may reject it.
I don't have a windows development environment so I can't test this, and REP3 does not specify Windows support or a target compiler or toolchain for Windows either, so I don't think this support should be mandatory.

Copy link
Member

Choose a reason for hiding this comment

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

Since some (brave) users do build ROS from source on Windows I would rather not introduce changes which are known ahead of time to break that. (Their might be regressions on Windows due to unforeseen side effects which we can't check without build infrastructure - we can't do anything about that atm.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok; I've wrapped both of these in feature guards so that we won't build the mocks on Windows and won't use the GCC diagnostic pragma unless __GNUC__ is defined.

I don't have a Windows machine and don't know how to set up the development environment on it, so I don't have any way to test this.

Copy link
Member

Choose a reason for hiding this comment

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

Since e.g. clang also supports this I would suggest using #ifndef _WIN32 instead.

utilities/xmlrpcpp/test/CMakeLists.txt Outdated
)
set_target_properties(test_socket PROPERTIES
LINK_FLAGS
"-Wl,--wrap=accept -Wl,--wrap=bind -Wl,--wrap=close -Wl,--wrap=connect -Wl,--wrap=getaddrinfo -Wl,--wrap=getsockname -Wl,--wrap=listen -Wl,--wrap=read -Wl,--wrap=setsockopt -Wl,--wrap=select -Wl,--wrap=socket -Wl,--wrap=write -Wl,--wrap=fcntl -Wl,--wrap=freeaddrinfo"
Copy link
Member

Choose a reason for hiding this comment

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

I would expect these arguments to fail on Windows?

dirk-thomas
dirk-thomas reviewed Nov 7, 2017
View reviewed changes
utilities/xmlrpcpp/test/CMakeLists.txt Outdated
set_target_properties(test_socket PROPERTIES
LINK_FLAGS
"-Wl,--wrap=accept -Wl,--wrap=bind -Wl,--wrap=close -Wl,--wrap=connect -Wl,--wrap=getaddrinfo -Wl,--wrap=getsockname -Wl,--wrap=listen -Wl,--wrap=read -Wl,--wrap=setsockopt -Wl,--wrap=select -Wl,--wrap=socket -Wl,--wrap=write -Wl,--wrap=fcntl -Wl,--wrap=freeaddrinfo"
)
Copy link
Member

Choose a reason for hiding this comment

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

The block within the if and endif should be indented one level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dirk-thomas
Copy link
Member

@ros-pull-request-builder retest this please

@dirk-thomas
Copy link
Member

Thank you for the patch and for iterating on it.

@dirk-thomas dirk-thomas merged commit 8b741ac into ros:lunar-devel Nov 7, 2017
@dirk-thomas dirk-thomas mentioned this pull request Feb 9, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tfoote tfoote tfoote left review comments

@mikepurvis mikepurvis mikepurvis left review comments

@dirk-thomas dirk-thomas dirk-thomas left review comments

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
4 participants
@trainman419 @dirk-thomas @tfoote @mikepurvis