[RFR] browser windows handling #157
Conversation
digitronik
changed the title
|
aaaaaaahhh its due to |
Signed-off-by: Nikhil Dhandre <[email protected]>
digitronik
force-pushed
the
windows_handles
branch
from
1a884e8
to
8dd9420
Compare
digitronik
changed the title
There was a problem hiding this comment.
Thanks for addressing this @digitronik! Clean code, thanks for writing docblocks from the start.
Let's discuss some additional functionality for this first iteration of window handling, but I don't think any major changes from what you have here.
|
@digitronik Please write some unit tests for all of these functions. |
Signed-off-by: Nikhil Dhandre <[email protected]>
|
@mshriver I added unit tests for current implementation of windows handling. I tried other as well but I think it will effect more and may be break other dependent projects so adding issue for that idea. |
src/widgetastic/browser.py
Outdated
| def title(self): | ||
| """Returns current title""" | ||
| result = self.selenium.title | ||
| self.logger.info('current title -> %r', result) |
There was a problem hiding this comment.
Please use more consistent log formatting here, note the logging for the url properties. Should be 'Current title: %r'
digitronik
force-pushed
the
windows_handles
branch
from
2d98275
to
f4e5272
Compare
src/widgetastic/browser.py
Outdated
| @property | ||
| def title(self): | ||
| """Returns current title""" | ||
| result = self.selenium.title |
There was a problem hiding this comment.
please use self descriptive names for variables
Signed-off-by: Nikhil Dhandre <[email protected]>
digitronik
force-pushed
the
windows_handles
branch
from
f4e5272
to
30f6c14
Compare
|
|
||
|
|
||
| def test_current_window_handle(browser): | ||
| assert browser.title == "Test page" |
There was a problem hiding this comment.
Please write some docblocs for these tests, stating what component of window handling they're covering.
This case looks more like a test of the new title attribute than it is a test of current_window_handle, which is tested in test_window_handles below anyway.
There was a problem hiding this comment.
Added separate tests for each method... with small doc block. 👍
testing/test_browser.py
Outdated
|
|
||
| def test_window_handles(request, browser): | ||
| assert len(browser.window_handles) == 1 | ||
| handle = browser.new_window(url="http://example.com") |
There was a problem hiding this comment.
let's not include actual HTTP requests, and name resolution, in the unit tests.
Use the test_server fixture to just open another window to the testing page, hosted locally.
There was a problem hiding this comment.
moved to test_server.url
|
@mshriver changes added |
| def title(self): | ||
| """Returns current title""" | ||
| current_title = self.selenium.title | ||
| self.logger.info('Current title: %r', current_title) |
There was a problem hiding this comment.
I wouldn't do that. 1. f-strings will limit wt.core to py 3.6+ ; 2. logger module has its own syntax for logging statements.
Signed-off-by: Nikhil Dhandre [email protected]
selenium callso wrapping thing for browserManageIQ/integration_tests#9195