Fix failing AppVeyor Python2.7 tests #985
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
lukpueh
force-pushed
the
quickfix-win-py27-tests
branch
3 times, most recently
from
0fccc1f
to
d721cf0
Compare
Since theupdateframework#885 the tests in TestUpdater and TestKeyRevocation fail on Appveyor Python 2.7 builds. After some live debugging, it turns out that the tests fail due to the extra amount of http requests to the simple http server (see tests/simple_server.py) that were added in theupdateframework#885. The simple server runs in a subprocess and is re-used for the entire TestCase. After a certain amount of requests it becomes unresponsive. Note that neither the subprocess exits (ps -W), nor does the port get closed (netstat -a). It just doesn't serve the request, making it time out and fail the test. The following script can be used to reproduce the issue (run in tests directory): ```python import subprocess import requests import random counter = 0 port = random.randint(30000, 45000) command = ['python', 'simple_server.py', str(port)] server_process = subprocess.Popen(command, stderr=subprocess.PIPE) url = 'http://localhost:'+str(port) + '/' sess = requests.Session() try: while True: sess.get(url, timeout=3) counter +=1 finally: print(counter) server_process.kill() ``` It fails repeatedly on the 69th request, but only if `stderr=subprocess.PIPE` is passed to Popen. Given that for each request the simple server writes about ~60 characters to stderr, e.g. ... ``` 127.0.0.1 - - [24/Feb/2020 12:01:23] "GET / HTTP/1.1" 200 - ``` ... it looks a lot like a full pipe buffer of size 4096. Note that the `bufsize` argument to Popen does not change anything. As a simple work around we silence the test server on Windows/Python2 to not fill the buffer. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
force-pushed
the
quickfix-win-py27-tests
branch
from
d721cf0
to
7dbb30a
Compare
|
Great job tracking the sources of failure down @lukpueh! These changes LGTM. |
tests/simple_server.py was copied to tuf/scripts/ to "make testing easier" (cf84d3f), although with the current test setup the original (and recently patched to fix an Windows/Py2 test issue) test simple_server.py can be used just as well. This commit: - removes tuf/scripts/simple_server.py Note: that version slightly differed from the original test server, probably due to demands by the linter that is only executed on the tuf core code and not on the tests. However, for the testing purposes of simple_server.py these changes (i.e., `SystemRandom()`, `if __name__ =='__main__':`) are not necessary. - updates the tests that used tuf.scripts.simple_server to instead use tests.simple_server, - updates setup.py to not install the simple_server module as script, when installing tuf, as it is only a testing script and not meant for end-user usage. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
force-pushed
the
quickfix-win-py27-tests
branch
from
52071b1
to
842f843
Compare
|
Thanks for the feedback, @joshuagl! I say we merge this. |
3 tasks
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.
Fixes issue #:
#965
Description of the changes being introduced by the pull request:
Since #885 the tests in TestUpdater and TestKeyRevocation fail on Appveyor Python 2.7 builds. After some live debugging, it turns out that the tests fail due to the extra amount of http requests to the simple http server (see tests/simple_server.py) that were added in #885.
The simple server runs in a subprocess and is re-used for the entire TestCase. After a certain amount of requests it becomes unresponsive. Note that neither the subprocess exits (ps -W), nor does the port get closed (netstat -a). It just doesn't serve the request, making it time out and fail the test.
The following script can be used to reproduce the issue (run in tests directory):
It fails repeatedly on the 69th request, but only if
stderr=subprocess.PIPEis passed to Popen. Given that for each request the simple server writes about ~60 characters to stderr, e.g.... it looks a lot like a full pipe buffer of size 4096. Note that the
bufsizeargument to Popen does not change anything.As a simple workaround we silence the test server on Windows/Python2 to not fill the buffer.
Please verify and check that the pull request fulfills the following
requirements: