Repository navigation
Fixes Chromedriver tests under Windows, by @yahman72 - #694
HelioGuilherme66 wants to merge 2 commits into
Conversation
|
Tested under Windows 7 Professional x64 and Python 2.7.12 32bit, selenium 2.53.6, RF 3.0 |
|
There are few minor things to fix, mostly code cleanup and following pep8. But those things are hard to comment with mobile. I will try to find time to comment the changes later. Could you raise an issue about the problem? |
|
|
||
| import socket | ||
|
|
||
| class StoppableHttpRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler): |
There was a problem hiding this comment.
Two empty lines before the class definition
| while not self.stop: | ||
| self.handle_request() | ||
|
|
||
| class HttpEchoer(socket._fileobject): |
There was a problem hiding this comment.
Two empty lines before the class definition
|
|
||
| def start_http_server(): | ||
| server_output = TemporaryFile() | ||
| #server_output = open("server.log", 'w+', buffering=False) |
There was a problem hiding this comment.
If this is commented out, where the line 51 server_output comes.
There was a problem hiding this comment.
That is just an alternative way to keep the server logs (like Jari explained in initial code/Issue).
There was a problem hiding this comment.
OK, need to read it from there.
There was a problem hiding this comment.
But should we remove the comment then
There was a problem hiding this comment.
No, we should not remove the comment. We should add a comment describing that by default the output goes to a temporary file, but toggling the commented lines allows developer to keep the output.
There was a problem hiding this comment.
I don not like commented code, but adding the doc about the that part is good to go.
There was a problem hiding this comment.
Added comment for clarification.
| # we are responding with HTTP 1.0 = we do not need content-length header | ||
| #self.send_header("Content-Length", str(fs[6])) | ||
| self.send_header("Last-Modified", self.date_time_string(fs.st_mtime)) | ||
| #self.send_header("Connection", "close") |
There was a problem hiding this comment.
If this is not needed, remove it
| self.end_headers() | ||
| return f | ||
|
|
||
|
|
There was a problem hiding this comment.
Two empty lines before the class definition
3a642b4 to
e03bdf4
Compare
|
Code is PEP8 compliant now. |
|
Commented code makes no sense. If we think server logs are needed, we should write code like that. I think that would be fine, writing them into the same directory as other results shouldn't harm. If we want to make it configurable, we should add a command line option. I doubt it's worth the effort in this case. |
|
Also, the code is not PEP-8 compliant. |
|
I changed run_tests.py to preserve server logs in PR #710. Assuming it is merged, changes to run_tests.py in this PR aren't needed anymore. |
(from his commit: webdriver process management) PEP8 Compliant. (untouch test/run_tests.py)
adcfbfa to
eff7ab7
Compare
|
Also this PR does not resolve the problem in Windows. The test execution still fails with or without this PR. But I must say that it improves the situation a lot. |
|
This PR was valid at the time it was proposed. |
|
It could be that this is some sort of windows problem. If I comment out the Having the support for running test in windows would be really nice, but it strongly looks like that we need to look some other http server. |
|
Closing in favor of #748. |
(from his commit: webdriver process management)