-
Notifications
You must be signed in to change notification settings - Fork 1
test: Run Guidepup tests in CI #40
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
Conversation
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.
Pull request overview
This PR enables Guidepup tests to run successfully in CI by fixing path resolution issues, adding timeouts for test stability, and configuring separate CI jobs for macOS and Windows platforms.
Changes:
- Fixed file path resolution in test fixtures to correctly serve static files
- Added explicit waits and focus actions to improve test reliability with screen readers
- Switched Playwright configuration to use Firefox for testing the ariaNotify polyfill
- Split CI workflow into three separate jobs: web-test-runner, guidepup-macos, and guidepup-windows
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/guidepup/voiceover.spec.mjs | Fixed path resolution, removed redundant navigation commands, added waits for stability, commented out failing assertion |
| tests/guidepup/nvda.spec.mjs | Fixed path resolution and added waits for test stability |
| playwright.config.mjs | Changed browser from Edge to Firefox to test the polyfill |
| package.json | Added Firefox installation to test:guidepup script |
| .github/workflows/test.yml | Split test job into three separate jobs with platform-specific Guidepup setup |
Comments suppressed due to low confidence (2)
tests/guidepup/voiceover.spec.mjs:63
- Using hardcoded timeouts with waitForTimeout is an anti-pattern in Playwright tests as it makes tests flaky and slower. Consider using waitFor conditions or polling assertions instead, such as waiting for specific elements or states.
await page.waitForTimeout(500);
tests/guidepup/nvda.spec.mjs:92
- Using hardcoded timeouts with waitForTimeout is an anti-pattern in Playwright tests as it makes tests flaky and slower. Consider using waitFor conditions or polling assertions instead, such as waiting for specific elements or states.
await page.waitForTimeout(500);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lindseywild
left a comment
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.
Cool! Thanks!
This PR updates Guidepup tests so they run successfully, and runs them in CI. This can catch regressions that may occur as we develop the polyfill, increasing confidence in changes.
I updated the branch policy’s required checks accordingly.