-
Notifications
You must be signed in to change notification settings - Fork 0
🧪 [testing improvement] Add Playwright end-to-end testing for experiment init #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,82 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import time | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import time |
Copilot
AI
Mar 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
page.goto(file_uri) defaults to waiting for the full load event; with external resources being aborted by the route handler, this can make the navigation timing less predictable and the extra wait_for_load_state('domcontentloaded') is redundant. Consider setting wait_until='domcontentloaded' directly on goto and removing the separate wait, to reduce flakiness and speed up the check.
| page.goto(file_uri) | |
| page.wait_for_load_state("domcontentloaded") | |
| page.goto(file_uri, wait_until="domcontentloaded") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current approach of using evaluate and assert works, but it can be made more robust and readable by using Playwright's expect API. The expect function includes auto-waiting, which helps prevent flaky tests that can result from race conditions.
You would first need to add expect to your imports: from playwright.sync_api import sync_playwright, expect.
Then, you can refactor this entire block of initial state assertions to be more declarative and concise.
| screen1 = page.locator("#screen-1") | |
| screen1_is_active = screen1.evaluate("el => el.classList.contains('active')") | |
| # Note: The code doesn't explicitly add a 'hidden' class, it removes 'active' and sets display:none. | |
| # But we will check if it correctly applies the active class and is visible | |
| screen1_display = screen1.evaluate("el => window.getComputedStyle(el).display") | |
| assert screen1_is_active, "Screen 1 should be active initially." | |
| assert screen1_display != "none", f"Screen 1 display should not be none, got {screen1_display}" | |
| # Ensure screen 2 is hidden initially | |
| screen2 = page.locator("#screen-2") | |
| screen2_is_active = screen2.evaluate("el => el.classList.contains('active')") | |
| screen2_display = screen2.evaluate("el => el.style.display || window.getComputedStyle(el).display") | |
| assert not screen2_is_active, "Screen 2 should not be active initially." | |
| assert screen2_display == "none", f"Screen 2 should be hidden initially, got {screen2_display}" | |
| # 1. Initial State Assertion | |
| screen1 = page.locator("#screen-1") | |
| expect(screen1).to_have_class("active") | |
| expect(screen1).to_be_visible() | |
| # Ensure screen 2 is hidden initially | |
| screen2 = page.locator("#screen-2") | |
| expect(screen2).not_to_have_class("active") | |
| expect(screen2).to_be_hidden() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSS selector .btn-primary#btn-consent is redundant. Since element IDs must be unique in a document, using just the ID selector #btn-consent is sufficient, more efficient, and a common best practice.
| page.locator('.btn-primary#btn-consent').click() | |
| page.locator('#btn-consent').click() |
Copilot
AI
Mar 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says the locator should use .btn-consent, but the actual selector used is .btn-primary#btn-consent and index.html does not define a .btn-consent class. Please either update the selector/comment to match the DOM (e.g., just #btn-consent) or update the markup to include the class so the test aligns with the stated intent.
| # The prompt explicitly asked to use '.btn-consent' for the locator | |
| page.locator('.btn-primary#btn-consent').click() | |
| # Click the consent button by its id selector | |
| page.locator('#btn-consent').click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block can be greatly simplified and made more robust by using the expect API. It will handle waiting for the state changes automatically after the click, replacing the explicit wait_for_function and the series of evaluate/assert calls with more declarative assertions. This assumes expect has been imported as suggested in other feedback.
| # Wait for the setTimeout in showScreen | |
| page.wait_for_function("document.getElementById('screen-2').classList.contains('active')") | |
| screen1_is_active_after = screen1.evaluate("el => el.classList.contains('active')") | |
| screen1_display_after = screen1.evaluate("el => el.style.display") | |
| assert not screen1_is_active_after, "Screen 1 should lose the 'active' class." | |
| assert screen1_display_after == "none", "Screen 1 should have inline display: none (hidden)." | |
| screen2_is_active_after = screen2.evaluate("el => el.classList.contains('active')") | |
| screen2_display_after = screen2.evaluate("el => el.style.display") | |
| assert screen2_is_active_after, "Screen 2 should gain the 'active' class." | |
| assert screen2_display_after == "flex", f"Screen 2 should have display: flex, got {screen2_display_after}" | |
| # Wait for screen changes and verify the new state using auto-waiting assertions | |
| expect(screen2).to_have_class("active") | |
| expect(screen2).to_have_css("display", "flex") | |
| expect(screen1).not_to_have_class("active") | |
| expect(screen1).to_be_hidden() |
Copilot
AI
Mar 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description claims verification of “cryptographic participant ID generation”, but this test only checks that STATE.pid is a non-empty string. Either update the PR description to remove the crypto claim, or strengthen the assertion to verify the expected PID format/length produced by the current implementation (and/or that it uses crypto APIs if that’s the real requirement).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
timemodule is imported but it is not used within the file. Unused imports should be removed to maintain code cleanliness and avoid potential confusion.