Added BasePage for reusable Playwright actions#217
Open
priy-tech wants to merge 3 commits intonirtal85:mainfrom
Open
Added BasePage for reusable Playwright actions#217priy-tech wants to merge 3 commits intonirtal85:mainfrom
priy-tech wants to merge 3 commits intonirtal85:mainfrom
Conversation
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pages/base_page.py (1)
19-41: Support bothstrandLocatorin BasePage methods for better flexibility.Currently these methods only accept strings. Adding support for
Locatorinstances via a simple resolver makes the wrapper more useful for pages that work with pre-stored locators, and aligns with patterns already seen in the codebase.Proposed refactor
-from playwright.sync_api import Page, expect +from playwright.sync_api import Page, Locator, expect +from typing import Union class BasePage: """ BasePage provides common Playwright actions that can be reused across all Page Objects. """ + def _resolve(self, locator: Union[str, Locator]) -> Locator: + return locator if isinstance(locator, Locator) else self.page.locator(locator) + - def click(self, locator: str) -> None: + def click(self, locator: Union[str, Locator]) -> None: """Click on a web element""" - self.page.locator(locator).click() + self._resolve(locator).click() - def fill(self, locator: str, value: str) -> None: + def fill(self, locator: Union[str, Locator], value: str) -> None: """Fill input field with value""" - self.page.locator(locator).fill(value) + self._resolve(locator).fill(value) - def get_text(self, locator: str) -> str: + def get_text(self, locator: Union[str, Locator]) -> str: """Return text of an element""" - return self.page.locator(locator).inner_text() + return self._resolve(locator).inner_text() - def is_visible(self, locator: str) -> bool: + def is_visible(self, locator: Union[str, Locator]) -> bool: """Check if element is visible""" - return self.page.locator(locator).is_visible() + return self._resolve(locator).is_visible() - def wait_for(self, locator: str) -> None: + def wait_for(self, locator: Union[str, Locator]) -> None: """Wait until element is visible""" - self.page.locator(locator).wait_for() + self._resolve(locator).wait_for() - def assert_text(self, locator: str, expected_text: str) -> None: + def assert_text(self, locator: Union[str, Locator], expected_text: str) -> None: """Assert exact text of an element""" - expect(self.page.locator(locator)).to_have_text(expected_text) + expect(self._resolve(locator)).to_have_text(expected_text)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/base_page.py` around lines 19 - 41, The BasePage methods (click, fill, get_text, is_visible, wait_for, assert_text) currently accept only locator strings; modify them to accept either str or a Playwright Locator by adding a small resolver helper (e.g., _resolve_locator) that returns a Locator: if the argument is a str call self.page.locator(arg) else assume it's already a Locator and return it. Update type hints to Union[str, Locator] (import Locator from playwright.sync_api and Union from typing) and have each method use the resolver (e.g., locator = self._resolve_locator(locator)) before calling click(), fill(), inner_text(), is_visible(), wait_for(), or expect(...).to_have_text(...) respectively so pre-stored Locator objects are supported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pages/base_page.py`:
- Around line 19-41: The BasePage methods (click, fill, get_text, is_visible,
wait_for, assert_text) currently accept only locator strings; modify them to
accept either str or a Playwright Locator by adding a small resolver helper
(e.g., _resolve_locator) that returns a Locator: if the argument is a str call
self.page.locator(arg) else assume it's already a Locator and return it. Update
type hints to Union[str, Locator] (import Locator from playwright.sync_api and
Union from typing) and have each method use the resolver (e.g., locator =
self._resolve_locator(locator)) before calling click(), fill(), inner_text(),
is_visible(), wait_for(), or expect(...).to_have_text(...) respectively so
pre-stored Locator objects are supported.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
161-165: Consider making this section directly actionable.At Line 163, add the concrete path (e.g.,
src/pages/base_page.py) and a short “how to use in page objects” snippet or link so readers can jump from concept to implementation quickly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 161 - 165, Update the "Enhancements by Me" section to be actionable by adding the concrete location of the BasePage implementation and a short usage snippet showing how page objects extend and call its methods: mention the BasePage class name and where it lives, then show a minimal example of "class MyPage(BasePage):" with an __init__ that accepts a page/driver, and example calls to BasePage methods like click(selector) and fill(selector, value) so readers can copy-paste into their page objects or follow a link to the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Around line 161-165: Update the "Enhancements by Me" section to be actionable
by adding the concrete location of the BasePage implementation and a short usage
snippet showing how page objects extend and call its methods: mention the
BasePage class name and where it lives, then show a minimal example of "class
MyPage(BasePage):" with an __init__ that accepts a page/driver, and example
calls to BasePage methods like click(selector) and fill(selector, value) so
readers can copy-paste into their page objects or follow a link to the
implementation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduced BasePage to centralize reusable Playwright actions and improve framework maintainability.
Summary by CodeRabbit