Fix flaky integration tests by properly awaiting Vite dev server cleanup #346
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The integration tests were flaky due to improper async cleanup: 1. The onClose hook in development.js called devServer.close() without awaiting it, causing the Vite dev server to continue cleanup in the background while the next test started. This created race conditions on CI runners with limited resources. 2. test-factories.mjs had a 3-second setTimeout as a workaround, but this was unreliable on slower CI runners and added unnecessary delay. Changes: - Make onClose hook async and await devServer.close() for proper cleanup - Remove arbitrary setTimeout workaround from makeIndexTest This ensures Vite dev servers fully shut down before tests complete, eliminating resource conflicts between consecutive test runs.
|
The module runners created by createServerModuleRunner() also need explicit cleanup. Each runner maintains HMR listeners and module caches that must be closed to prevent resource leaks between test runs. According to Vite's ModuleRunner API documentation, the close() method: - Clears all caches including HMR listeners - Removes all HMR listeners - Resets sourcemap support This ensures complete cleanup of all Vite dev resources before proceeding to the next test.
The loadEntries() function is called on every request in development mode
to enable HMR. However, it was resetting this.runners = {} without closing
the old runners first, causing them to accumulate in memory with:
- Active file watchers
- HMR connection listeners
- Module caches
- Sourcemap support state
This resource leak caused flaky tests as runners accumulated across
requests, especially when running many tests sequentially on CI.
Now properly closes old runners before creating new ones, ensuring
clean resource management on every request.
Previous approach closed ALL runners on every loadEntries() call, which happens on every request for HMR. This caused race conditions: 1. Request 1 arrives -> closes all runners, creates new ones 2. Request 2 arrives (before cleanup completes) -> closes all runners again 3. server.close() -> onClose tries to close already-closing runners This overlap caused unpredictable behavior on CI runners. New approach: Only close a specific runner when we're replacing it for the same environment. This prevents: - Race conditions between concurrent requests - Conflicts between loadEntries and onClose cleanup - Attempting to close already-closed runners The onClose hook still serves as final cleanup for any remaining runners.
loadEntries() is called: 1. At startup (initial setup) 2. On every HTTP request (for HMR in development) This created a race condition where multiple concurrent requests could trigger overlapping loadEntries() calls, causing: - Runners being closed while still being created - onClose hook conflicting with in-progress loadEntries() - Unpredictable state when server.close() is called during a request The mutex ensures only one loadEntries() execution at a time: - Subsequent requests skip if already loading - Prevents concurrent runner close/create operations - Eliminates conflicts between onRequest and onClose hooks This is the final piece for reliable test execution on CI.
The loadBundle() function had a bug where if neither .js nor .mjs bundle files existed, it would still attempt to import the last checked path, causing a confusing ERR_MODULE_NOT_FOUND error. This could happen due to: - Filesystem race conditions on CI (build completes but files not fully flushed to disk before next test runs) - Build failures that weren't caught - Incorrect build output paths Now explicitly checks if a bundle file was found and throws a clear error message listing all attempted paths if not, making it easier to diagnose the root cause. This addresses intermittent test failures where the production test runs before build outputs are fully available on disk.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The integration tests were flaky due to improper async cleanup:
The onClose hook in development.js called devServer.close() without awaiting it, causing the Vite dev server to continue cleanup in the background while the next test started. This created race conditions on CI runners with limited resources.
test-factories.mjs had a 3-second setTimeout as a workaround, but this was unreliable on slower CI runners and added unnecessary delay.
Changes:
This ensures Vite dev servers fully shut down before tests complete, eliminating resource conflicts between consecutive test runs.
Checklist
npm run test && npm run benchmark --if-presentand the Code of conduct