-
Notifications
You must be signed in to change notification settings - Fork 1
tests: Both Guidepup tests work! #39
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 fixes the Guidepup tests for both VoiceOver (macOS) and NVDA (Windows) screen readers. The changes address path resolution issues, add necessary timing delays for screen reader synchronization, and improve CI/CD configuration.
Changes:
- Fixed file path resolution in test routing by changing from ".." to "../.."
- Added explicit focus and timing delays to ensure screen reader cursors are properly positioned
- Switched from Microsoft Edge to Firefox for consistent polyfill testing across all test suites
- Added Windows-specific Guidepup job to GitHub Actions workflow
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/guidepup/voiceover.spec.mjs | Fixed path resolution and added timing delays for VoiceOver cursor synchronization |
| tests/guidepup/nvda.spec.mjs | Fixed path resolution and added timing delays for NVDA virtual cursor synchronization |
| playwright.config.mjs | Switched from Edge to Firefox to ensure consistent polyfill testing |
| package.json | Added Firefox installation to test:guidepup script |
| .github/workflows/test.yml | Renamed job and added Windows-specific Guidepup testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await page.getByRole("textbox", { name: "Add a comment" }).click(); | ||
| await page.waitForTimeout(500); |
Copilot
AI
Jan 15, 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.
Using hard-coded timeouts with waitForTimeout is an anti-pattern that can lead to flaky tests. Consider using more reliable waiting strategies such as waiting for specific elements, network requests, or state changes. For example, you could wait for the textarea to be ready using waitFor with a specific state or condition.
| await page.getByRole("textbox", { name: "Add a comment" }).click(); | |
| await page.waitForTimeout(500); | |
| const commentTextbox = page.getByRole("textbox", { name: "Add a comment" }); | |
| await commentTextbox.click(); | |
| await expect(commentTextbox).toBeFocused(); |
| await page.getByRole("textbox", { name: "Add a comment" }).click(); | ||
| await page.waitForTimeout(500); |
Copilot
AI
Jan 15, 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.
Using hard-coded timeouts with waitForTimeout is an anti-pattern that can lead to flaky tests. Consider using more reliable waiting strategies such as waiting for specific elements, network requests, or state changes. For example, you could wait for the textarea to be ready using waitFor with a specific state or condition.
| await page.getByRole("textbox", { name: "Add a comment" }).click(); | |
| await page.waitForTimeout(500); | |
| const commentTextbox = page.getByRole("textbox", { name: "Add a comment" }); | |
| await commentTextbox.click(); | |
| await expect(commentTextbox).toBeFocused(); |
No description provided.