-
Notifications
You must be signed in to change notification settings - Fork 603
Add Playwright browser testing infrastructure #490
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
Set up Playwright for browser-based WebSocket testing with: - Express-based test server with echo protocol support - Interactive HTML test page with WebSocket client UI - 12 comprehensive browser tests for real WebSocket connections - Support for Chromium, Firefox, and WebKit browsers - Auto-start/stop test server via webServer configuration 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary of ChangesHello @theturtle32, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's testing capabilities by introducing a robust browser testing infrastructure using Playwright. This new setup allows for real-world validation of WebSocket connections directly within various browser environments, ensuring greater reliability and compatibility. It includes a dedicated test server and an interactive client UI, providing a comprehensive framework for verifying WebSocket functionality end-to-end. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a comprehensive browser testing setup using Playwright, which is a great addition for ensuring the library works correctly in real browser environments. The implementation is solid, with a well-structured test server, an interactive test page, and a good suite of initial tests. I've identified a minor bug in the test page's logging logic and have a few suggestions to improve the test configuration and test code robustness, mainly around debugging and avoiding potential flakiness.
test/browser/index.html
Outdated
| if (typeof event.data === 'string') { | ||
| addLog(`← Received: ${event.data}`, 'received'); | ||
| } else { | ||
| addLog(`← Received binary data: ${event.data.size} bytes`, 'received'); |
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.
There's a bug here when logging received binary data. When binaryType is 'arraybuffer' (which is the case here), event.data is an ArrayBuffer instance. ArrayBuffer objects do not have a .size property; you should use .byteLength to get the size in bytes.
| addLog(`← Received binary data: ${event.data.size} bytes`, 'received'); | |
| addLog(`← Received binary data: ${event.data.byteLength} bytes`, 'received'); |
| completed. | ||
| completed. | ||
| - When it would be helpful to reference the latest documentation, use the context7 mcp tools | ||
| - If needed, you have the ability to run commands with `sudo` without requiring a password. |
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 instruction about having passwordless sudo capability is a significant security consideration. While this might be for a controlled development environment, it's a practice that can lead to unintended security vulnerabilities if replicated elsewhere. It would be beneficial to add a strong warning here about the risks, or clarify the specific context where this is considered safe.
playwright.config.js
Outdated
| command: 'node test/browser/server.js', | ||
| url: 'http://localhost:8080', | ||
| reuseExistingServer: !process.env.CI, | ||
| stdout: 'ignore', |
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.
Ignoring the stdout of the web server can make debugging test failures very difficult, as any information logged to standard output by the server will be lost. It's better to pipe it to the test runner's output so it's available for inspection when needed.
| stdout: 'ignore', | |
| stdout: 'pipe', |
| await page.fill('#messageInput', msg); | ||
| await page.click('#sendBtn'); | ||
| // Small delay between messages | ||
| await page.waitForTimeout(100); |
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 page.waitForTimeout() is generally discouraged in Playwright tests as it can introduce flakiness. The test might pass locally but fail in a different environment (like CI) where performance characteristics vary. The underlying WebSocket implementation should be able to handle messages sent in quick succession without an artificial delay. I'd recommend removing this line. If a delay is absolutely necessary to reproduce a specific condition, it should be accompanied by a comment explaining why.
- Fix binary data logging bug: Use byteLength for ArrayBuffer - Enable server stdout piping for better test debugging - Remove waitForTimeout to avoid test flakiness 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary
This PR adds comprehensive browser testing infrastructure using Playwright to test real WebSocket connections in actual browser environments.
Key additions:
test/browser/server.js) with:test/browser/index.html) with:window.testAPIfor PlaywrightConfiguration updates:
vitest.config.mjsto exclude browser testswebServerconfiguration to auto-start/stop test servertest:browser,test:browser:chromium,test:browser:uiCLAUDE.mdwith browser testing documentationTest plan
pnpm lint)Run tests:
🤖 Generated with Claude Code