Add test framework#959
Conversation
Deploying webgal-dev with
|
| Latest commit: |
4dac148
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e8461b47.webgal-dev.pages.dev |
| Branch Preview URL: | https://add-test-framework.webgal-dev.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive integration testing framework for the WebGAL engine using Playwright and Vitest, including a new webgal-test package, a custom static server, and updated project dependencies. The test suite covers core functionalities such as save/load consistency, backlog management, and complex animations. Feedback focuses on improving framework reliability and efficiency, specifically by suggesting safer HTTP header handling in the test server, optimized build verification logic, the use of state-based waits instead of fixed delays, and the reduction of non-determinism in test logic.
|
|
||
| function createStaticServer(root: string): http.Server { | ||
| const normalizedRoot = path.resolve(root); | ||
| return http.createServer((req, res) => { |
There was a problem hiding this comment.
The host header is not guaranteed to be present in all HTTP requests. If it is missing, the new URL() constructor will throw an error. It is safer to provide a fallback value like localhost for this internal test server.
const url = new URL(req.url || '/', \| function hasTestExposureInBundle(distDir: string): boolean { | ||
| const bundleFiles = listFiles(distDir).filter((file) => ['.js', '.mjs', '.html'].includes(extname(file))); | ||
| return bundleFiles.some((file) => { | ||
| const content = readFileSync(file, 'utf8'); | ||
| return content.includes('webgalTest') && content.includes('WebGAL Test Mode Active'); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The hasTestExposureInBundle function recursively lists and reads every JS/HTML file in the dist directory to verify the build type. This can be very inefficient for large projects with many assets. Consider optimizing this by checking only the main entry point or looking for a specific marker file that indicates a test build.
| await delay(10000); | ||
|
|
There was a problem hiding this comment.
Using a fixed 10-second delay slows down the test suite and can be unreliable depending on the environment. It is better to use waitForSentenceAdvance() to wait for a specific state change, which makes the test both faster and more robust.
| await delay(10000); | |
| // Wait for the sentence ID to advance instead of a fixed long delay\n await waitForSentenceAdvance(page, initialId); |
| function randomDelay(min: number, max: number): number { | ||
| return Math.floor(Math.random() * (max - min + 1)) + min; | ||
| } |
There was a problem hiding this comment.
The use of Math.random() in tests introduces non-determinism, which can lead to flaky tests that are difficult to reproduce or debug. Consider using fixed values or a pseudo-random generator with a constant seed to ensure test runs are reproducible.
1 participant
Close #928