Add Playwright-based YouTube UI tests using Optics#227
Open
sheshnath1st wants to merge 31 commits intomozarkai:mainfrom
Open
Add Playwright-based YouTube UI tests using Optics#227sheshnath1st wants to merge 31 commits intomozarkai:mainfrom
sheshnath1st wants to merge 31 commits intomozarkai:mainfrom
Conversation
…egration Signed-off-by: Dhruv Menon <dhruvmenon1104@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request introduces Playwright-based browser automation support for the Optics framework, adding web UI testing capabilities for YouTube. The implementation includes a complete Playwright driver, element source implementations, async utilities, and test examples.
Changes:
- Added Playwright driver and three element source implementations (find_element, page_source, screenshot)
- Created async utility module to handle Playwright's async operations from synchronous code
- Added YouTube web UI test examples demonstrating the framework's capabilities
- Updated element type detection in utils.py to support CSS selectors and Playwright-specific syntax
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/feature/engine/test_web_youtube.py | Primary YouTube test suite with independent test cases for launch, click, and search operations |
| tests/feature/engine/test_playwright_youtube_backup.py | Backup test file containing experimental/debug tests with fixture naming issues |
| optics_framework/engines/drivers/playwright.py | Core Playwright driver implementation with browser lifecycle and interaction methods |
| optics_framework/engines/elementsources/playwright_screenshot.py | Screenshot capture implementation using Playwright |
| optics_framework/engines/elementsources/playwright_page_source.py | Page DOM parsing and element extraction with XPath generation |
| optics_framework/engines/elementsources/playwright_find_element.py | Element location using Playwright locators |
| optics_framework/common/async_utils.py | Utility for running async coroutines from sync code with persistent event loop |
| optics_framework/common/utils.py | Enhanced element type detection for CSS selectors and Playwright syntax |
| optics_framework/common/strategies.py | Updated to support CSS element type and added null checks |
| optics_framework/optics.py | Increased default timeout from 30 to 60 seconds |
| requirements.txt | Added dependencies for Playwright-based testing |
| optics_framework/samples/youtube_web/config.yaml | Configuration file with hardcoded absolute paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
|
|
||
|
|
||
| def load_config(path): |
There was a problem hiding this comment.
Missing return type hints. The function should declare its return type to improve code clarity and enable better IDE support. Add '-> str' return type annotation.
Suggested change
| def load_config(path): | |
| def load_config(path) -> str: |
| # --------------------------------------------------------- | ||
| optics.press_element('input[name="search_query"]') | ||
| optics.enter_text_using_keyboard( | ||
| "Wild Stone Edge Perfume Revie" |
There was a problem hiding this comment.
Typo in search text. The search term "Wild Stone Edge Perfume Revie" appears to be missing the final 'w' in "Review". This should be corrected to "Wild Stone Edge Perfume Review" to match the intended search query.
Suggested change
| "Wild Stone Edge Perfume Revie" | |
| "Wild Stone Edge Perfume Review" |
Comment on lines
145
to
146
| html = optics.capture_pagesource() # Tested not sure | ||
| html = optics.capture_pagesource() # Working |
There was a problem hiding this comment.
Redundant duplicate function call. Line 146 duplicates the exact same call from line 145 (optics.capture_pagesource()). The comment "# Tested not sure" on line 145 and "# Working" on line 146 suggests this is debug code. Remove the redundant call.
Suggested change
| html = optics.capture_pagesource() # Tested not sure | |
| html = optics.capture_pagesource() # Working | |
| html = optics.capture_pagesource() # Working |
Comment on lines
1
to
191
| import os | ||
| import yaml | ||
| import pytest | ||
|
|
||
| from optics_framework.common.async_utils import run_async | ||
| from optics_framework.optics import Optics | ||
|
|
||
|
|
||
| PLAYWRIGHT_CONFIG_PATH = os.path.join( | ||
| os.path.dirname(__file__), | ||
| "../../../optics_framework/samples/playwright/config.yaml" | ||
| ) | ||
|
|
||
|
|
||
| def load_config(path): | ||
| with open(path, "r", encoding="utf-8") as f: | ||
| return yaml.safe_load(f) | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def optics(): | ||
| """ | ||
| BEFORE all tests in the module | ||
| """ | ||
| config = load_config(PLAYWRIGHT_CONFIG_PATH) | ||
|
|
||
| optics = Optics() | ||
| optics.setup(config=config) | ||
|
|
||
| # Launch YouTube once | ||
| optics.launch_app("https://www.youtube.com") | ||
|
|
||
| yield optics | ||
|
|
||
| # AFTER all tests | ||
| optics.quit() | ||
|
|
||
|
|
||
| def test_youtube_launch(optics): | ||
| """ | ||
| Smoke test: YouTube loads and search box is visible | ||
| """ | ||
| optics.assert_presence('//input[@name="search_query"]') | ||
|
|
||
|
|
||
| def test_youtube_click_search_box(optics): | ||
| """ | ||
| Verify search box can be clicked | ||
| """ | ||
| optics.press_element('//input[@name="search_query"]') | ||
|
|
||
| def test_youtube_click_search_box_and_pressKeyCode(optics): | ||
| """ | ||
| Verify search box can be clicked | ||
| """ | ||
| optics.press_element('//input[@name="search_query"]') | ||
| optics.sleep("2") | ||
|
|
||
| def test_youtube_search(optics): | ||
| """ | ||
| Enter search text and submit | ||
| """ | ||
| optics.press_element('//input[@name="search_query"]') | ||
| optics.enter_text_using_keyboard("Wild Stone Edge Perfume Review") | ||
| optics.sleep("2") | ||
| optics.press_keycode("Enter") | ||
| optics.sleep("2") | ||
|
|
||
|
|
||
| def test_youtube_scroll_and_capture(optics): | ||
| """ | ||
| Scroll page and capture artifacts | ||
| """ | ||
| optics.press_element('//input[@name="search_query"]') | ||
| optics.enter_text_using_keyboard("Wild Stone Edge Perfume Review") | ||
| optics.press_keycode("Enter") | ||
| optics.sleep("2") | ||
| optics.scroll_until_element_appears("Better than") | ||
| optics.capture_screenshot() | ||
|
|
||
| page_source = optics.capture_pagesource() | ||
| assert page_source is not None | ||
|
|
||
|
|
||
| def test_youtube_launch_app(optics_instance): | ||
| optics = optics_instance | ||
| # --------------------------------------------------------- | ||
| # Step 1: Launch YouTube | ||
| # --------------------------------------------------------- | ||
| optics.launch_app("https://www.youtube.com") | ||
|
|
||
| def test_youtube_assert_presence(optics_instance): | ||
| optics = optics_instance | ||
| # --------------------------------------------------------- | ||
| # Step 1: Launch YouTube | ||
| # --------------------------------------------------------- | ||
| optics.launch_app("https://www.youtube.com") | ||
| optics.assert_presence('input[name="search_query"]') | ||
|
|
||
|
|
||
|
|
||
| def test_youtube_search_and_play(optics_instance): | ||
| optics = optics_instance | ||
|
|
||
| # --------------------------------------------------------- | ||
| # Step 1: Launch YouTube | ||
| # --------------------------------------------------------- | ||
| optics.launch_app("https://www.youtube.com") | ||
|
|
||
| # --------------------------------------------------------- | ||
| # Step 2: Register elements | ||
| # --------------------------------------------------------- | ||
| optics.add_element("search_box", 'input[name="search_query"]') | ||
| optics.add_element("video_title", 'text=Wild Stone Edge') | ||
|
|
||
| # --------------------------------------------------------- | ||
| # Step 3: Assert search box | ||
| # --------------------------------------------------------- | ||
| optics.assert_presence('input[name="search_query"]') | ||
|
|
||
| # --------------------------------------------------------- | ||
| # Step 4: Search | ||
| # --------------------------------------------------------- | ||
| optics.press_element('input[name="search_query"]') | ||
| optics.enter_text_using_keyboard( | ||
| "Wild Stone Edge Perfume Revie" | ||
| ) | ||
| optics.press_keycode("Enter") | ||
| optics.scroll("down") | ||
| optics.scroll("down") | ||
| optics.sleep(1) | ||
| optics.scroll("down") | ||
| optics.sleep(1) | ||
|
|
||
| # --------------------------------------------------------- | ||
| # Step 5: Scroll until video appears | ||
| # --------------------------------------------------------- | ||
| optics.add_element("search_box", 'input[name="search_query"]') | ||
| optics.add_element("video_title", 'text=Wild Stone Edge') | ||
| optics.scroll_until_element_appears( | ||
| "Wild Stone Edge Perfume Review", | ||
| direction="down" | ||
| ) | ||
| optics.sleep("10") | ||
| html = optics.capture_pagesource() # Tested not sure | ||
| html = optics.capture_pagesource() # Working | ||
| print("========== PAGE SOURCE (XML TREE) ==========") | ||
| # print(html) | ||
| # print(run_async(optics.app_management.driver.page.content())) | ||
| # print("========================= XML Tree =========================") | ||
| # print(html) | ||
| # execution_logger.info("========================= XML Tree log =========================") | ||
| # execution_logger.info(html) | ||
| # print("========================= XML Tree =========================") | ||
| optics.capture_screenshot() # Tested Working | ||
| version = optics.get_app_version() # Tested Working | ||
| print(version) | ||
| # optics.press_by_percentage("50","50") # Tested and Working | ||
| # optics.press_element_with_index() | ||
| # optics.detect_and_press("Best Budget Perfume for Men") # Tested and Working | ||
| # optics.swipe("1500","1500","up") # Tested and Working | ||
| # optics.swipe_until_element_appears("Best Budget Perfume for Men") # Tested and Working | ||
| # optics.swipe_from_element("wild stone edge perfume","up","100") # Tested and Working | ||
| # optics.swipe_from_element("wild stone edge perfume", "down","100") # Tested and Working | ||
| optics.scroll_until_element_appears("Better than") | ||
| # optics.scroll_from_element() | ||
| # optics.enter_text() | ||
| # optics.enter_text_direct() | ||
| # optics.enter_number() | ||
| # optics.press_keycode() | ||
| # optics.clear_element_text() | ||
| # optics.get_text() | ||
| # optics.validate_screen() | ||
|
|
||
|
|
||
|
|
||
| optics.validate_element("Best Budget Perfume for Men ₹356") | ||
| optics.press_element("Best Budget Perfume for Men ₹356") | ||
| optics.assert_presence("Wild stone Edge perfume review") | ||
|
|
||
| optics.sleep("10") | ||
| optics.quit() | ||
|
|
||
| if __name__ == "__main__": | ||
| import pytest | ||
| pytest.main([ | ||
| __file__, | ||
| "-v", | ||
| "-s", | ||
| "--log-cli-level=DEBUG" | ||
| ]) No newline at end of file |
There was a problem hiding this comment.
Misleading filename. This file is named "test_playwright_youtube_backup.py" suggesting it's a backup, but it appears to be committed to version control. Backup files should not be committed. Either remove this file if it's truly a backup, or rename it appropriately if it contains valid test code.
Comment on lines
234
to
250
| # def scroll(self, direction: str, pixels: int, event_name=None): | ||
| # execution_logger.debug( | ||
| # "[Playwright] scroll direction=%s pixels=%d", | ||
| # direction, pixels | ||
| # ) | ||
| # run_async(self._scroll_async(direction, pixels)) | ||
|
|
||
| # async def _scroll_async(self, direction: str, pixels: int): | ||
| # if direction == "down": | ||
| # await self.page.evaluate( | ||
| # "(p) => window.scrollBy(0, p)", pixels | ||
| # ) | ||
| # else: | ||
| # await self.page.evaluate( | ||
| # "(p) => window.scrollBy(0, -p)", pixels | ||
| # ) | ||
|
|
There was a problem hiding this comment.
Commented-out code should be removed. Lines 221-249 contain extensive commented-out code. Version control systems track history, so there's no need to keep commented code in the codebase. This clutters the file and reduces maintainability.
Suggested change
| # def scroll(self, direction: str, pixels: int, event_name=None): | |
| # execution_logger.debug( | |
| # "[Playwright] scroll direction=%s pixels=%d", | |
| # direction, pixels | |
| # ) | |
| # run_async(self._scroll_async(direction, pixels)) | |
| # async def _scroll_async(self, direction: str, pixels: int): | |
| # if direction == "down": | |
| # await self.page.evaluate( | |
| # "(p) => window.scrollBy(0, p)", pixels | |
| # ) | |
| # else: | |
| # await self.page.evaluate( | |
| # "(p) => window.scrollBy(0, -p)", pixels | |
| # ) |
Comment on lines
85
to
91
| def test_youtube_launch_app(optics_instance): | ||
| optics = optics_instance | ||
| # --------------------------------------------------------- | ||
| # Step 1: Launch YouTube | ||
| # --------------------------------------------------------- | ||
| optics.launch_app("https://www.youtube.com") | ||
|
|
There was a problem hiding this comment.
Misleading function name and implementation. The test references a fixture named 'optics_instance' which doesn't exist in this file. The available fixture is named 'optics'. This will cause the test to fail at runtime.
Comment on lines
4
to
9
| from typing import Any, Coroutine | ||
| from optics_framework.common.logging_config import internal_logger | ||
| from optics_framework.common.error import OpticsError, Code | ||
|
|
||
| _persistent_loop: asyncio.AbstractEventLoop | None = None | ||
| _loop_thread: threading.Thread | None = None |
There was a problem hiding this comment.
Python 3.10+ union type syntax used without version constraint. Line 8 uses 'asyncio.AbstractEventLoop | None' which requires Python 3.10+. Either add a Python version constraint to the project or use 'Optional[asyncio.AbstractEventLoop]' for compatibility with earlier Python versions.
Suggested change
| from typing import Any, Coroutine | |
| from optics_framework.common.logging_config import internal_logger | |
| from optics_framework.common.error import OpticsError, Code | |
| _persistent_loop: asyncio.AbstractEventLoop | None = None | |
| _loop_thread: threading.Thread | None = None | |
| from typing import Any, Coroutine, Optional | |
| from optics_framework.common.logging_config import internal_logger | |
| from optics_framework.common.error import OpticsError, Code | |
| _persistent_loop: Optional[asyncio.AbstractEventLoop] = None | |
| _loop_thread: Optional[threading.Thread] = None |
Comment on lines
145
to
149
| html = optics.capture_pagesource() # Tested not sure | ||
| html = optics.capture_pagesource() # Working | ||
| print("========== PAGE SOURCE (XML TREE) ==========") | ||
| # print(html) | ||
| # print(run_async(optics.app_management.driver.page.content())) |
There was a problem hiding this comment.
Variable html is not used.
Suggested change
| html = optics.capture_pagesource() # Tested not sure | |
| html = optics.capture_pagesource() # Working | |
| print("========== PAGE SOURCE (XML TREE) ==========") | |
| # print(html) | |
| # print(run_async(optics.app_management.driver.page.content())) | |
| optics.capture_pagesource() # Working | |
| print("========== PAGE SOURCE (XML TREE) ==========") | |
| # print(html) | |
| # print(run_async(optics.app_management.driver.page.content())) | |
| # print(run_async(optics.app_management.driver.page.content())) |
Comment on lines
145
to
146
| html = optics.capture_pagesource() # Tested not sure | ||
| html = optics.capture_pagesource() # Working |
There was a problem hiding this comment.
This assignment to 'html' is unnecessary as it is redefined before this value is used.
Suggested change
| html = optics.capture_pagesource() # Tested not sure | |
| html = optics.capture_pagesource() # Working | |
| html = optics.capture_pagesource() # Working |
|
|
||
| try: | ||
| asyncio.get_running_loop() | ||
| except RuntimeError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Suggested change
| except RuntimeError: | |
| except RuntimeError: | |
| # No running event loop in this thread; this is expected and safe to ignore |
# This is the 1st commit message: test(playwright): add unit test for app launch # This is the commit message mozarkai#2: test(playwright): verify youtube search box is clickable # This is the commit message mozarkai#3: test(playwright): verify youtube search box click with delay # This is the commit message mozarkai#4: test(playwright): change config sample # This is the commit message mozarkai#5: test(playwright): add youtube search box click and sleep test # This is the commit message mozarkai#6: test(playwright): add comprehensive framework feature coverage launch_app , add_and_get_element , clear_element_text , _flow , validation_methods , get_text
sheshnath1st
force-pushed
the
feat_playwright
branch
from
6b15862 to
326c42a
Compare
… logic to eliminate duplication
sheshnath1st
force-pushed
the
feat_playwright
branch
from
326c42a to
f4398a9
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Enter search text and submit | ||
| """ | ||
| optics.press_element('//input[@name="search_query"]') | ||
| optics.enter_text_using_keyboard("Wild Stone Edge Perfume Review") |
There was a problem hiding this comment.
The search query "Wild Stone Edge Perfume Review" appears to be test-specific content. Consider using a more generic or parameterized search term, or document why this specific query is used for testing purposes.
Comment on lines
+189
to
+195
| internal_logger.error("trying get_page_source ..............") | ||
|
|
||
| # Ensure Playwright page is initialized and available | ||
| page = self._require_page() | ||
|
|
||
| # Secondary diagnostic log to confirm successful page resolution | ||
| internal_logger.error("trying get_page_source _require_page ..............") |
There was a problem hiding this comment.
Using internal_logger.error() for diagnostic logging is misleading. The log statements on lines 189 and 195 are not errors but rather debug/trace information. These should use internal_logger.debug() instead to avoid polluting error logs with non-error messages.
Suggested change
| internal_logger.error("trying get_page_source ..............") | |
| # Ensure Playwright page is initialized and available | |
| page = self._require_page() | |
| # Secondary diagnostic log to confirm successful page resolution | |
| internal_logger.error("trying get_page_source _require_page ..............") | |
| internal_logger.debug("trying get_page_source ..............") | |
| # Ensure Playwright page is initialized and available | |
| page = self._require_page() | |
| # Secondary diagnostic log to confirm successful page resolution | |
| internal_logger.debug("trying get_page_source _require_page ..............") |
Comment on lines
+1002
to
+1023
| - If the string contains ONLY double quotes → wrap with single quotes. | ||
| - If the string contains ONLY single quotes → wrap with double quotes. | ||
| - If the string contains BOTH quote types → use XPath `concat()` to | ||
| assemble the literal safely. | ||
|
|
||
| Why `concat()` is required: | ||
| - XPath does not support escaping quotes inside literals. | ||
| - `concat()` allows us to split the string into safe fragments and | ||
| reconstruct it at runtime inside the XPath engine. | ||
|
|
||
| Design considerations: | ||
| - This method is deterministic and side-effect free. | ||
| - It performs no XPath execution—only string transformation. | ||
| - Centralizing this logic avoids subtle XPath bugs scattered across code. | ||
|
|
||
| Args: | ||
| s (str): | ||
| Raw string value that may contain single and/or double quotes. | ||
|
|
||
| Returns: | ||
| str: | ||
| A valid XPath string literal representation of `s`. |
There was a problem hiding this comment.
Similarly, the docstring for _build_extra_metadata (lines 991-1024) actually describes the _escape_for_xpath_literal functionality. The docstrings for these two methods appear to be swapped. This will confuse developers trying to understand what each method does.
Suggested change
| - If the string contains ONLY double quotes → wrap with single quotes. | |
| - If the string contains ONLY single quotes → wrap with double quotes. | |
| - If the string contains BOTH quote types → use XPath `concat()` to | |
| assemble the literal safely. | |
| Why `concat()` is required: | |
| - XPath does not support escaping quotes inside literals. | |
| - `concat()` allows us to split the string into safe fragments and | |
| reconstruct it at runtime inside the XPath engine. | |
| Design considerations: | |
| - This method is deterministic and side-effect free. | |
| - It performs no XPath execution—only string transformation. | |
| - Centralizing this logic avoids subtle XPath bugs scattered across code. | |
| Args: | |
| s (str): | |
| Raw string value that may contain single and/or double quotes. | |
| Returns: | |
| str: | |
| A valid XPath string literal representation of `s`. | |
| Build a dictionary of "extra" metadata for an element from its attributes. | |
| This helper is used to derive additional context about a DOM element | |
| without duplicating the primary attribute that has already been used to | |
| identify or locate the element (specified by ``used_key``). | |
| The method: | |
| - Starts from the full ``attrs`` mapping provided by Playwright. | |
| - Excludes the attribute named by ``used_key``. | |
| - Filters out falsy values and string values equal to ``"false"`` (case- | |
| insensitive), so only meaningful metadata is kept. | |
| - Ensures that a set of common, high-value attributes are explicitly | |
| present in the result (even if missing from the filtered subset), | |
| such as: ``tag``, ``class``, ``id``, ``role``, ``type``, and ``href``. | |
| This centralizes metadata construction, making logging and diagnostics | |
| consistent across the codebase. | |
| Args: | |
| tag (str): | |
| The tag name of the element (for example, ``"button"`` or | |
| ``"input"``), recorded explicitly in the metadata. | |
| attrs (Dict[str, Any]): | |
| Raw attribute mapping for the element as returned by Playwright. | |
| used_key (str): | |
| The name of the primary attribute already used for element | |
| identification and therefore excluded from the "extra" metadata. | |
| Returns: | |
| Dict[str, Any]: | |
| A dictionary of additional, filtered attributes and common fields | |
| describing the element. |
| self, | ||
| elements: fallback_str, | ||
| timeout: fallback_str = "30", | ||
| timeout: fallback_str = "60", |
There was a problem hiding this comment.
The timeout value has been doubled from 30 to 60 seconds without any explanation. This significant change to default wait behavior should be documented in code comments or the PR description. Consider whether this timeout increase is appropriate for all use cases, as it could slow down test execution when elements are genuinely missing.
Suggested change
| timeout: fallback_str = "60", | |
| timeout: fallback_str = "60", # Intentionally higher default (60s) to reduce flakiness on slower environments; override in tests if a shorter wait is acceptable. |
Comment on lines
51
to
57
| def test_youtube_click_search_box_and_sleep(optics): | ||
| """ | ||
| Verify search box can be clicked | ||
| """ | ||
| optics.press_element('//input[@name="search_query"]') | ||
| optics.sleep("2") | ||
|
|
There was a problem hiding this comment.
The test case "test_youtube_click_search_box_and_sleep" is redundant with "test_youtube_click_search_box" (lines 45-49) and only adds a sleep call. This duplicates test logic unnecessarily. Consider removing this test or merging it with test_youtube_search which also includes the necessary sleep for stability.
Suggested change
| def test_youtube_click_search_box_and_sleep(optics): | |
| """ | |
| Verify search box can be clicked | |
| """ | |
| optics.press_element('//input[@name="search_query"]') | |
| optics.sleep("2") |
Comment on lines
+947
to
+974
| Construct a hierarchical XPath based purely on DOM structure. | ||
|
|
||
| Design intent: | ||
| - Provide a deterministic fallback when attribute-based XPath generation | ||
| is not possible or not reliable. | ||
| - Ensure every element can still be addressed, even in the absence of | ||
| unique attributes (id, name, data-testid, etc.). | ||
| - Preserve DOM order by using positional indices where required. | ||
|
|
||
| Characteristics: | ||
| - This method relies ONLY on parent-child relationships. | ||
| - XPath segments are built from the target node up to the root. | ||
| - Positional indices ([n]) are added only when siblings share the same tag. | ||
| - The resulting XPath is stable for a given DOM structure but may change | ||
| if the DOM hierarchy itself changes. | ||
|
|
||
| Important notes: | ||
| - This method does not validate uniqueness globally. | ||
| - It does not interact with Playwright or perform runtime queries. | ||
| - It is intentionally simple and predictable to avoid side effects. | ||
|
|
||
| Args: | ||
| node (etree.Element): | ||
| The lxml DOM element for which the hierarchical XPath is generated. | ||
|
|
||
| Returns: | ||
| str: | ||
| A hierarchical XPath string, or an empty string if the node is invalid. |
There was a problem hiding this comment.
The docstring for _escape_for_xpath_literal (lines 945-975) is misplaced. It describes the behavior of _build_hierarchical_xpath but is attached to _escape_for_xpath_literal. The docstring should accurately describe the escape functionality (handling quotes in XPath literals), not hierarchical XPath construction.
Suggested change
| Construct a hierarchical XPath based purely on DOM structure. | |
| Design intent: | |
| - Provide a deterministic fallback when attribute-based XPath generation | |
| is not possible or not reliable. | |
| - Ensure every element can still be addressed, even in the absence of | |
| unique attributes (id, name, data-testid, etc.). | |
| - Preserve DOM order by using positional indices where required. | |
| Characteristics: | |
| - This method relies ONLY on parent-child relationships. | |
| - XPath segments are built from the target node up to the root. | |
| - Positional indices ([n]) are added only when siblings share the same tag. | |
| - The resulting XPath is stable for a given DOM structure but may change | |
| if the DOM hierarchy itself changes. | |
| Important notes: | |
| - This method does not validate uniqueness globally. | |
| - It does not interact with Playwright or perform runtime queries. | |
| - It is intentionally simple and predictable to avoid side effects. | |
| Args: | |
| node (etree.Element): | |
| The lxml DOM element for which the hierarchical XPath is generated. | |
| Returns: | |
| str: | |
| A hierarchical XPath string, or an empty string if the node is invalid. | |
| Escape a Python string so it can be safely used as an XPath string literal. | |
| XPath string literals can be delimited by either single quotes (') or | |
| double quotes ("). If the value contains only one type of quote, this | |
| method wraps the string in the other type. If it contains both single | |
| and double quotes, it builds a `concat(...)` expression that joins | |
| appropriately quoted segments and embedded quote characters. | |
| Args: | |
| s (str): | |
| The raw string value to be embedded in an XPath expression. | |
| Returns: | |
| str: | |
| A string that is a syntactically valid XPath string literal | |
| (or `concat(...)` expression) representing the input value. |
Comment on lines
+501
to
+502
| result = self.element_source.locate(element) | ||
| return self.driver.get_text_element(result) |
There was a problem hiding this comment.
The change to get_text implementation adds fallback logic to use element_source.locate and driver.get_text_element. However, this code path lacks error handling. If either locate or get_text_element fails, it could raise an unhandled exception. Consider adding try-except blocks or null checks to handle cases where the element cannot be located or text cannot be retrieved.
| optics = Optics() | ||
| optics.setup(config=config) | ||
|
|
||
| optics.launch_app("https://www.saucedemo.com/") |
There was a problem hiding this comment.
The test file name and fixture launch "saucedemo.com" instead of YouTube. This is inconsistent with the PR title and description which states these are "YouTube UI tests". The file should either be renamed to reflect testing of saucedemo, or the tests should actually interact with YouTube.
| try: | ||
| if condition_fn(): | ||
| return True | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Suggested change
| except Exception: | |
| except Exception: | |
| # Intentionally ignore all exceptions from condition_fn and continue | |
| # retrying until the timeout is reached. Transient errors (e.g., DOM | |
| # changes, navigation) are expected and should not break the loop. |
…duce assertion complexity
…ognitive complexity
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+39
to
+174
| def test_framework_launch_app(optics): | ||
| optics.assert_presence('//input[@id="user-name"]') | ||
| optics.assert_presence('//input[@id="password"]') | ||
|
|
||
|
|
||
| # --------------------------------------------------------- | ||
| # TEST 2: add_element + get_element_value | ||
| # --------------------------------------------------------- | ||
| def test_framework_add_and_get_element(optics): | ||
| optics.add_element("username_input", '//input[@id="user-name"]') | ||
| optics.add_element("password_input", '//input[@id="password"]') | ||
|
|
||
| value = optics.get_element_value("username_input") | ||
|
|
||
| assert isinstance(value, list) | ||
| assert value == ['//input[@id="user-name"]'] | ||
|
|
||
|
|
||
| # --------------------------------------------------------- | ||
| # TEST 3: enter_text_using_keyboard + clear_element_text | ||
| # --------------------------------------------------------- | ||
| def test_framework_clear_element_text(optics): | ||
| optics.add_element("username_input", '//input[@id="user-name"]') | ||
| # Focus input | ||
| username = optics.get_element_value("username_input") | ||
| optics.press_element(username) | ||
| # Enter text INTO element | ||
| optics.enter_text_using_keyboard("standard_user") | ||
| optics.sleep("2") | ||
| # Clear input | ||
| optics.clear_element_text(username) | ||
| optics.sleep("2") | ||
| # Verify cleared | ||
| value = optics.get_text(username) | ||
| assert value is None or value == "" | ||
|
|
||
|
|
||
|
|
||
| # --------------------------------------------------------- | ||
| # TEST 4: Login flow (press + enter + keycode) | ||
| # --------------------------------------------------------- | ||
| def test_framework_login_flow(optics): | ||
| optics.press_element('//input[@id="user-name"]') | ||
| optics.enter_text_using_keyboard("standard_user") | ||
| optics.press_element('//input[@id="password"]') | ||
| optics.enter_text_using_keyboard("secret_sauce") | ||
| optics.press_keycode("Enter") | ||
| optics.sleep("2") | ||
| optics.assert_presence('//span[text()="Products"]') | ||
|
|
||
|
|
||
| # --------------------------------------------------------- | ||
| # TEST 5: validate_element + validate_screen | ||
| # --------------------------------------------------------- | ||
| def test_framework_validation_methods(optics): | ||
| optics.press_element('//input[@id="user-name"]') | ||
| optics.enter_text_using_keyboard("standard_user") | ||
| optics.press_element('//input[@id="password"]') | ||
| optics.enter_text_using_keyboard("secret_sauce") | ||
| optics.press_keycode("Enter") | ||
| optics.sleep("2") | ||
| optics.validate_element('//span[text()="Products"]') | ||
| optics.validate_screen( | ||
| [ | ||
| '//div[@class="inventory_list"]', | ||
| '//a[@class="shopping_cart_link"]' | ||
| ] | ||
| ) | ||
| page_source = optics.capture_pagesource() | ||
| assert "inventory_list" in page_source and "shopping_cart_link" in page_source | ||
|
|
||
| # --------------------------------------------------------- | ||
| # TEST 6: get_text | ||
| # --------------------------------------------------------- | ||
| def test_framework_get_text(optics): | ||
| optics.press_element('//input[@id="user-name"]') | ||
| optics.enter_text_using_keyboard("standard_user") | ||
|
|
||
| optics.press_element('//input[@id="password"]') | ||
| optics.enter_text_using_keyboard("secret_sauce") | ||
|
|
||
| optics.press_keycode("Enter") | ||
| optics.sleep("2") | ||
| optics.add_element("page_title", '//span[@class="title"]') | ||
| title = optics.get_text('//span[@class="title"]') | ||
| assert title == "Products" | ||
|
|
||
|
|
||
| # --------------------------------------------------------- | ||
| # TEST 7: press_by_percentage | ||
| # --------------------------------------------------------- | ||
| def test_framework_press_by_percentage(optics): | ||
| # Click somewhere in the viewport (non-destructive) | ||
| optics.press_by_percentage("50", "50") | ||
| optics.sleep("1") | ||
|
|
||
|
|
||
| # --------------------------------------------------------- | ||
| # TEST 8: scroll + scroll_until_element_appears | ||
| # --------------------------------------------------------- | ||
| def test_framework_scroll_methods(optics): | ||
| optics.scroll("down") | ||
| optics.sleep("1") | ||
| optics.scroll_until_element_appears( | ||
| '//button[contains(@id,"add-to-cart")]', | ||
| timeout="10" | ||
| ) | ||
|
|
||
|
|
||
| # --------------------------------------------------------- | ||
| # TEST 9: get_interactive_elements | ||
| # --------------------------------------------------------- | ||
| def test_framework_get_interactive_elements(optics): | ||
| elements = optics.get_interactive_elements(["buttons"]) | ||
| assert isinstance(elements, list) | ||
| assert len(elements) > 0 | ||
|
|
||
|
|
||
| # --------------------------------------------------------- | ||
| # TEST 10: capture screenshot + page source | ||
| # --------------------------------------------------------- | ||
| def test_framework_capture_artifacts(optics): | ||
| screenshot = optics.capture_screenshot() | ||
| assert screenshot is not None | ||
|
|
||
| page_source = optics.capture_pagesource() | ||
| assert "<html" in page_source.lower() | ||
|
|
||
|
|
||
| # --------------------------------------------------------- | ||
| # TEST 11: App metadata | ||
| # --------------------------------------------------------- | ||
| def test_framework_get_app_version(optics): | ||
| version = optics.get_app_version() | ||
| # Playwright web apps often return None; just ensure no crash | ||
| assert version is None or isinstance(version, str) |
There was a problem hiding this comment.
All test cases in this file are testing saucedemo.com functionality (username, password, login flow, products page) rather than YouTube functionality. The file name and PR description indicate this should contain YouTube tests, but the implementation tests an entirely different application. These tests should be moved to a separate file or the file should be renamed to match the actual content.
Comment on lines
+394
to
+431
| - Prefer stable attributes (id, data-testid, name). | ||
| - Fall back to structural XPath only when needed. | ||
| - Keep output simple and deterministic. | ||
| """ | ||
| """ | ||
| Build a minimal, Playwright-compatible XPath for a given DOM node. | ||
|
|
||
| Design intent: | ||
| - Provide a *best-effort* XPath that is simple, readable, and fast to resolve. | ||
| - Prefer stable, unique attributes over deep DOM traversal. | ||
| - Act strictly as a fallback mechanism for operations like bounding-box | ||
| calculation where a locator is required but precision is not critical. | ||
|
|
||
| Why this method is intentionally "simple": | ||
| - It is NOT meant to generate a perfectly unique or future-proof XPath. | ||
| - It avoids expensive document-wide uniqueness checks. | ||
| - It prioritizes speed and resilience over absolute accuracy. | ||
| - More advanced XPath generation is handled elsewhere (`get_xpath`). | ||
|
|
||
| Resolution strategy (in order): | ||
| 1. Use `id` attribute if present (most reliable and unique). | ||
| 2. Use `data-testid` if available (common in test-friendly UIs). | ||
| 3. Use `name` attribute when applicable. | ||
| 4. Fall back to a hierarchical tag-based XPath with positional indexes. | ||
|
|
||
| Important behavior notes: | ||
| - Returned XPath may match multiple elements; callers must handle this. | ||
| - The XPath is always absolute (`//` or `/`) for Playwright compatibility. | ||
| - If the node cannot be resolved meaningfully, `None` is returned. | ||
| - This method performs NO validation against the live DOM. | ||
|
|
||
| Args: | ||
| node (etree.Element): | ||
| lxml DOM element for which an XPath is required. | ||
|
|
||
| Returns: | ||
| Optional[str]: | ||
| A simple XPath string or None if it cannot be constructed. |
There was a problem hiding this comment.
The duplicate docstring in _build_simple_xpath creates confusion. Lines 391-432 contain two consecutive docstrings describing the same method, with overlapping content. Only one docstring should be present for a function definition.
Suggested change
| - Prefer stable attributes (id, data-testid, name). | |
| - Fall back to structural XPath only when needed. | |
| - Keep output simple and deterministic. | |
| """ | |
| """ | |
| Build a minimal, Playwright-compatible XPath for a given DOM node. | |
| Design intent: | |
| - Provide a *best-effort* XPath that is simple, readable, and fast to resolve. | |
| - Prefer stable, unique attributes over deep DOM traversal. | |
| - Act strictly as a fallback mechanism for operations like bounding-box | |
| calculation where a locator is required but precision is not critical. | |
| Why this method is intentionally "simple": | |
| - It is NOT meant to generate a perfectly unique or future-proof XPath. | |
| - It avoids expensive document-wide uniqueness checks. | |
| - It prioritizes speed and resilience over absolute accuracy. | |
| - More advanced XPath generation is handled elsewhere (`get_xpath`). | |
| Resolution strategy (in order): | |
| 1. Use `id` attribute if present (most reliable and unique). | |
| 2. Use `data-testid` if available (common in test-friendly UIs). | |
| 3. Use `name` attribute when applicable. | |
| 4. Fall back to a hierarchical tag-based XPath with positional indexes. | |
| Important behavior notes: | |
| - Returned XPath may match multiple elements; callers must handle this. | |
| - The XPath is always absolute (`//` or `/`) for Playwright compatibility. | |
| - If the node cannot be resolved meaningfully, `None` is returned. | |
| - This method performs NO validation against the live DOM. | |
| Args: | |
| node (etree.Element): | |
| lxml DOM element for which an XPath is required. | |
| Returns: | |
| Optional[str]: | |
| A simple XPath string or None if it cannot be constructed. | |
| - Provide a *best-effort* XPath that is simple, readable, and fast to resolve. | |
| - Prefer stable, unique attributes over deep DOM traversal. | |
| - Prefer stable attributes (id, data-testid, name). | |
| - Fall back to structural XPath only when needed. | |
| - Act strictly as a fallback mechanism for operations like bounding-box | |
| calculation where a locator is required but precision is not critical. | |
| - Keep output simple and deterministic. | |
| Why this method is intentionally "simple": | |
| - It is NOT meant to generate a perfectly unique or future-proof XPath. | |
| - It avoids expensive document-wide uniqueness checks. | |
| - It prioritizes speed and resilience over absolute accuracy. | |
| - More advanced XPath generation is handled elsewhere (`get_xpath`). | |
| Resolution strategy (in order): | |
| 1. Use `id` attribute if present (most reliable and unique). | |
| 2. Use `data-testid` if available (common in test-friendly UIs). | |
| 3. Use `name` attribute when applicable. | |
| 4. Fall back to a hierarchical tag-based XPath with positional indexes. | |
| Important behavior notes: | |
| - Returned XPath may match multiple elements; callers must handle this. | |
| - The XPath is always absolute (`//` or `/`) for Playwright compatibility. | |
| - If the node cannot be resolved meaningfully, `None` is returned. | |
| - This method performs NO validation against the live DOM. | |
| Args: | |
| node (etree.Element): | |
| lxml DOM element for which an XPath is required. | |
| Returns: | |
| Optional[str]: | |
| A simple XPath string or None if it cannot be constructed. |
Comment on lines
+974
to
+995
| - If the string contains ONLY double quotes → wrap with single quotes. | ||
| - If the string contains ONLY single quotes → wrap with double quotes. | ||
| - If the string contains BOTH quote types → use XPath `concat()` to | ||
| assemble the literal safely. | ||
|
|
||
| Why `concat()` is required: | ||
| - XPath does not support escaping quotes inside literals. | ||
| - `concat()` allows us to split the string into safe fragments and | ||
| reconstruct it at runtime inside the XPath engine. | ||
|
|
||
| Design considerations: | ||
| - This method is deterministic and side-effect free. | ||
| - It performs no XPath execution—only string transformation. | ||
| - Centralizing this logic avoids subtle XPath bugs scattered across code. | ||
|
|
||
| Args: | ||
| s (str): | ||
| Raw string value that may contain single and/or double quotes. | ||
|
|
||
| Returns: | ||
| str: | ||
| A valid XPath string literal representation of `s`. |
There was a problem hiding this comment.
The docstring for _build_extra_metadata is misplaced. Lines 965-996 contain documentation describing XPath string literal escaping strategy, but they are positioned within the _build_extra_metadata method definition starting at line 963. This creates confusion about what the method actually does versus what its documentation describes.
Suggested change
| - If the string contains ONLY double quotes → wrap with single quotes. | |
| - If the string contains ONLY single quotes → wrap with double quotes. | |
| - If the string contains BOTH quote types → use XPath `concat()` to | |
| assemble the literal safely. | |
| Why `concat()` is required: | |
| - XPath does not support escaping quotes inside literals. | |
| - `concat()` allows us to split the string into safe fragments and | |
| reconstruct it at runtime inside the XPath engine. | |
| Design considerations: | |
| - This method is deterministic and side-effect free. | |
| - It performs no XPath execution—only string transformation. | |
| - Centralizing this logic avoids subtle XPath bugs scattered across code. | |
| Args: | |
| s (str): | |
| Raw string value that may contain single and/or double quotes. | |
| Returns: | |
| str: | |
| A valid XPath string literal representation of `s`. | |
| """Build a dictionary of extra metadata for an element. | |
| This helper takes the raw attribute mapping for an element and: | |
| - Filters out the attribute identified by ``used_key``. | |
| - Drops attributes with falsey values, and string values equal to | |
| ``"false"`` (case-insensitive). | |
| - Retains all other attributes as-is. | |
| - Explicitly adds commonly used fields such as ``tag``, ``class``, | |
| ``id``, ``role``, ``type`` and ``href`` to the metadata. | |
| The resulting mapping is used to provide richer context about an | |
| element without changing or interpreting the underlying attributes. | |
| Args: | |
| tag (str): The HTML tag name of the element. | |
| attrs (Dict[str, Any]): Raw attribute dictionary for the element. | |
| used_key (str): Attribute name that has already been consumed | |
| by the primary locator logic and should be omitted here. | |
| Returns: | |
| Dict[str, Any]: A dictionary of additional metadata attributes. |
Comment on lines
+989
to
+996
| Args: | ||
| s (str): | ||
| Raw string value that may contain single and/or double quotes. | ||
|
|
||
| Returns: | ||
| str: | ||
| A valid XPath string literal representation of `s`. | ||
| """ |
There was a problem hiding this comment.
The docstring for _escape_for_xpath_literal is misplaced. Lines 918-996 contain documentation describing the _build_hierarchical_xpath method, but they are positioned within the _escape_for_xpath_literal method definition. This makes the code confusing and the docstring content doesn't match the actual function implementation.
Suggested change
| Args: | |
| s (str): | |
| Raw string value that may contain single and/or double quotes. | |
| Returns: | |
| str: | |
| A valid XPath string literal representation of `s`. | |
| """ |
|
|
||
| try: | ||
| asyncio.get_running_loop() | ||
| except RuntimeError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Suggested change
| except RuntimeError: | |
| except RuntimeError: | |
| # It's safe to ignore this: a RuntimeError here just means there is no | |
| # currently running event loop in this thread. We always use our own | |
| # persistent background loop below, so this probe is purely informational. |
| try: | ||
| if condition_fn(): | ||
| return True | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Suggested change
| except Exception: | |
| except Exception: | |
| # Intentionally ignore transient exceptions from condition_fn (e.g. DOM changes) |
|
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
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.
Added Playwright-based YouTube UI tests
Introduced shared pytest fixture for Optics setup and teardown
Split YouTube flow into independent test cases, including:
Launching YouTube
Verifying search box visibility
Clicking the search box
Entering search text
Submitting search using keyboard (Enter)
Handling wait/sleep for stability