Fixes Chromedriver tests under Windows, by @yahman72 #694
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
test/run_tests.py Outdated
| | ||
| 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
HelioGuilherme66 force-pushed the fix_chrome_testing branch from 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)
HelioGuilherme66 force-pushed the fix_chrome_testing branch from 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. |
3 participants
(from his commit: webdriver process management)