🧪 Add Playwright geometric audit for createChoiceCard - #12
hashexplaindata wants to merge 1 commit into
Conversation
Creates a Python Playwright script to verify the DOM rendering and structural integrity of the generated Bento Grid cards without altering the vanilla JS application logic. - Audits Visual Equivalence - Checks Straitjacket Constraint (AI Badge injection geometry) - Simulates User Selection interaction surface Co-authored-by: hashexplaindata <221828969+hashexplaindata@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new automated UI regression test using Playwright and Python. The test addresses a gap in coverage for the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Playwright-based geometric audit test for the createChoiceCard component, which is a great step towards ensuring UI consistency. The test script is well-structured and covers several important aspects, including visual equivalence, the impact of badge injection, and user interaction.
My review includes a few suggestions to enhance the test's robustness and maintainability:
- A critical fix to replace a fixed-duration wait (
wait_for_timeout) with a condition-based wait to prevent test flakiness. - A recommendation to refactor the main test function into smaller, more focused functions for better readability.
- A suggestion to use more idiomatic Playwright selectors to simplify the code.
Overall, this is a valuable addition to the test suite.
|
|
||
| print("Waiting for next trial transition...") | ||
| # Since transition happens after a 200ms timeout | ||
| await page.wait_for_timeout(300) |
There was a problem hiding this comment.
Using page.wait_for_timeout() can introduce flakiness into the test suite, as the underlying operation might take longer than the fixed timeout on a slow or loaded machine. It's a best practice to wait for a specific application state instead. In this case, you can wait for the trial counter element to contain the text for the next trial.
| await page.wait_for_timeout(300) | |
| await page.wait_for_selector("#trial-counter:has-text('2/6')") |
| async def run(): | ||
| async with async_playwright() as p: | ||
| # Launch browser | ||
| browser = await p.chromium.launch(headless=True) | ||
| context = await browser.new_context() | ||
| page = await context.new_page() | ||
|
|
||
| # Listen for console errors to ensure pure JS execution without exceptions | ||
| page.on("console", lambda msg: print(f"Browser Console [{msg.type}]: {msg.text}")) | ||
| page.on("pageerror", lambda err: print(f"Browser Error: {err}")) | ||
|
|
||
| # Construct URL to load locally | ||
| current_dir = os.path.dirname(os.path.abspath(__file__)) | ||
| index_path = os.path.abspath(os.path.join(current_dir, "..", "code", "index.html")) | ||
| file_url = f"file://{index_path}?condition=ai" | ||
|
|
||
| # Intercept network requests to prevent hang on external resources | ||
| async def handle_route(route): | ||
| url = route.request.url | ||
| if url.startswith("file://") or url.startswith("data:"): | ||
| await route.continue_() | ||
| else: | ||
| await route.abort() | ||
|
|
||
| await page.route("**/*", handle_route) | ||
|
|
||
| print(f"Loading {file_url}") | ||
| await page.goto(file_url, wait_until='domcontentloaded') | ||
|
|
||
| # Navigate through the initial screens to reach the trial grid | ||
| print("Navigating to first trial...") | ||
| # Screen 1: Click consent button | ||
| consent_btn = page.locator("#btn-consent") | ||
| await consent_btn.wait_for(state="visible") | ||
| await consent_btn.click() | ||
|
|
||
| # Screen 2: Click familiarity button (e.g., value "3") | ||
| familiarity_btn = page.locator(".btn-familiarity[data-val='3']") | ||
| await familiarity_btn.wait_for(state="visible") | ||
| await familiarity_btn.click() | ||
|
|
||
| # Wait for the trial grid to be populated | ||
| trial_grid = page.locator("#trial-grid") | ||
| await trial_grid.wait_for(state="visible") | ||
|
|
||
| # Verify Choice Cards | ||
| print("Auditing Choice Card Geometry...") | ||
| cards = page.locator(".bento-choice-card") | ||
|
|
||
| # Wait until exactly 2 cards are in the DOM and visible | ||
| await cards.nth(1).wait_for(state="visible") | ||
|
|
||
| count = await cards.count() | ||
| assert count == 2, f"Expected 2 choice cards, found {count}" | ||
|
|
||
| card_l = cards.nth(0) | ||
| card_r = cards.nth(1) | ||
|
|
||
| box_l = await card_l.bounding_box() | ||
| box_r = await card_r.bounding_box() | ||
|
|
||
| print(f"Card L Box: {box_l}") | ||
| print(f"Card R Box: {box_r}") | ||
|
|
||
| # Assert Visual Equivalence using math.isclose to account for pixel fraction rendering differences | ||
| assert math.isclose(box_l['width'], box_r['width'], abs_tol=1.0), f"Card widths are not equal: {box_l['width']} vs {box_r['width']}" | ||
| assert math.isclose(box_l['height'], box_r['height'], abs_tol=1.0), f"Card heights are not equal: {box_l['height']} vs {box_r['height']}" | ||
|
|
||
| # The Straitjacket Constraint | ||
| print("Checking AI Badge injection...") | ||
| badge = page.locator(".ai-recommendation-badge") | ||
| badge_count = await badge.count() | ||
| assert badge_count == 1, f"Expected exactly 1 AI badge, found {badge_count}" | ||
|
|
||
| # Find which card has the badge | ||
| card_l_has_badge = await card_l.locator(".ai-recommendation-badge").count() > 0 | ||
|
|
||
| target_card = card_l if card_l_has_badge else card_r | ||
| control_card = card_r if card_l_has_badge else card_l | ||
|
|
||
| target_box = await target_card.bounding_box() | ||
| control_box = await control_card.bounding_box() | ||
|
|
||
| print(f"Target Card Box (with badge): {target_box}") | ||
| print(f"Control Card Box (without badge): {control_box}") | ||
|
|
||
| assert math.isclose(target_box['width'], control_box['width'], abs_tol=1.0), f"AI Badge altered width: {target_box['width']} vs {control_box['width']}" | ||
| assert math.isclose(target_box['height'], control_box['height'], abs_tol=1.0), f"AI Badge altered height: {target_box['height']} vs {control_box['height']}" | ||
|
|
||
| # Interaction Surface | ||
| print("Simulating User Selection...") | ||
| # Simulate pointerdown since createChoiceCard uses pointerdown instead of click | ||
| await control_card.dispatch_event("pointerdown") | ||
|
|
||
| # Check if the class 'selected' was added | ||
| is_selected = await control_card.evaluate("node => node.classList.contains('selected')") | ||
| assert is_selected, "Card did not receive 'selected' class upon interaction" | ||
|
|
||
| print("Waiting for next trial transition...") | ||
| # Since transition happens after a 200ms timeout | ||
| await page.wait_for_timeout(300) | ||
|
|
||
| # Ensure we advanced to the next trial | ||
| trial_counter = page.locator("#trial-counter") | ||
| counter_text = await trial_counter.inner_text() | ||
| print(f"Current Trial Status: {counter_text}") | ||
| assert "2/6" in counter_text, "Failed to transition to the next trial" | ||
|
|
||
| print("Geometric Audit Passed Successfully!") | ||
|
|
||
| await browser.close() | ||
|
|
There was a problem hiding this comment.
The run function is quite long and handles multiple responsibilities: browser setup, navigation, and several distinct verification steps. To improve readability and maintainability, consider breaking it down into smaller, more focused asynchronous functions. This will make the test flow clearer and the code easier to manage as more tests are added.
For example, you could structure it like this:
async def setup_browser_and_page(p):
# ...
return page
async def navigate_to_trial_grid(page):
# ...
async def verify_card_geometry(page):
# ...
async def verify_straitjacket_constraint(page):
# ...
async def verify_interaction_surface(page):
# ...
async def run():
async with async_playwright() as p:
page = await setup_browser_and_page(p)
try:
await navigate_to_trial_grid(page)
await verify_card_geometry(page)
await verify_straitjacket_constraint(page)
await verify_interaction_surface(page)
print("Geometric Audit Passed Successfully!")
finally:
await page.browser.close()| card_l_has_badge = await card_l.locator(".ai-recommendation-badge").count() > 0 | ||
|
|
||
| target_card = card_l if card_l_has_badge else card_r | ||
| control_card = card_r if card_l_has_badge else card_l |
There was a problem hiding this comment.
The logic to find the card with the AI badge (target_card) and the one without (control_card) can be simplified by using more expressive Playwright locators with the :has() and :not() pseudo-classes. This approach is more declarative and leverages Playwright's selector engine more effectively, resulting in cleaner and more maintainable code.
| card_l_has_badge = await card_l.locator(".ai-recommendation-badge").count() > 0 | |
| target_card = card_l if card_l_has_badge else card_r | |
| control_card = card_r if card_l_has_badge else card_l | |
| target_card = page.locator(".bento-choice-card:has(.ai-recommendation-badge)") | |
| control_card = page.locator(".bento-choice-card:not(:has(.ai-recommendation-badge))") |
There was a problem hiding this comment.
Pull request overview
Adds a Playwright-based geometric audit test for createChoiceCard that runs in headless Chromium, validating card dimension equality, AI badge layout impact, and pointer interaction behavior.
Changes:
- New Python/Playwright test that navigates through consent/familiarity screens to the trial grid, then asserts choice card geometry and interaction behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| assert math.isclose(target_box['width'], control_box['width'], abs_tol=1.0), f"AI Badge altered width: {target_box['width']} vs {control_box['width']}" | ||
| assert math.isclose(target_box['height'], control_box['height'], abs_tol=1.0), f"AI Badge altered height: {target_box['height']} vs {control_box['height']}" | ||
|
|
||
| # Interaction Surface | ||
| print("Simulating User Selection...") | ||
| # Simulate pointerdown since createChoiceCard uses pointerdown instead of click | ||
| await control_card.dispatch_event("pointerdown") |
There was a problem hiding this comment.
The bounding box assertions on lines 87-94 are redundant with lines 72-73. You already fetched box_l and box_r (which are the same elements as target_box and control_box) and asserted their width/height are equal. Re-fetching bounding boxes and re-asserting the same thing adds no value. If the intent is to verify the badge didn't alter dimensions, consider comparing against a known baseline or removing this duplicate check.
| assert math.isclose(target_box['width'], control_box['width'], abs_tol=1.0), f"AI Badge altered width: {target_box['width']} vs {control_box['width']}" | |
| assert math.isclose(target_box['height'], control_box['height'], abs_tol=1.0), f"AI Badge altered height: {target_box['height']} vs {control_box['height']}" | |
| # Interaction Surface | |
| print("Simulating User Selection...") | |
| # Simulate pointerdown since createChoiceCard uses pointerdown instead of click | |
| await control_card.dispatch_event("pointerdown") | |
| # Interaction Surface | |
| print("Simulating User Selection...") | |
| # Simulate pointerdown since createChoiceCard uses pointerdown instead of click | |
| await control_card.dispatch_event("pointerdown") | |
| # Simulate pointerdown since createChoiceCard uses pointerdown instead of click | |
| await control_card.dispatch_event("pointerdown") |
| page.on("console", lambda msg: print(f"Browser Console [{msg.type}]: {msg.text}")) | ||
| page.on("pageerror", lambda err: print(f"Browser Error: {err}")) |
There was a problem hiding this comment.
Console errors and page errors are printed but never cause the test to fail. A JS exception during interaction (which the PR description says should be caught) will be silently logged. Consider collecting these errors into a list and asserting it's empty at the end of the test.
| browser = await p.chromium.launch(headless=True) | ||
| context = await browser.new_context() | ||
| page = await context.new_page() | ||
|
|
||
| # Listen for console errors to ensure pure JS execution without exceptions | ||
| page.on("console", lambda msg: print(f"Browser Console [{msg.type}]: {msg.text}")) | ||
| page.on("pageerror", lambda err: print(f"Browser Error: {err}")) | ||
|
|
||
| # Construct URL to load locally | ||
| current_dir = os.path.dirname(os.path.abspath(__file__)) | ||
| index_path = os.path.abspath(os.path.join(current_dir, "..", "code", "index.html")) | ||
| file_url = f"file://{index_path}?condition=ai" | ||
|
|
||
| # Intercept network requests to prevent hang on external resources | ||
| async def handle_route(route): | ||
| url = route.request.url | ||
| if url.startswith("file://") or url.startswith("data:"): | ||
| await route.continue_() | ||
| else: | ||
| await route.abort() | ||
|
|
||
| await page.route("**/*", handle_route) | ||
|
|
||
| print(f"Loading {file_url}") | ||
| await page.goto(file_url, wait_until='domcontentloaded') | ||
|
|
||
| # Navigate through the initial screens to reach the trial grid | ||
| print("Navigating to first trial...") | ||
| # Screen 1: Click consent button | ||
| consent_btn = page.locator("#btn-consent") | ||
| await consent_btn.wait_for(state="visible") | ||
| await consent_btn.click() | ||
|
|
||
| # Screen 2: Click familiarity button (e.g., value "3") | ||
| familiarity_btn = page.locator(".btn-familiarity[data-val='3']") | ||
| await familiarity_btn.wait_for(state="visible") | ||
| await familiarity_btn.click() | ||
|
|
||
| # Wait for the trial grid to be populated | ||
| trial_grid = page.locator("#trial-grid") | ||
| await trial_grid.wait_for(state="visible") | ||
|
|
||
| # Verify Choice Cards | ||
| print("Auditing Choice Card Geometry...") | ||
| cards = page.locator(".bento-choice-card") | ||
|
|
||
| # Wait until exactly 2 cards are in the DOM and visible | ||
| await cards.nth(1).wait_for(state="visible") | ||
|
|
||
| count = await cards.count() | ||
| assert count == 2, f"Expected 2 choice cards, found {count}" | ||
|
|
||
| card_l = cards.nth(0) | ||
| card_r = cards.nth(1) | ||
|
|
||
| box_l = await card_l.bounding_box() | ||
| box_r = await card_r.bounding_box() | ||
|
|
||
| print(f"Card L Box: {box_l}") | ||
| print(f"Card R Box: {box_r}") | ||
|
|
||
| # Assert Visual Equivalence using math.isclose to account for pixel fraction rendering differences | ||
| assert math.isclose(box_l['width'], box_r['width'], abs_tol=1.0), f"Card widths are not equal: {box_l['width']} vs {box_r['width']}" | ||
| assert math.isclose(box_l['height'], box_r['height'], abs_tol=1.0), f"Card heights are not equal: {box_l['height']} vs {box_r['height']}" | ||
|
|
||
| # The Straitjacket Constraint | ||
| print("Checking AI Badge injection...") | ||
| badge = page.locator(".ai-recommendation-badge") | ||
| badge_count = await badge.count() | ||
| assert badge_count == 1, f"Expected exactly 1 AI badge, found {badge_count}" | ||
|
|
||
| # Find which card has the badge | ||
| card_l_has_badge = await card_l.locator(".ai-recommendation-badge").count() > 0 | ||
|
|
||
| target_card = card_l if card_l_has_badge else card_r | ||
| control_card = card_r if card_l_has_badge else card_l | ||
|
|
||
| target_box = await target_card.bounding_box() | ||
| control_box = await control_card.bounding_box() | ||
|
|
||
| print(f"Target Card Box (with badge): {target_box}") | ||
| print(f"Control Card Box (without badge): {control_box}") | ||
|
|
||
| assert math.isclose(target_box['width'], control_box['width'], abs_tol=1.0), f"AI Badge altered width: {target_box['width']} vs {control_box['width']}" | ||
| assert math.isclose(target_box['height'], control_box['height'], abs_tol=1.0), f"AI Badge altered height: {target_box['height']} vs {control_box['height']}" | ||
|
|
||
| # Interaction Surface | ||
| print("Simulating User Selection...") | ||
| # Simulate pointerdown since createChoiceCard uses pointerdown instead of click | ||
| await control_card.dispatch_event("pointerdown") | ||
|
|
||
| # Check if the class 'selected' was added | ||
| is_selected = await control_card.evaluate("node => node.classList.contains('selected')") | ||
| assert is_selected, "Card did not receive 'selected' class upon interaction" | ||
|
|
||
| print("Waiting for next trial transition...") | ||
| # Since transition happens after a 200ms timeout | ||
| await page.wait_for_timeout(300) | ||
|
|
||
| # Ensure we advanced to the next trial | ||
| trial_counter = page.locator("#trial-counter") | ||
| counter_text = await trial_counter.inner_text() | ||
| print(f"Current Trial Status: {counter_text}") | ||
| assert "2/6" in counter_text, "Failed to transition to the next trial" | ||
|
|
||
| print("Geometric Audit Passed Successfully!") | ||
|
|
||
| await browser.close() |
There was a problem hiding this comment.
If an assertion fails, browser.close() on line 117 is never called, leaking the browser process. Wrap the test body in a try/finally block to ensure the browser is always closed, or use the browser as an async context manager.
|
Closing: Fixes have already been incorporated directly into the master branch. |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
🎯 What: The testing gap addressed
Missing tests for
createChoiceCardincode/experiment.js.📊 Coverage: What scenarios are now tested
.ai-recommendation-badgenode, asserting its injection did not expand or warp the parent card's dimensions compared to the control card.✨ Result: The improvement in test coverage
We now have an automated UI regression test written in Python and Playwright (under
/telemetry_verification/verify_card_geometry.py) that executes in a real browser (Chromium) to mathematically assert the layout rendering of the choice cards, validating their geometric bounds effectively without mutating our vanilla JS implementation to accommodate testing environments like JSDOM.PR created automatically by Jules for task 8839351114638706877 started by @hashexplaindata