Philimon/suite refactor #445
Conversation
| yield find_toolbar | ||
|
|
||
|
|
||
| @pytest.fixture() |
There was a problem hiding this comment.
This may be for much later but...we might want to do away with BrowserActions at some point, and either way, I probably shouldn't have gotten in the habit of calling it ba in tests. So maybe we'll work on this part a bit.
There was a problem hiding this comment.
That makes a lot of sense. Most methods can be moved to the page_base or the corresponding BOM. For now, I can rename the instance fixture to be more descriptive of what it is.
| @@ -103,6 +104,28 @@ def set_chrome_context(self): | |||
| if self._xul_source_snippet not in self.driver.page_source: | |||
| self.driver.set_context(self.driver.CONTEXT_CHROME) | |||
|
|
|||
| @staticmethod | |||
There was a problem hiding this comment.
Awesome way of solving the problem.
There was a problem hiding this comment.
I wanted to go further with it to enable the BOM method to work by default in the Chrome context but faced some difficulties. I'd like to get it working for the next round of improvements.
|
|
||
|
|
||
| def test_find_toolbar_navigation(driver: Firefox): | ||
| def are_lists_different(a: int, b: int) -> bool: |
There was a problem hiding this comment.
this is a good rename
| """ | ||
| C127249: Navigate through found items | ||
|
|
||
| Arguments: | ||
| ba: instantiation of BrowserActions BOM. |
There was a problem hiding this comment.
BrowserActions isn't strictly a BOM. Let's talk about BrowserActions sometime, I do think it might be worth considering eliminating it and merging it with BasePage
There was a problem hiding this comment.
Definitely Agree
| @@ -2,10 +2,13 @@ | |||
| from time import sleep | |||
|
|
|||
| import pytest | |||
| from pynput.keyboard import Controller | |||
There was a problem hiding this comment.
So I know that it's standard to import everything at the top, but in the case of pynput, we really don't want to do that--the simple act of importing pynput on a *nix system without a DISPLAY env var set causes a crash.
There was a problem hiding this comment.
I suspected there was a reason the import wasn't at the top. my bad, should have asked before the change 😅
| ): | ||
| """ | ||
| C3932: PDF files can be successfully downloaded via pdf.js | ||
|
|
||
|
|
There was a problem hiding this comment.
Let's also talk about whether we need args in the docstring for tests. I can see it both ways, and I want to decide what are the criteria for inclusion in the docstring.
There was a problem hiding this comment.
Thats fair. Some comments on the parameters are necessary though, especially until we reach a point where we all have descriptive parameters. But we could skip the repeated parameters like driver since we all know what it is.
There was a problem hiding this comment.
Yeah, that's a good point
|
|
||
|
|
||
| def zoom_pdf_viewer( |
There was a problem hiding this comment.
Maybe this should move to the GenericPdf POM or to the pdf_viewer conftest? Seems like a generic function that may be needed in future tests...
| numeric_field.send_keys(test_value + Keys.TAB) | ||
| numeric_field = pdf.get_element("zipcode-field") | ||
| numeric_field.clear() | ||
| numeric_field.send_keys("12345" + Keys.TAB) |
There was a problem hiding this comment.
is there a reason to move the constant test value out of the variable and into two different lines? Open to it, but would love to know the rationale
Description
General Improvement of the
find_toolbarandpdf_viewertest suites.This includes improvements to the
GenericPDF,Navigation, andFindToolbarobject models.Bugzilla bug ID: 1935748
Testrail:
Link: Points of Improvment.pdf
Type of change
How does this resolve / make progress on that bug?
Clean up to improve the tests' and object models' readability and clarity.
Comments / Concerns
Test improvements include clean-up, moving context switches to decorators, moving large functionalities to object models, and more.