[Task] Replace blind 5s sleep delays in triggers-end-to-end tests with active condition polling - #11113
[Task] Replace blind 5s sleep delays in triggers-end-to-end tests with active condition polling#11113joehan wants to merge 1 commit into
Conversation
… active condition polling Replace hardcoded setTimeout delays in scripts/triggers-end-to-end-tests with active condition polling via a new waitForCondition utility in scripts/integration-helpers/framework.ts. Bug: b/563476178
There was a problem hiding this comment.
Code Review
This pull request replaces hardcoded sleep delays in integration tests with a new polling utility, waitForCondition, which supports both Promise-based and legacy callback-based APIs, and adds corresponding unit tests. The review feedback suggests improving error handling in the callback wrapper to prevent unhandled promise rejections, increasing the default timeout to 20 seconds to reduce flakiness in slow CI environments, and consistently using the TriggerEndToEndTest instance method instead of importing the global helper directly.
| if (cb) { | ||
| promise.then( | ||
| () => cb?.(), | ||
| (err: unknown) => cb?.(err instanceof Error ? err : new Error(String(err))), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
If the callback cb throws an error (such as an assertion failure in a test), it will result in an unhandled promise rejection because the promise chain lacks a .catch() block. To ensure that errors thrown inside the callback are correctly propagated as uncaught exceptions (which test runners like Mocha can catch and report), we should append a .catch() block that re-throws the error asynchronously using setTimeout.
| if (cb) { | |
| promise.then( | |
| () => cb?.(), | |
| (err: unknown) => cb?.(err instanceof Error ? err : new Error(String(err))), | |
| ); | |
| return; | |
| } | |
| if (cb) { | |
| promise.then( | |
| () => cb?.(), | |
| (err: unknown) => cb?.(err instanceof Error ? err : new Error(String(err))), | |
| ).catch((err) => { | |
| setTimeout(() => { | |
| throw err; | |
| }, 0); | |
| }); | |
| return; | |
| } |
References
- Ensure that asynchronous errors and callback exceptions are safely handled and propagated to prevent unhandled promise rejections.
| export async function waitForCondition( | ||
| predicate: () => boolean | Promise<boolean>, | ||
| timeoutMs = 10000, | ||
| intervalMs = 50, | ||
| ): Promise<void> { |
There was a problem hiding this comment.
The default timeout of 10000 ms (10 seconds) might be too short for slow CI environments (such as Windows runners), where emulator triggers can take longer to fire. Since waitForCondition uses active polling and resolves immediately when the condition is met, increasing the default timeout to 20000 or 30000 ms will significantly reduce flakiness without any performance penalty on fast environments.
| export async function waitForCondition( | |
| predicate: () => boolean | Promise<boolean>, | |
| timeoutMs = 10000, | |
| intervalMs = 50, | |
| ): Promise<void> { | |
| export async function waitForCondition( | |
| predicate: () => boolean | Promise<boolean>, | |
| timeoutMs = 20000, | |
| intervalMs = 50, | |
| ): Promise<void> { |
References
- Use generous timeouts for active polling in integration tests to prevent flakiness in slow CI environments.
| waitForCondition( | ||
| conditionFn: () => boolean | Promise<boolean>, | ||
| timeoutMs = 10000, | ||
| intervalMsOrCallback: number | ((err?: Error) => void) = 50, | ||
| callback?: (err?: Error) => void, | ||
| ): Promise<void> | void { |
There was a problem hiding this comment.
To be consistent with the global waitForCondition helper and to prevent flakiness on slow CI environments, we should increase the default timeoutMs to 20000 ms here as well.
| waitForCondition( | |
| conditionFn: () => boolean | Promise<boolean>, | |
| timeoutMs = 10000, | |
| intervalMsOrCallback: number | ((err?: Error) => void) = 50, | |
| callback?: (err?: Error) => void, | |
| ): Promise<void> | void { | |
| waitForCondition( | |
| conditionFn: () => boolean | Promise<boolean>, | |
| timeoutMs = 20000, | |
| intervalMsOrCallback: number | ((err?: Error) => void) = 50, | |
| callback?: (err?: Error) => void, | |
| ): Promise<void> | void { |
References
- Maintain consistent default timeout values across overloaded methods and helper functions.
| const response = await test.writeToAuth(); | ||
| expect(response.status).to.equal(200); | ||
| await new Promise((resolve) => setTimeout(resolve, EMULATORS_WRITE_DELAY_MS)); | ||
| await waitForCondition(() => test.authTriggerCount >= 1); |
There was a problem hiding this comment.
Instead of importing and calling the global waitForCondition helper directly, we can use the instance method test.waitForCondition(...). This keeps the imports cleaner and uses the class's API consistently.
await test.waitForCondition(() => test.authTriggerCount >= 1);References
- Prefer using instance methods over global helpers when they are already available on the test class instance.
| await waitForCondition( | ||
| () => | ||
| test.rtdbTriggerCount >= 1 && | ||
| test.rtdbV2TriggerCount >= 1 && | ||
| test.firestoreTriggerCount >= 1 && | ||
| test.firestoreV2TriggerCount >= 1 && | ||
| test.success(), | ||
| ); |
There was a problem hiding this comment.
Instead of importing and calling the global waitForCondition helper directly, we can use the instance method test.waitForCondition(...). This keeps the imports cleaner and uses the class's API consistently.
await test.waitForCondition(
() =>
test.rtdbTriggerCount >= 1 &&
test.rtdbV2TriggerCount >= 1 &&
test.firestoreTriggerCount >= 1 &&
test.firestoreV2TriggerCount >= 1 &&
test.success(),
);References
- Prefer using instance methods over global helpers when they are already available on the test class instance.
Description
In
scripts/triggers-end-to-end-tests/tests.tsandscripts/triggers-end-to-end-tests/tests.inspect.ts, test steps previously relied on hardcodedsetTimeout(resolve, EMULATORS_WRITE_DELAY_MS)(5–15 seconds each) following database writes, storage mutations, auth actions, and pubsub events. In aggregate across tests, these blind pauses accumulated significant test suite delay and introduced flakiness when triggers took slightly longer or ran concurrently.This PR replaces those blind sleep delays with active condition polling:
waitForConditionUtility:scripts/integration-helpers/framework.ts.TriggerEndToEndTest.prototype.waitForConditionwith an overloaded signature returningPromise<void>while preserving full backward compatibility with the legacy node-style callback signature.scripts/integration-helpers/framework.spec.tscovering immediate resolution, polling resolution, timeout error rejection, predicate error propagation, and legacy callback compatibility.scripts/triggers-end-to-end-tests/tests.ts(Database, Firestore, PubSub, Auth, Storage, background triggers).scripts/triggers-end-to-end-tests/tests.inspect.ts(Auth, Storage).Parent Goal: b/563410897
Fixes: b/563476178
Scenarios Tested
scripts/integration-helpers/framework.spec.ts: 9/9 mocha unit tests passed.npm run build: MCP apps Vite build + TypeScript compilation passed with zero errors.eslint: passed with zero errors on all modified/added files.