-
Notifications
You must be signed in to change notification settings - Fork 0
🧪 Implement Automated Telemetry & Navigation Verification #8
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,147 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from playwright.sync_api import Page, expect, sync_playwright | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import json |
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.
Using pathlib provides a more robust and readable way to construct file paths and convert them to a file URI, avoiding potential issues with string formatting across different operating systems.
| current_dir = os.getcwd() | |
| html_file_path = f"file://{current_dir}/code/index.html" | |
| html_file_path = Path("code/index.html").resolve().as_uri() |
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.
Building the file://.../code/index.html URL from os.getcwd() is brittle (depends on the caller's working directory) and can also break on Windows paths / spaces because it isn't URL-encoded. Prefer constructing the path relative to this file (e.g., via Path(__file__).resolve()) and using Path(...).as_uri() for a correct file URL.
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.
Assert screen deactivation by token, not full class string
not_to_have_class("active") only checks that the full class attribute is not exactly "active", so it still passes when the element remains "screen active". In this test, a regression that fails to deactivate #screen-1 would not be caught, which undermines the navigation verification this script is meant to provide.
Useful? React with 👍 / 👎.
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.
This layout check assumes there are at least two visible .bento-choice-card elements and that bounding_box() returns a dict. In Playwright, bounding_box() can return None (e.g., if the element is not visible yet), and .all() can return fewer than 2 elements if rendering fails. Add an explicit count/visibility assertion before indexing and handle None boxes to avoid intermittent failures.
| cards = page.locator(".bento-choice-card").all() | |
| card_a_box = cards[0].bounding_box() | |
| card_b_box = cards[1].bounding_box() | |
| cards_locator = page.locator(".bento-choice-card") | |
| # Ensure we have at least two visible cards before measuring layout | |
| expect(cards_locator).to_have_count(2) | |
| expect(cards_locator.nth(0)).to_be_visible() | |
| expect(cards_locator.nth(1)).to_be_visible() | |
| cards = cards_locator.all() | |
| card_a_box = cards[0].bounding_box() | |
| card_b_box = cards[1].bounding_box() | |
| if card_a_box is None or card_b_box is None: | |
| raise AssertionError("Unable to measure card layout: one or both card bounding boxes are None.") |
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.
Remove duplicate trial advancement in simulation loop
The loop manually calls loadNextTrial() immediately after handleUserSelection(...), but handleUserSelection already schedules loadNextTrial via setTimeout in code/experiment.js. This introduces overlapping trial transitions and timing races, making the E2E check flaky and potentially validating state/screens at unintended points in the flow.
Useful? React with 👍 / 👎.
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 loop has a couple of issues:
- The number of trials is hardcoded (
6), making the test brittle if the experiment configuration changes. - The logic for advancing trials is incorrect. The comment on line 96 is mistaken;
handleUserSelectiondoes scheduleloadNextTrialafter a delay. Calling it again immediately is a bug, and usingwait_for_timeoutis unreliable.
A better approach is to get the number of trials from the page, and then in the loop, simply trigger the user selection and let Playwright's auto-waiting handle the delay by asserting on an element of the next state.
| for i in range(6): | |
| # Trigger selection directly via JS to avoid race conditions with DOM injection | |
| page.evaluate("""() => { | |
| const trial = TRIALS[STATE.currentTrial]; | |
| handleUserSelection('A', trial); | |
| }""") | |
| # We still need to call loadNextTrial because handleUserSelection only increments state | |
| page.evaluate("loadNextTrial()") | |
| page.wait_for_timeout(100) | |
| num_trials = page.evaluate("() => CFG.NUM_TRIALS") | |
| for i in range(num_trials): | |
| # Trigger selection directly via JS to avoid race conditions with DOM injection | |
| page.evaluate("""() => { | |
| const trial = TRIALS[STATE.currentTrial]; | |
| handleUserSelection('A', trial); | |
| }""") | |
| # After the selection, wait for the UI to update to the next trial. | |
| # This replaces the incorrect manual call to loadNextTrial() and the unreliable wait_for_timeout(). | |
| # On the last trial, the screen will change to 9, which is checked later. | |
| if i < num_trials - 1: | |
| expect(page.locator("#trial-counter")).to_have_text(f"Diagnostic {i + 2}/{num_trials}") |
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 loop calls handleUserSelection(...) (which already schedules loadNextTrial via setTimeout(loadNextTrial, 200)) and then immediately calls loadNextTrial() again. This can cause the next-trial UI to re-render unexpectedly while the loop continues, leading to flaky timing-dependent behavior. Prefer either waiting for the built-in transition (e.g., wait ~250ms and do not call loadNextTrial() manually) or temporarily disabling/overriding the timer when driving the flow via direct JS calls.
| # Trigger selection directly via JS to avoid race conditions with DOM injection | |
| page.evaluate("""() => { | |
| const trial = TRIALS[STATE.currentTrial]; | |
| handleUserSelection('A', trial); | |
| }""") | |
| # We still need to call loadNextTrial because handleUserSelection only increments state | |
| page.evaluate("loadNextTrial()") | |
| page.wait_for_timeout(100) | |
| # Trigger selection directly via JS to avoid race conditions with DOM injection. | |
| # Rely on handleUserSelection's built-in timeout to call loadNextTrial. | |
| page.evaluate("""() => { | |
| const trial = TRIALS[STATE.currentTrial]; | |
| handleUserSelection('A', trial); | |
| }""") | |
| # Wait long enough for the internal loadNextTrial transition to complete | |
| page.wait_for_timeout(250) |
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.
Using wait_for_timeout (a hard sleep) can make tests flaky and slow. It's better to wait for a specific condition or state change in the UI. In this case, you should wait for screen 10 to become active before proceeding to check the results.
| page.wait_for_timeout(500) | |
| expect(page.locator("#screen-10")).to_have_class("screen active") |
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.
This script relies on Python assert statements for validation. When Python is run with optimizations (python -O), assert statements are stripped and the script may report success without executing checks. For a verification tool, prefer explicit if checks that raise AssertionError (or use unittest/pytest) so the validations always run.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,39 @@ | ||||||||||||||||||||
| /** | ||||||||||||||||||||
| * BEHAVIORAL DIAGNOSTIC TOOL — LOGIC VERIFICATION | ||||||||||||||||||||
| * Microscopic, Zero-Dependency Assertion Engine | ||||||||||||||||||||
| */ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const assert = (condition, message) => { | ||||||||||||||||||||
| if (!condition) { | ||||||||||||||||||||
| console.error(`❌ [FAILED] ${message}`); | ||||||||||||||||||||
| process.exit(1); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| console.log(`✅ [PASSED] ${message}`); | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // --- Test Suite for Pure Functions (Non-DOM) --- | ||||||||||||||||||||
|
|
||||||||||||||||||||
| console.log("Running Pure Logic Verifications..."); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Note: In this environment, we load the code via filesystem if possible | ||||||||||||||||||||
| // or define critical logic tests that don't depend on JSDOM. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Example: Mocking the state transition logic for result logging | ||||||||||||||||||||
| const simulateResultLogging = (results, selection, target) => { | ||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This script defines and validates a local mock ( Useful? React with 👍 / 👎. |
||||||||||||||||||||
| results.push({ | ||||||||||||||||||||
| user_selection: selection, | ||||||||||||||||||||
|
Comment on lines
+23
to
+24
|
||||||||||||||||||||
| results.push({ | |
| user_selection: selection, | |
| const formattedSelection = | |
| selection === 'A' ? 'Layout A' : | |
| selection === 'B' ? 'Layout B' : | |
| selection; | |
| results.push({ | |
| user_selection: formattedSelection, |
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 suite will load the real code from the filesystem if possible, but the script currently never imports/loads code/experiment.js (and instead tests a local mock). As written, this won't catch regressions in the actual experiment logic; consider wiring this to the real functions (e.g., by exporting a pure module from experiment.js or duplicating the exact logic under test) or adjust the wording so the scope is clear.
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
osmodule can be replaced with the more modernpathlibfor path operations later in the file. Additionally, thejsonmodule is imported but never used and can be removed.