Skip to content

🧪 [testing improvement description] #16

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.

Sign up for GitHub

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

Closed
hashexplaindata wants to merge 1 commit into from
Closed

🧪 [testing improvement description] #16

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions telemetry_verification/verify_navigation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import asyncio
from playwright.async_api import async_playwright
Copy link

@gemini-code-assist gemini-code-assist bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To write more robust, non-flaky tests, it's best to use Playwright's auto-waiting assertions via the expect function. This requires importing it, along with the re module for more precise class name assertions.

Suggested change
from playwright.async_api import async_playwright
from playwright.async_api import async_playwright, expect
import re

import time
Copy link

@gemini-code-assist gemini-code-assist bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The time module is imported but not used. The same applies to os (line 5) and signal (line 6). Please remove these unused imports to improve code clarity and reduce clutter.

import subprocess
import os
import signal
Comment on lines +3 to +6
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imports time, os, and signal are unused in this script. Removing them avoids lint noise and makes dependencies clearer.

Suggested change
import time
import subprocess
import os
import signal
import subprocess

Copilot uses AI. Check for mistakes.

async def main():
# Start the local server
server_process = subprocess.Popen(['python3', '-m', 'http.server', '8000'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
Copy link

@gemini-code-assist gemini-code-assist bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The port 8000 is hardcoded here and also used in the page.goto call on line 27. It's a good practice to define this as a constant at the top of the script (e.g., PORT = 8000). This avoids 'magic numbers' and makes the script easier to configure and read.


# Wait a bit for the server to start
await asyncio.sleep(1)
Comment on lines +12 to +13
Copy link

@gemini-code-assist gemini-code-assist bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using a fixed asyncio.sleep(1) to wait for the server to start can lead to flaky tests, as the server might take more or less time to initialize depending on system load. A more robust approach is to actively poll the server to check if it's ready, for instance by trying to establish a connection in a loop.

    # Wait for the server to be ready by polling
    for _ in range(20):  # Try for 2 seconds
        try:
            _, writer = await asyncio.open_connection('localhost', 8000)
            writer.close()
            await writer.wait_closed()
            break
        except ConnectionRefusedError:
            await asyncio.sleep(0.1)
    else:
        raise RuntimeError("Server did not start in time.")

Comment on lines +9 to +13
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The local server is started with a hard-coded command (python3 -m http.server 8000) and a fixed port. This can fail on Windows/CI (no python3) or when port 8000 is already in use, and the failures are hard to debug because stdout/stderr are discarded. Consider using sys.executable, choosing an ephemeral free port, and serving the repo root explicitly (via cwd or --directory) so the script works no matter where it's launched from.

Copilot uses AI. Check for mistakes.

try:
async with async_playwright() as p:
# Setup browser
browser = await p.chromium.launch(headless=True)
context = await browser.new_context()
page = await context.new_page()

# Intercept and abort external requests to prevent hanging
await page.route("**/*", lambda route: route.abort() if route.request.url.startswith("http") and not route.request.url.startswith("http://localhost") else route.continue_())

Comment on lines +23 to +24
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

page.route handler uses a synchronous lambda that calls route.abort() / route.continue_() without awaiting them. In Playwright's async API these are coroutines; this will raise "coroutine was never awaited" and the routing will not behave reliably. Use an async def route handler and await route.abort() / await route.continue_() (or remove routing if not needed).

Suggested change
await page.route("**/*", lambda route: route.abort() if route.request.url.startswith("http") and not route.request.url.startswith("http://localhost") else route.continue_())
async def handle_route(route):
if route.request.url.startswith("http") and not route.request.url.startswith("http://localhost"):
await route.abort()
else:
await route.continue_()
await page.route("**/*", handle_route)

Copilot uses AI. Check for mistakes.
# Navigate to the experiment
print("Navigating to the experiment...")
await page.goto("http://localhost:8000/code/index.html?condition=control", wait_until="commit")

Comment on lines +27 to +28
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

page.goto(..., wait_until="commit") can return before DOMContentLoaded and before experiment.js attaches the click handler / defines showScreen. That makes the subsequent page.click('#btn-consent') and page.evaluate("showScreen('trial')") race-prone and can fail nondeterministically. Prefer waiting for domcontentloaded (or load) and/or explicitly wait_for_function that window.showScreen exists before interacting.

Suggested change
await page.goto("http://localhost:8000/code/index.html?condition=control", wait_until="commit")
await page.goto("http://localhost:8000/code/index.html?condition=control", wait_until="domcontentloaded")
# Ensure the experiment script has defined showScreen before interacting
await page.wait_for_function("window.showScreen !== undefined")

Copilot uses AI. Check for mistakes.
# Wait for the initial screen to be ready
await page.wait_for_selector('#screen-1', state="attached")

print("Testing initial state...")
# 1. Visibility Assertion: Screen 1 should be visible, others hidden
screen1 = page.locator('#screen-1')
screen2 = page.locator('#screen-2')

# Check classes
screen1_classes = await screen1.get_attribute('class')
assert 'active' in screen1_classes, "Screen 1 should have 'active' class initially"

Comment on lines +38 to +40
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_attribute('class') can return None when the attribute is missing; 'active' in screen1_classes (and similar checks below) will then raise a TypeError. Consider defaulting to an empty string when the attribute is None, or use evaluate/locator.evaluate to check element.classList.contains('active').

Copilot uses AI. Check for mistakes.
# Check actual visibility
assert await screen1.is_visible(), "Screen 1 should be visible initially"
assert not await screen2.is_visible(), "Screen 2 should be hidden initially"
Comment on lines +37 to +43
Copy link

@gemini-code-assist gemini-code-assist bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

These initial state assertions can be converted to use expect() for consistency and robustness. expect() will wait for the elements to be in the asserted state, which is safer than asserting immediately after navigation.

            # Check classes and visibility using auto-waiting assertions
            await expect(screen1).to_have_class(re.compile(r'\bactive\b'))
            await expect(screen1).to_be_visible()

            await expect(screen2).not_to_be_visible()
            await expect(screen2).not_to_have_class(re.compile(r'\bactive\b'))


print("Testing transition to Screen 2...")
# 2. Transition Assertion: Click consent button
await page.click('#btn-consent')

# Wait for the setTimeout in showScreen to complete (50ms)
await asyncio.sleep(0.1)

Comment on lines +49 to +51
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixed asyncio.sleep(0.1) waits for the setTimeout-driven transition are timing-dependent and can be flaky on slower CI machines. It's more reliable to wait on an actual condition (e.g., wait for #screen-2 to become visible / have active, or wait for #screen-1 to become hidden) using wait_for_function or Playwright's locator assertions.

Copilot uses AI. Check for mistakes.
# 3. State Verification: Screen 1 hidden, Screen 2 visible
# Check classes
screen1_classes = await screen1.get_attribute('class')
screen2_classes = await screen2.get_attribute('class')

assert 'active' not in screen1_classes, "Screen 1 should NOT have 'active' class after transition"
assert 'active' in screen2_classes, "Screen 2 should have 'active' class after transition"

# Check actual visibility
assert not await screen1.is_visible(), "Screen 1 should be hidden after transition"
assert await screen2.is_visible(), "Screen 2 should be visible after transition"
Comment on lines +49 to +62
Copy link

@gemini-code-assist gemini-code-assist bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using asyncio.sleep() for waiting on UI updates is a common source of test flakiness. The subsequent manual assertions don't wait, so they might run before the UI has finished updating. By using expect(), Playwright will automatically wait for the condition to be true before proceeding, making the test much more reliable. This suggestion replaces the sleep and the manual assertions.

            # 3. State Verification: Screen 1 hidden, Screen 2 visible
            # Using expect() for robust, auto-waiting assertions.
            await expect(screen1).not_to_have_class(re.compile(r'\bactive\b'))
            await expect(screen2).to_have_class(re.compile(r'\bactive\b'))

            await expect(screen1).not_to_be_visible()
            await expect(screen2).to_be_visible()


print("Testing programmatic transition to Screen 'trial'...")
# 4. Trigger a programmatic transition to 'trial'
await page.evaluate("showScreen('trial')")

# Wait for the setTimeout in showScreen to complete
await asyncio.sleep(0.1)

screen_trial = page.locator('#screen-trial')

# Check classes
screen2_classes = await screen2.get_attribute('class')
screen_trial_classes = await screen_trial.get_attribute('class')

assert 'active' not in screen2_classes, "Screen 2 should NOT have 'active' class after transition to trial"
assert 'active' in screen_trial_classes, "Screen trial should have 'active' class after transition"

# Check actual visibility
assert not await screen2.is_visible(), "Screen 2 should be hidden after transition to trial"
assert await screen_trial.is_visible(), "Screen trial should be visible after transition to trial"
Comment on lines +68 to +82
Copy link

@gemini-code-assist gemini-code-assist bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the previous transition, this asyncio.sleep() and manual assertion block should be replaced with auto-waiting expect() calls to improve test reliability and prevent flakiness.

            screen_trial = page.locator('#screen-trial')

            # Check classes and visibility using auto-waiting assertions
            await expect(screen2).not_to_have_class(re.compile(r'\bactive\b'))
            await expect(screen_trial).to_have_class(re.compile(r'\bactive\b'))

            await expect(screen2).not_to_be_visible()
            await expect(screen_trial).to_be_visible()


print("✅ All navigation and visibility assertions passed successfully.")

await browser.close()

Comment on lines +18 to +87
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any assertion or Playwright call raises inside the async with async_playwright() block, the browser/context may not be closed because await browser.close() is not in a finally. Wrapping browser/context creation in a try/finally (or using async with p.chromium.launch(...) as browser if supported) helps avoid orphaned processes on failure.

Suggested change
browser = await p.chromium.launch(headless=True)
context = await browser.new_context()
page = await context.new_page()
# Intercept and abort external requests to prevent hanging
await page.route("**/*", lambda route: route.abort() if route.request.url.startswith("http") and not route.request.url.startswith("http://localhost") else route.continue_())
# Navigate to the experiment
print("Navigating to the experiment...")
await page.goto("http://localhost:8000/code/index.html?condition=control", wait_until="commit")
# Wait for the initial screen to be ready
await page.wait_for_selector('#screen-1', state="attached")
print("Testing initial state...")
# 1. Visibility Assertion: Screen 1 should be visible, others hidden
screen1 = page.locator('#screen-1')
screen2 = page.locator('#screen-2')
# Check classes
screen1_classes = await screen1.get_attribute('class')
assert 'active' in screen1_classes, "Screen 1 should have 'active' class initially"
# Check actual visibility
assert await screen1.is_visible(), "Screen 1 should be visible initially"
assert not await screen2.is_visible(), "Screen 2 should be hidden initially"
print("Testing transition to Screen 2...")
# 2. Transition Assertion: Click consent button
await page.click('#btn-consent')
# Wait for the setTimeout in showScreen to complete (50ms)
await asyncio.sleep(0.1)
# 3. State Verification: Screen 1 hidden, Screen 2 visible
# Check classes
screen1_classes = await screen1.get_attribute('class')
screen2_classes = await screen2.get_attribute('class')
assert 'active' not in screen1_classes, "Screen 1 should NOT have 'active' class after transition"
assert 'active' in screen2_classes, "Screen 2 should have 'active' class after transition"
# Check actual visibility
assert not await screen1.is_visible(), "Screen 1 should be hidden after transition"
assert await screen2.is_visible(), "Screen 2 should be visible after transition"
print("Testing programmatic transition to Screen 'trial'...")
# 4. Trigger a programmatic transition to 'trial'
await page.evaluate("showScreen('trial')")
# Wait for the setTimeout in showScreen to complete
await asyncio.sleep(0.1)
screen_trial = page.locator('#screen-trial')
# Check classes
screen2_classes = await screen2.get_attribute('class')
screen_trial_classes = await screen_trial.get_attribute('class')
assert 'active' not in screen2_classes, "Screen 2 should NOT have 'active' class after transition to trial"
assert 'active' in screen_trial_classes, "Screen trial should have 'active' class after transition"
# Check actual visibility
assert not await screen2.is_visible(), "Screen 2 should be hidden after transition to trial"
assert await screen_trial.is_visible(), "Screen trial should be visible after transition to trial"
print("✅ All navigation and visibility assertions passed successfully.")
await browser.close()
async with p.chromium.launch(headless=True) as browser:
async with browser.new_context() as context:
page = await context.new_page()
# Intercept and abort external requests to prevent hanging
await page.route("**/*", lambda route: route.abort() if route.request.url.startswith("http") and not route.request.url.startswith("http://localhost") else route.continue_())
# Navigate to the experiment
print("Navigating to the experiment...")
await page.goto("http://localhost:8000/code/index.html?condition=control", wait_until="commit")
# Wait for the initial screen to be ready
await page.wait_for_selector('#screen-1', state="attached")
print("Testing initial state...")
# 1. Visibility Assertion: Screen 1 should be visible, others hidden
screen1 = page.locator('#screen-1')
screen2 = page.locator('#screen-2')
# Check classes
screen1_classes = await screen1.get_attribute('class')
assert 'active' in screen1_classes, "Screen 1 should have 'active' class initially"
# Check actual visibility
assert await screen1.is_visible(), "Screen 1 should be visible initially"
assert not await screen2.is_visible(), "Screen 2 should be hidden initially"
print("Testing transition to Screen 2...")
# 2. Transition Assertion: Click consent button
await page.click('#btn-consent')
# Wait for the setTimeout in showScreen to complete (50ms)
await asyncio.sleep(0.1)
# 3. State Verification: Screen 1 hidden, Screen 2 visible
# Check classes
screen1_classes = await screen1.get_attribute('class')
screen2_classes = await screen2.get_attribute('class')
assert 'active' not in screen1_classes, "Screen 1 should NOT have 'active' class after transition"
assert 'active' in screen2_classes, "Screen 2 should have 'active' class after transition"
# Check actual visibility
assert not await screen1.is_visible(), "Screen 1 should be hidden after transition"
assert await screen2.is_visible(), "Screen 2 should be visible after transition"
print("Testing programmatic transition to Screen 'trial'...")
# 4. Trigger a programmatic transition to 'trial'
await page.evaluate("showScreen('trial')")
# Wait for the setTimeout in showScreen to complete
await asyncio.sleep(0.1)
screen_trial = page.locator('#screen-trial')
# Check classes
screen2_classes = await screen2.get_attribute('class')
screen_trial_classes = await screen_trial.get_attribute('class')
assert 'active' not in screen2_classes, "Screen 2 should NOT have 'active' class after transition to trial"
assert 'active' in screen_trial_classes, "Screen trial should have 'active' class after transition"
# Check actual visibility
assert not await screen2.is_visible(), "Screen 2 should be hidden after transition to trial"
assert await screen_trial.is_visible(), "Screen trial should be visible after transition to trial"
print("✅ All navigation and visibility assertions passed successfully.")

Copilot uses AI. Check for mistakes.
finally:
# Clean up the server process
server_process.terminate()
server_process.wait()

if __name__ == "__main__":
asyncio.run(main())
Loading