Win32 tests and timeout fix #776
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit gets our unit tests compiling and running on Windows. There
are two things of particular interest here:
1. In Windows, opening a non-existent hangs forever in WSAPoll whereas
it correctly returns with a "Connection refused" error on Linux.
For that reason, I simply skip this test in Windows.
It may be related to this known issue:
https://daniel.haxx.se/blog/2012/10/10/wsapoll-is-broken/
2. Hiredis handles timeouts slightly differently in Windows than on
Linux. In Linux we intentionally set REDIS_ERR_IO for connection
timeouts whereas in Windows we set REDIS_ERR_TIMEOUT. It may be
prudent to fix this discrepancy although there are almost certainly
users relying on the current behavior.
As of CMake 3.4.3 there is an option to have it generate the .def file on Windows dynamically. This seems better than a .def file which can get out of date but also that needs to be slightly different when building in Cygwin vs. MinGW.
|
@mnunberg There aren't any changes to the core library here, but let me know if you'd like some modifications w/r/t the Windows changes. I feel like the correct solution would be to provide the same contract on Windows and Linux in terms of the timeout errors, but I'm sure it will break people's code. |
* Move our unix socket detection to after argument parsing such that we don't only look for the default value of '/tmp/redis.sock'. * Add an argument to treat any skipped tests as a failure. This way we won't accidentally think tests are passing in Travis when they were actually skipped due to test detection. * Minor Travis tweaks (remove deprecated `root` and switch `matrix` -> `jobs`)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds three things:
When running unit tests in Windows I found two places where functionality differed.
In Linux attempting to connect to a non-existent port returns an error, but in Windows it just hangs forever. I believe this is related to a bug in WSAPoll but I really don't know enough about the Windows API to say for sure. It sure seems related though.
Timeouts have slightly different behavior on Windows (example 1, example 2). It seems as if we should honor the same contract in either OS but OTOH people are almost certainly relying on the current behavior. Interested in opinions.
The test detection logic helps with running the unit tests in Windows (e.g. it skips AF_UNIX tests) but also makes it slightly nicer to run tests if your server isn't local (see #762 as mentioned above).