Repository navigation
Reusable session - #783
Reusable session#783yahman72 wants to merge 9 commits into
Conversation
aaltat
left a comment
aaltat
left a comment
There was a problem hiding this comment.
Did only has time to do partial review, but there is few things where you can proceed already.
| @@ -1,13 +1,20 @@ | |||
| import os | |||
| import os, signal | |||
There was a problem hiding this comment.
- The master have moved forward and there are quite many conflicts which you must resolve.
- Write:
import os
import signal - Fix, in the code you did write, all the pep8 related errors. Example line length must be followed.
The only default rule are the keyword examples which can exceed the line length.
| import os, signal | ||
| import robot | ||
| from robot.errors import DataError | ||
| from robot.api import logger |
There was a problem hiding this comment.
We do not use robot logger directly. There are logging methods in the library instance, like self.info
| } | ||
|
|
||
| class _ReusableDriver(webdriver.Remote): | ||
| def __init__(self, command_executor, sid, keep_alive=False, file_detector=None): |
There was a problem hiding this comment.
Empty line after class definition
| from keywordgroup import KeywordGroup | ||
| from selenium.common.exceptions import NoSuchWindowException | ||
|
|
||
| from selenium.webdriver.remote.errorhandler import ErrorHandler |
There was a problem hiding this comment.
Remove the imports that are not used.
| 'edge': "_make_edge" | ||
| } | ||
|
|
||
| class _ReusableDriver(webdriver.Remote): |
There was a problem hiding this comment.
Two empty lines before the class definition.
| file_detector=file_detector) | ||
|
|
||
| def stop_client(self): | ||
| if self.pid is not None and self.pid != '': |
There was a problem hiding this comment.
Would: if self.pid: work too? At least it would be more Pythonic way to write the code.
There was a problem hiding this comment.
Unfortunately not, because the selenium implementation is not consistent for different browsers.
Some of them use pid=None and some of them use pid=''.
| logger.info("Stopping driver for session '%s' with PID: %s" % (self.session_id, self.pid)) | ||
| try: | ||
| os.kill(int(self.pid), signal.SIGTERM) | ||
| except Exception: |
There was a problem hiding this comment.
- If the pid does not exist then the different OS will raise different types of errors. In Windows 10 it is
WindowsErrorand in Linux is itOSError. But both exceptions are based on the same error:EnvironmentError. Perhaps it would be best to do:expect EnvironmentError - If we can not kill the driver executable, is there any reason to continue? Should we just let the exception raise and just stop the test execution?
There was a problem hiding this comment.
1 done
2 I'd say no. The side-effect is that the affected browser just stays open and is not anymore "reachable" via Selenium and must be closed manually. IMHO this is okay when considering the use cases for these KW's = TC development i.e. not for production use.
| def save_webdriver(self, file=''): | ||
| """Stores the current web-driver session. | ||
|
|
||
| This KW makes the WebDriver sessions persistent. I.e. you can store the session(s) and leave the browser |
There was a problem hiding this comment.
-
Write that this is only for purposed for test development and is not for production use.
-
The documentation need some reformatting. But lets discuss that later.
|
quick one: But of course I'll update the PR accordingly as soon as I can (in 2 weeks hopefully) - now pretty busy with other stuff and easter holidays |
|
Ah ok. I will try to do functionality review and let's see after that. |
|
Sorry that I haven't found time to look on the PR. I have been busy with the new architecture and other real life stuff... I will try to take a look as soon as possible. |
|
@yahman72 are you still planning to do something with the PR or can we close it? |
|
Closing due inactivity. |
|
sorry, forgot to answer - still planning to update this, but at the moment I don't need this = no idea when I would do this. In short: better to close this, I can create a new PR/issue when I start working on this again |
|
No worries, I am just doing spring cleaning and closing old issues and PR. And if code is needed, then it can be found from the GitHub. |
PR for #690