Get unit tests working on all platforms. about sockpp HOT 10 CLOSED

#25 (comment)

from sockpp.

@borrrden If just setting the path for CMake works, that would be sufficient. I think I spent a total of 15min trying to get it working last month, and then never got back to it. I will try again.

from sockpp.

CMake doesn't really even need to be involved at all in such a trivial part of the build process. As long as the catch header is somewhere that is in your include paths then you will be able to build it. The complication, I guess, comes from the fact that you expect the user to supply it. Is this something you did on purpose or just something that you picked up somewhere and have no opinion about? The answer to this determines how to move forward. For example you could, as we do, include the catch header right in your repo so that the only setup needed is to make sure that folder is in your include paths in the CMake project.

from sockpp.

Oh, I’m a total CMake novice. I’m sure that came out of cookbook somewhere.

from sockpp.

I don't think I'd want to grab the Catch2 headers and throw them into this repository. That doesn't seem right.

I have a couple of Rust libraries that wrap C libraries. With those, I've taken to using a Git submodule to pull in the dependent project. That works really well with the Rust build system. I was thinking of trying that with a C++ project.

from sockpp.

OK. I got it. I saw a recommendation somewhere to just do a CMake build/install with Catch2, and that would work in a similar manner on Windows and Linux. When I tried it on Windows it failed, and I instantly gave up. This time I looked at the error and it was just an issue with file system privileges. So I tried the same thing in a console run as Administrator, and it worked, It installed into "C:\Program Files (x86)\Catch2" on my Win7 machine.

It was just this in a Git Bash window run as Administrator on Win7:

$ cd catch2
$ cmake -Bbuild -H. -DBUILD_TESTING=OFF
$ cmake --build build/ --target install

After that, the sockkpp build worked with -DSOCKPP_BUILD_TESTS=ON.
No other directory or path hint was required.

Now I'm getting those build errors with the tests. I'll start fixing them.

from sockpp.

Incorrect assumption: Address information is available before socket connection (family type will remain unspecified, as opposed to the various checks for AF_INET)

Interesting. That was my initial assumption. Surprised that Linux did, but it seemed to make sense, since it requires the family when creating the socket.

Incorrect assumption: Windows uses POSIX error codes (e.g. sock.last_error() == EAFNOSUPPORT needs to be sock.last_error() == WSAEAFNOSUPPORT). Also, often these codes are WSAEINVAL or WSAENOTSOCK instead.

This really stinks for portability.
For the unit tests, I wasn't sure if I should check for the specific failure, but now that it's there, I put conditional compilation for the WIN32 checks. I may reconsider how to do this as the number of tests grow.

socket::pair not implemented for Windows, so its corresponding test fails

Ah, yes. The call was properly failing on Windows, but it wasn't setting the last error. Now it sets it to ENOTSUP.

to_timeval is not implemented on Windows, causing compilation failures

Interesting... Winsock implements the timeval struct. I didn't know that. But, it doesn't use it in some of the typical places (like setting timeouts on the socket)!

So now to_timeval() is implemented on Windows and passing its unit tests, even though it's not currently used anywhere in the code.

from sockpp.

Thanks, @borrrden !

from sockpp.

Installing the catch headers to the program files directory is quite awkward, and it makes it so that you cannot test this library on a machine that has no administrator access by default. But since Windows has no concept of a standard header directory (Leaving aside the vortex that is C:\Windows and the IDE specific Visual Studio directories) I guess it will be acceptable.

The remedy for someone with no admin access would be that they needed to patch the CMake file to take a hint directory for the find_package() call for Catch, and set their CMake install directory to be that same directory. We use submodules quite heavily, and I can't say it has been all roses but it does get the job done of making sure everything is in a location that is reliable on all platforms.

Regarding portability......you'll get a resounding agreement from me. I don't know why they felt the need to create separate error codes when they could have easily reused the existing ones. Regardless, the one saving grace is that the errors are all the same as the POSIX errors, just prefixed with WSA so it's easy enough to create a map of WSA to posix and back if you wish (we do for the branch of our library that uses sockpp). In fact I came up with a dirty little #define hack to auto switch them:

#if defined(_WIN32)
#define SOCKET_ERR_CODE(x) WSA##x
#else
#define SOCKET_ERR_CODE(x) x
#endif

I was so proud of myself too :p until I realized that the error codes are different most of the time for the cases in your tests....but maybe the above can be useful elsewhere in case they are the "same".

from sockpp.

Yeah, I was on the fence as whether to add specific error code checks to the unit tests or not. Many of them are weird corner cases that probably wouldn't be checked for specific errors in an actual application. As I add new tests, I will likely just check that the last error is something (non-zero), except for the places where it should check for specific errors like timeouts and non-blocking returns.

On the other hand, it's cool to use the unit tests to show specific and non-portable behavior.

So it's a toss-up.

from sockpp.