type(feat): Implement Bookmark Classes and Tests #327
Conversation
Temidayo32
changed the title
|
@b4handjr Current tests didn't meet 95% requirement hence the failure. I will add missing tests to meet the requirements. |
This PR add functionalities to set, unset, and retrieve bookmarks
| class AdvancedBookmark(BasicBookmark): | ||
| """Handles advanced bookmark operations.""" | ||
|
|
There was a problem hiding this comment.
What's an "Advanced Bookmark" exactly?
There was a problem hiding this comment.
Okay, so in Desktop Smoke test, a similar term is used so I decided to adopt it in FoxPuppet. Basically, an advanced bookmark is a more detailed way of adding a bookmark, which involves name, url, tags, and keywords, as against just simply using the url alone when creating a Basic Bookmark with the star button.
There was a problem hiding this comment.
Okay let me ask Ben C for clarifications
There was a problem hiding this comment.
There was a problem hiding this comment.
@ben-c-at-moz Can you explain what your "Advanced Bookmark" action does exactly?
There was a problem hiding this comment.
This is maybe not the best name--the function here is "advanced" only in that it adds a bookmark through the "Other bookmarks" option in the Library window. The bookmark itself should be normal.
There was a problem hiding this comment.
Yeah, I agree, the name is a little confusing.
There was a problem hiding this comment.
But I'm fine if we just want to keep using it for more detailed bookmark operations, as long as we explain it.
There was a problem hiding this comment.
Could we just call it Bookmarks and put all the functionality into one class? If a user wants to do it through the "Other Bookmarks" option, we can just have a flag or something when they create the bookmark.
Temidayo32
force-pushed
the
bookmark
branch
2 times, most recently
from
93e63d3 to
6f591c7
Compare
| class AdvancedBookmark(BasicBookmark): | ||
| """Handles advanced bookmark operations.""" | ||
|
|
There was a problem hiding this comment.
But I'm fine if we just want to keep using it for more detailed bookmark operations, as long as we explain it.
@ben-c-at-moz Actually, it's failing in Nightly build due to this: browser-actions/setup-firefox#621 |
foxpuppet/windows/browser/navbar.py
Outdated
|
|
||
| def click_element(self, locator: Tuple[str, str]) -> None: | ||
| """Click on an element by its locator.""" | ||
| with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
| self.find_element(locator).click() | ||
|
|
||
| def find_element(self, locator: Tuple[str, str]) -> WebElement: | ||
| """Find and return a web element by its locator.""" | ||
| with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
| try: | ||
| element = self.root.find_element(*locator) | ||
| return element | ||
| except Exception as e: | ||
| raise NoSuchElementException( | ||
| f"Error locating element with locator {locator}: {e}" | ||
| ) | ||
|
|
||
| def context_click(self, locator: Tuple[str, str]) -> None: | ||
| """ | ||
| Perform a right-click (context-click) on an element. | ||
|
|
||
| Args: | ||
| locator (tuple): Locator for the element to right-click. | ||
| """ | ||
| with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
| element = self.find_element(locator) | ||
| self.actions.context_click(element).perform() | ||
|
|
||
| def switch_to_frame(self, locator: Tuple[str, str]) -> None: | ||
| """ | ||
| Switch to an iFrame using its locator | ||
|
|
||
| Args: | ||
| locator (tuple): Locator for the iFrame elemeent. | ||
| """ | ||
| with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
| frame_element = self.find_element(locator) | ||
| if frame_element is not None: | ||
| self.selenium.switch_to.frame(frame_element) | ||
| else: | ||
| raise NoSuchElementException(f"iFrame with locator {locator} not found.") | ||
|
|
||
| def switch_to_default_context(self) -> None: | ||
| """Switch back to default context.""" | ||
| with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
| self.selenium.switch_to.default_content() | ||
|
|
||
| def open_main_menu( | ||
| self, locator_toolbar: Tuple[str, str], locator_menu_bar: Tuple[str, str] | ||
| ) -> None: | ||
| """Activate main menu bar""" | ||
| with self.selenium.context(self.selenium.CONTEXT_CHROME): | ||
| self.context_click(locator_toolbar) | ||
| self.click_element(locator_menu_bar) | ||
|
|
There was a problem hiding this comment.
This seems out of scope for this PR no? Maybe I am missing something
There was a problem hiding this comment.
These methods are used in Bookmarks. I added them here because the issue stated to extend the NavBar with Bookmark functionalities. But then, along the way, I realized Bookmarks doesn't exactly fit with the nav-bar element. While some bookmark web elements exist in the nav bar, others are scattered across different parts of the dom. I could move them all to Bookmark itself if that makes things clearer.
Add functionality to NavBar (and elsewhere) to set, unset, and retrieve bookmarks.
There was a problem hiding this comment.
Well some of these just seem like generic actions, like switch_to_default_context() and find_element.
There was a problem hiding this comment.
Will you suggest I move them to a class higher up the hierarchy?
There was a problem hiding this comment.
So I moved all the generic actions into region.py
There was a problem hiding this comment.
I think these should be removed if they aren't specifically related to bookmark functionality. We can add these later if need be.
There was a problem hiding this comment.
Actually, these methods are functionalities I reused a lot in the Bookmark functionality. So instead of repeating them, I just factored them out into those methods.
There was a problem hiding this comment.
I understand but a lot of them are doing what selenium has built in. Selenium has a find_element method already, as an example.
There was a problem hiding this comment.
@b4handjr I have refactored and removed all of them. It is now ready for another review.
foxpuppet/windows/browser/window.py
Outdated
| else: | ||
| raise ValueError(f"Unsupported bookmark class: {bookmark_class}") |
There was a problem hiding this comment.
Won't we get this for free with the static typing we're adding to the definition?
tests/test_bookmarks.py
Outdated
| if bookmark_instance is None: | ||
| raise ValueError("Failed to get AdvancedBookmark instance.") | ||
| if not isinstance(bookmark_instance, AdvancedBookmark): | ||
| raise ValueError("Expected AdvancedBookmark instance but got different type") |
There was a problem hiding this comment.
I don't quite like this logic in a test. Why would bookmark_instance ever be none after wait_for_bookmark?
The raise ValueError seems redundant as you currently have the wait_for_bookmark throwing a ValueError too.
I think we don't want to do instance checking here if we don't need to. We should assume that we get an AdvancedBookmark until proven that we don't.
There was a problem hiding this comment.
I didn't have those lines in initially. So if I remove those lines, mypy will raise type errors like this. This is coming from the create method returning None instead of throwing the exception as you pointed out below.
tests/test_bookmarks.py:95: error: Item "BasicBookmark" of "BasicBookmark | None" has no attribute
"add_bookmark" [union-attr]
bookmark_instance.add_bookmark(bookmark_data)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/test_bookmarks.py:95: error: Item "None" of "BasicBookmark | None" has no attribute "add_bookmark"
[union-attr]
bookmark_instance.add_bookmark(bookmark_data)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/test_bookmarks.py:96: error: Incompatible return value type (got "BasicBookmark | None", expected
"AdvancedBookmark") [return-value]
return bookmark_instanceI will do a different work around for this when I refactor it.
There was a problem hiding this comment.
This is a good error, but the solution isn't what you currently have. Looking forward to seeing how you figure this out
| with window.selenium.context(window.selenium.CONTEXT_CHROME): | ||
| try: | ||
| return BasicBookmark(window, root) | ||
| except NoSuchElementException: | ||
| return None |
There was a problem hiding this comment.
I am curious as to why you'd like to return None and not throw the error.
There was a problem hiding this comment.
I think it might be less verbose. But, now again, it could confusing and unclear. I will just correct that.
|
Let me know when you'd like me to look at this again. |
Yeah, you can. Thanks. |
Well I added a comment above too |
|
Hi @b4handjr The code is now ready for another review. Thanks. |
There was a problem hiding this comment.
Lots of great work here, this is really coming together! I would still like to see some consolidation and clarification with it comes to adding or deleting a bookmark. Also, there are some isinstance checks that I think aren't needed as we are using types to verify that.
| return True | ||
| return False | ||
|
|
||
| def add(self) -> None: |
There was a problem hiding this comment.
What's the difference with add() or add_bookmark()?
There was a problem hiding this comment.
I should probably use a flag instead and combine both into one method so it isn't confusing
| if tags is not None: | ||
| for tag in tags: | ||
| self.actions.send_keys(tag).perform() | ||
| self.actions.send_keys(",").perform() | ||
| self.actions.send_keys(Keys.TAB).perform() |
There was a problem hiding this comment.
Think you could use the walrus operator here? https://peps.python.org/pep-0572/
| if panel_bookmarks: | ||
| menu_items = panel_bookmarks.find_elements( |
There was a problem hiding this comment.
Why do you check if the panel_bookmarks is true? Wouldn't selenium throw an error if it couldn't find the element needed?
| for item in menu_items: | ||
| item_label = item.get_attribute("label") | ||
| if item_label and label.lower() in item_label.lower(): | ||
| return True |
There was a problem hiding this comment.
This could probably be done with a list comprehension.
There was a problem hiding this comment.
I thought to go with a generator expression here which might better fit, since the intention here is not to create a list, but to evaluate a condition.
foxpuppet/windows/browser/navbar.py
Outdated
| _tracking_protection_shield_locator: Tuple[str, str] = ( | ||
| By.ID, | ||
| "tracking-protection-icon-box", | ||
| ) |
| self.actions.send_keys(Keys.TAB, Keys.TAB, Keys.TAB, Keys.ENTER).perform() | ||
| self.actions.send_keys(Keys.TAB, Keys.ENTER).perform() | ||
|
|
||
| self.selenium.switch_to.default_content() |
There was a problem hiding this comment.
Using the with clause, we don't need to switch back to default content here.
| if frame_element is not None: | ||
| self.selenium.switch_to.frame(frame_element) | ||
| else: | ||
| raise NoSuchElementException( | ||
| f"iFrame with locator {BookmarkLocators.ADD_BOOKMARK_FRAME} not found." | ||
| ) |
There was a problem hiding this comment.
Selenium will raise an error before the program gets here, if you want to raise an error with specific context you'll have to use a try/except block.
foxpuppet/region.py
Outdated
| from foxpuppet.windows import BaseWindow | ||
| from typing import Tuple |
There was a problem hiding this comment.
I don't this this is used in here?
foxpuppet/region.py
Outdated
| @@ -6,7 +6,10 @@ | |||
| from selenium.webdriver.remote.webelement import WebElement | |||
| from selenium.webdriver.support.wait import WebDriverWait | |||
| from selenium.webdriver.remote.webdriver import WebDriver | |||
| from selenium.webdriver.common.action_chains import ActionChains | |||
| from selenium.common.exceptions import NoSuchElementException | |||
There was a problem hiding this comment.
I don't this this is used in here?
Temidayo32
force-pushed
the
bookmark
branch
5 times, most recently
from
a831f09 to
49ecba8
Compare
| ).perform() | ||
| self.selenium.find_element(*BookmarkLocators.ADD_BOOKMARK).click() | ||
|
|
||
| frame_element = self.selenium.find_element( |
There was a problem hiding this comment.
Could we name this bookmark_frame or something like that?
| if bookmark_data is not None: | ||
| if bookmark_data["name"]: |
There was a problem hiding this comment.
I think you could use the walrus operator here, and it would allow bookmark_data to be used bellow as well.
| if ( | ||
| folder := self.selenium.find_element( | ||
| *BookmarkLocators.BOOKMARK_FOLDER | ||
| ) | ||
| ) is not None: |
There was a problem hiding this comment.
I don't think we need to do this None checks. You can remove the is not None part and do the same check as well. Also, good use of the walrus operator!
| def retrieve_bookmark(self, label: str) -> bool: | ||
| """ | ||
| Check if a bookmark with the given label exists. | ||
|
|
There was a problem hiding this comment.
The function name reads as if this will return a bookmark but it seems to be more of a check. Can you update the name to reflect that?
| self, label: Optional[str] = None, is_detailed: bool = False | ||
| ) -> None: | ||
| """ | ||
| Delete a bookmark using either quick delete (star button) or detailed menu approach. |
There was a problem hiding this comment.
I wonder if this is overly complex? Does it matter how a bookmark is deleted?
There was a problem hiding this comment.
The only reason I added the more detailed approach is because if we test adding a bookmark using the more detailed approach, then we should do same with the delete.
| if not is_detailed: | ||
| star_button_image = self.selenium.find_element( | ||
| *BookmarkLocators.STAR_BUTTON_IMAGE | ||
| ) | ||
| if ( | ||
| star_button_image | ||
| and star_button_image.get_attribute("starred") == "true" | ||
| ): | ||
| self.selenium.find_element(*BookmarkLocators.STAR_BUTTON).click() | ||
| self.selenium.find_element(*BookmarkLocators.REMOVE_BUTTON).click() | ||
| else: | ||
| self.actions.context_click( |
There was a problem hiding this comment.
For operations like this where you have just 1 condition to check you can just have an if with the expression you want to evaluate ending with a return and then outside of that if block you can add the actions that will be done if the expression evaluates opposing to what you wanted.
So something like
if True:
code here if true
return
code here if false
Does that make sense?
| class BookmarkData(TypedDict): | ||
| """Bookmark properties.""" | ||
|
|
||
| name: str | ||
| url: str | ||
| tags: Optional[List[str]] | ||
| keyword: Optional[str] |
Temidayo32
force-pushed
the
bookmark
branch
3 times, most recently
from
587f95f to
1c42add
Compare
Moved all bookmark functionalities into a single class, Bookmark
This PR add functionalities to set, unset, and retrieve bookmarks
Because
This commit
Fixes #312