Conversation
…active port polling - Implement waitForPortClosed(port, host, timeoutMs) using tcp-port-used in scripts/emulator-import-export-tests/tests.ts. - Replace 10 blind 2-second sleep delays across import/export tests with active port release polling for hub and target emulator ports. - Fixes race conditions on socket release and eliminates 20 seconds of hardcoded idle delay per test run.
There was a problem hiding this comment.
Code Review
This pull request improves the reliability of the emulator import/export integration tests by replacing arbitrary 2-second delays with a helper function, waitForPortClosed, which polls until the required emulator ports are released. The reviewer feedback correctly points out a violation of the repository's style guide regarding strict null checks, recommending that the emulator configuration be validated once at the top level to avoid using the non-null assertion operator (!) repeatedly across the test cases.
| return JSON.parse(data); | ||
| } | ||
|
|
||
| const CONFIG = readConfig(); |
There was a problem hiding this comment.
According to the repository style guide, we should use strict null checks and handle undefined/null explicitly instead of using non-null assertions (!). Since CONFIG.emulators is optional, we should validate its presence once at the top level and throw a FirebaseError with a non-zero exit code if it is missing. This allows us to safely access EMULATORS without using the non-null assertion operator (!) in every test case.
const CONFIG = readConfig();
if (!CONFIG.emulators) {
throw new FirebaseError("Emulator configuration is missing in firebase.json", { exit: 1 });
}
const EMULATORS = CONFIG.emulators;| await Promise.all([ | ||
| waitForPortClosed(CONFIG.emulators!.hub.port), | ||
| waitForPortClosed(CONFIG.emulators!.firestore.port), | ||
| ]); |
There was a problem hiding this comment.
Use the validated EMULATORS constant instead of CONFIG.emulators! to adhere to the repository style guide's rule of handling undefined/null explicitly.
await Promise.all([
waitForPortClosed(EMULATORS.hub.port),
waitForPortClosed(EMULATORS.firestore.port),
]);References
- Use strict null checks and handle undefined/null explicitly. (link)
| await Promise.all([ | ||
| waitForPortClosed(CONFIG.emulators!.hub.port), | ||
| waitForPortClosed(CONFIG.emulators!.database.port), | ||
| ]); |
There was a problem hiding this comment.
Use the validated EMULATORS constant instead of CONFIG.emulators! to adhere to the repository style guide's rule of handling undefined/null explicitly.
await Promise.all([
waitForPortClosed(EMULATORS.hub.port),
waitForPortClosed(EMULATORS.database.port),
]);References
- Use strict null checks and handle undefined/null explicitly. (link)
| await Promise.all([ | ||
| waitForPortClosed(CONFIG.emulators!.hub.port), | ||
| waitForPortClosed(CONFIG.emulators!.auth.port), | ||
| ]); |
There was a problem hiding this comment.
Use the validated EMULATORS constant instead of CONFIG.emulators! to adhere to the repository style guide's rule of handling undefined/null explicitly.
await Promise.all([
waitForPortClosed(EMULATORS.hub.port),
waitForPortClosed(EMULATORS.auth.port),
]);References
- Use strict null checks and handle undefined/null explicitly. (link)
| await Promise.all([ | ||
| waitForPortClosed(CONFIG.emulators!.hub.port), | ||
| waitForPortClosed(CONFIG.emulators!.auth.port), | ||
| ]); |
There was a problem hiding this comment.
Use the validated EMULATORS constant instead of CONFIG.emulators! to adhere to the repository style guide's rule of handling undefined/null explicitly.
await Promise.all([
waitForPortClosed(EMULATORS.hub.port),
waitForPortClosed(EMULATORS.auth.port),
]);References
- Use strict null checks and handle undefined/null explicitly. (link)
| await Promise.all([ | ||
| waitForPortClosed(CONFIG.emulators!.hub.port), | ||
| waitForPortClosed(CONFIG.emulators!.auth.port), | ||
| ]); |
There was a problem hiding this comment.
Use the validated EMULATORS constant instead of CONFIG.emulators! to adhere to the repository style guide's rule of handling undefined/null explicitly.
await Promise.all([
waitForPortClosed(EMULATORS.hub.port),
waitForPortClosed(EMULATORS.auth.port),
]);References
- Use strict null checks and handle undefined/null explicitly. (link)
| await Promise.all([ | ||
| waitForPortClosed(CONFIG.emulators!.hub.port), | ||
| waitForPortClosed(CONFIG.emulators!.storage.port), | ||
| ]); |
There was a problem hiding this comment.
Use the validated EMULATORS constant instead of CONFIG.emulators! to adhere to the repository style guide's rule of handling undefined/null explicitly.
await Promise.all([
waitForPortClosed(EMULATORS.hub.port),
waitForPortClosed(EMULATORS.storage.port),
]);References
- Use strict null checks and handle undefined/null explicitly. (link)
| await Promise.all([ | ||
| waitForPortClosed(CONFIG.emulators!.hub.port), | ||
| waitForPortClosed(CONFIG.emulators!.storage.port), | ||
| waitForPortClosed(CONFIG.emulators!.auth.port), | ||
| ]); |
There was a problem hiding this comment.
Use the validated EMULATORS constant instead of CONFIG.emulators! to adhere to the repository style guide's rule of handling undefined/null explicitly.
await Promise.all([
waitForPortClosed(EMULATORS.hub.port),
waitForPortClosed(EMULATORS.storage.port),
waitForPortClosed(EMULATORS.auth.port),
]);References
- Use strict null checks and handle undefined/null explicitly. (link)
| await Promise.all([ | ||
| waitForPortClosed(CONFIG.emulators!.hub.port), | ||
| waitForPortClosed(CONFIG.emulators!.storage.port), | ||
| waitForPortClosed(CONFIG.emulators!.auth.port), | ||
| ]); |
There was a problem hiding this comment.
Use the validated EMULATORS constant instead of CONFIG.emulators! to adhere to the repository style guide's rule of handling undefined/null explicitly.
await Promise.all([
waitForPortClosed(EMULATORS.hub.port),
waitForPortClosed(EMULATORS.storage.port),
waitForPortClosed(EMULATORS.auth.port),
]);References
- Use strict null checks and handle undefined/null explicitly. (link)
| await Promise.all([ | ||
| waitForPortClosed(CONFIG.emulators!.hub.port), | ||
| waitForPortClosed(CONFIG.emulators!.firestore.port), | ||
| ]); |
There was a problem hiding this comment.
Use the validated EMULATORS constant instead of CONFIG.emulators! to adhere to the repository style guide's rule of handling undefined/null explicitly.
await Promise.all([
waitForPortClosed(EMULATORS.hub.port),
waitForPortClosed(EMULATORS.firestore.port),
]);References
- Use strict null checks and handle undefined/null explicitly. (link)
Description
Replaces hardcoded 2-second
setTimeoutsleep delays across the 10 end-to-end import/export test cases inscripts/emulator-import-export-tests/tests.tswith an active port release polling helper (waitForPortClosed).Motivation & Background
[Goal] Improve the speed and reliability of the integration test suite)[Task] Replace arbitrary 2s sleep delays in import-export tests with active port polling)tests.tsusedawait new Promise((resolve) => setTimeout(resolve, 2000));at the start of every test to give previously stopped emulator subprocesses time to release their listening ports.EADDRINUSEcollisions that led maintainers to disabletest:import-exportin Windows CI (.github/workflows/node-test.ymlL266).Changes
waitForPortClosed(port: number, host?: string, timeoutMs: number = 10000): Promise<void>inscripts/emulator-import-export-tests/tests.tsusingtcp-port-used.waitUntilFreeOnHostwith 50ms polling interval and dual-stack localhost/127.0.0.1 verification. Errors are wrapped inFirebaseErrorwith original error details preserved.CLIProcessinstances.Verification
npm run test:compile: Passed with 0 errors.npx eslint scripts/emulator-import-export-tests/tests.ts: Passed with 0 errors.