Skip to content

[AI Improvement] [Task] Prevent unclosed Admin SDK connections and eliminate 5s sleep in extensions-emulator tests - #11111

Open
joehan wants to merge 2 commits into
mainfrom
ai-improve-563476480-task-prevent-unclosed-admin-sdk-con
Open

joehan wants to merge 2 commits into
mainfrom
ai-improve-563476480-task-prevent-unclosed-admin-sdk-con

Conversation

@joehan

@joehan joehan commented Sep 18, 2026

Copy link
Copy Markdown
Member

Resolves Buganizer b/563476480

Proposed Improvement

  1. Added clean Admin SDK teardown in after() hook (await Promise.all(admin.apps.map((app) => app?.delete()))) in scripts/extensions-emulator-tests/tests.ts to close gRPC channels, active sockets, and event loop handles.
  2. Replaced the 5-second blind sleep with an active polling helper (pollForExtensionOutput) that checks Storage and Firestore emulator outputs every 100ms with a 15-second timeout, allowing the test to pass as soon as the extension finishes writing.

Verification

  • Ran npm run build - passed.
  • Ran npm run lint - passed.
  • Ran npm test -- src/test/emulators/ - all 128 tests passing.
  • Ran static type checks with tsc - clean.

@joehan joehan self-assigned this Sep 18, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 replaces a static write delay in the extensions emulator tests with a polling mechanism ('pollForExtensionOutput') that waits for the resized file and Firestore document, and adds cleanup of Firebase admin apps in the 'after' hook. Feedback suggests flattening the nested 'if' statements inside the polling function's 'try' block to reduce nesting, adhering to the repository's style guide.

Comment thread scripts/extensions-emulator-tests/tests.ts

@joehan joehan left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joehan
joehan requested a review from ajperel September 18, 2026 21:37
@joehan
joehan marked this pull request as ready for review September 18, 2026 21:38

after(async function (this) {
this.timeout(EMULATORS_SHUTDOWN_DELAY_MS);
await Promise.all(admin.apps.map((app) => app?.delete()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: Ensure cleanUpExtensionsCache() and test.stopEmulators() run even if app.delete() rejects

Rationale: If any app.delete() call rejects during teardown (or if before() fails before test is assigned), cleanUpExtensionsCache() and test.stopEmulators() would be skipped, leaving emulator child processes running.

Suggested Fix:

    await Promise.allSettled(admin.apps.map((app) => app?.delete()));
    cleanUpExtensionsCache();
    await test?.stopEmulators();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Updated to Promise.allSettled(admin.apps.map((app) => app?.delete())) and guarded await test?.stopEmulators() so emulator cleanup always runs even if Admin SDK deletion rejects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants