-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[Task] Replace blind 5s sleep delays in triggers-end-to-end tests with active condition polling #11113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[Task] Replace blind 5s sleep delays in triggers-end-to-end tests with active condition polling #11113
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| import { expect } from "chai"; | ||
| import { waitForCondition, TriggerEndToEndTest } from "./framework"; | ||
|
|
||
| describe("waitForCondition", () => { | ||
| it("should resolve immediately if condition is already true", async () => { | ||
| let callCount = 0; | ||
| const start = Date.now(); | ||
| await waitForCondition( | ||
| () => { | ||
| callCount++; | ||
| return true; | ||
| }, | ||
| 1000, | ||
| 50, | ||
| ); | ||
| const duration = Date.now() - start; | ||
|
|
||
| expect(callCount).to.equal(1); | ||
| expect(duration).to.be.lessThan(100); | ||
| }); | ||
|
|
||
| it("should poll and resolve when condition becomes true", async () => { | ||
| let count = 0; | ||
| setTimeout(() => { | ||
| count = 5; | ||
| }, 60); | ||
|
|
||
| const start = Date.now(); | ||
| await waitForCondition(() => count >= 5, 2000, 20); | ||
| const duration = Date.now() - start; | ||
|
|
||
| expect(count).to.be.at.least(5); | ||
| expect(duration).to.be.at.least(40); | ||
| expect(duration).to.be.lessThan(1500); | ||
| }); | ||
|
|
||
| it("should support async predicates", async () => { | ||
| let asyncFlag = false; | ||
| setTimeout(() => { | ||
| asyncFlag = true; | ||
| }, 50); | ||
|
|
||
| await waitForCondition( | ||
| async () => { | ||
| await new Promise((resolve) => setTimeout(resolve, 5)); | ||
| return asyncFlag; | ||
| }, | ||
| 2000, | ||
| 20, | ||
| ); | ||
|
|
||
| expect(asyncFlag).to.be.true; | ||
| }); | ||
|
|
||
| it("should reject with timeout error if condition never becomes true", async () => { | ||
| let error: Error | undefined; | ||
| try { | ||
| await waitForCondition(() => false, 100, 20); | ||
| } catch (err) { | ||
| error = err as Error; | ||
| } | ||
|
|
||
| expect(error).to.exist; | ||
| expect(error?.message).to.include("Timed out waiting for condition after 100ms"); | ||
| }); | ||
|
|
||
| it("should reject if predicate throws an error", async () => { | ||
| let error: Error | undefined; | ||
| try { | ||
| await waitForCondition( | ||
| () => { | ||
| throw new Error("Predicate failure"); | ||
| }, | ||
| 500, | ||
| 20, | ||
| ); | ||
| } catch (err) { | ||
| error = err as Error; | ||
| } | ||
|
|
||
| expect(error).to.exist; | ||
| expect(error?.message).to.equal("Predicate failure"); | ||
| }); | ||
|
|
||
| describe("TriggerEndToEndTest instance method", () => { | ||
| let test: TriggerEndToEndTest; | ||
|
|
||
| beforeEach(() => { | ||
| test = new TriggerEndToEndTest("test-project", "/tmp", {}); | ||
| }); | ||
|
|
||
| it("should return a Promise that resolves when condition is true", async () => { | ||
| let count = 0; | ||
| setTimeout(() => { | ||
| count = 3; | ||
| }, 50); | ||
|
|
||
| await test.waitForCondition(() => count >= 3, 1000, 20); | ||
| expect(count).to.equal(3); | ||
| }); | ||
|
|
||
| it("should reject the Promise on timeout", async () => { | ||
| let error: Error | undefined; | ||
| try { | ||
| await test.waitForCondition(() => false, 100, 20); | ||
| } catch (err) { | ||
| error = err as Error; | ||
| } | ||
|
|
||
| expect(error).to.exist; | ||
| expect(error?.message).to.include("Timed out waiting for condition after 100ms"); | ||
| }); | ||
|
|
||
| it("should support legacy callback on success", (done) => { | ||
| let count = 0; | ||
| setTimeout(() => { | ||
| count = 1; | ||
| }, 30); | ||
|
|
||
| test.waitForCondition( | ||
| () => count === 1, | ||
| 1000, | ||
| (err) => { | ||
| try { | ||
| expect(err).to.be.undefined; | ||
| expect(count).to.equal(1); | ||
| done(); | ||
| } catch (assertErr) { | ||
| done(assertErr); | ||
| } | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| it("should support legacy callback on timeout", (done) => { | ||
| test.waitForCondition( | ||
| () => false, | ||
| 100, | ||
| (err) => { | ||
| try { | ||
| expect(err).to.exist; | ||
| expect(err?.message).to.include("Timed out waiting for condition"); | ||
| done(); | ||
| } catch (assertErr) { | ||
| done(assertErr); | ||
| } | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -204,19 +204,19 @@ | |||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| this.cliProcess?.process?.stdout?.on("data", (data) => { | ||||||||||||||||||||||||||||||||||||||
| /* Functions V1 */ | ||||||||||||||||||||||||||||||||||||||
| if (data.includes(RTDB_FUNCTION_LOG)) { | ||||||||||||||||||||||||||||||||||||||
|
Check warning on line 207 in scripts/integration-helpers/framework.ts
|
||||||||||||||||||||||||||||||||||||||
| this.rtdbTriggerCount++; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if (data.includes(FIRESTORE_FUNCTION_LOG)) { | ||||||||||||||||||||||||||||||||||||||
|
Check warning on line 210 in scripts/integration-helpers/framework.ts
|
||||||||||||||||||||||||||||||||||||||
| this.firestoreTriggerCount++; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if (data.includes(PUBSUB_FUNCTION_LOG)) { | ||||||||||||||||||||||||||||||||||||||
|
Check warning on line 213 in scripts/integration-helpers/framework.ts
|
||||||||||||||||||||||||||||||||||||||
| this.pubsubTriggerCount++; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if (data.includes(AUTH_FUNCTION_LOG)) { | ||||||||||||||||||||||||||||||||||||||
|
Check warning on line 216 in scripts/integration-helpers/framework.ts
|
||||||||||||||||||||||||||||||||||||||
| this.authTriggerCount++; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if (data.includes(STORAGE_FUNCTION_ARCHIVED_LOG)) { | ||||||||||||||||||||||||||||||||||||||
|
Check warning on line 219 in scripts/integration-helpers/framework.ts
|
||||||||||||||||||||||||||||||||||||||
| this.storageArchivedTriggerCount++; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if (data.includes(STORAGE_FUNCTION_DELETED_LOG)) { | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -395,26 +395,40 @@ | |||||||||||||||||||||||||||||||||||||
| return this.invokeHttpFunction("updateDeleteFromSpecificStorageBucket"); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| waitForCondition( | ||||||||||||||||||||||||||||||||||||||
| conditionFn: () => boolean | Promise<boolean>, | ||||||||||||||||||||||||||||||||||||||
| timeoutMs?: number, | ||||||||||||||||||||||||||||||||||||||
| intervalMs?: number, | ||||||||||||||||||||||||||||||||||||||
| ): Promise<void>; | ||||||||||||||||||||||||||||||||||||||
| waitForCondition( | ||||||||||||||||||||||||||||||||||||||
| conditionFn: () => boolean, | ||||||||||||||||||||||||||||||||||||||
| timeout: number, | ||||||||||||||||||||||||||||||||||||||
| timeoutMs: number, | ||||||||||||||||||||||||||||||||||||||
| callback: (err?: Error) => void, | ||||||||||||||||||||||||||||||||||||||
| ): void { | ||||||||||||||||||||||||||||||||||||||
| let elapsed = 0; | ||||||||||||||||||||||||||||||||||||||
| const interval = 10; | ||||||||||||||||||||||||||||||||||||||
| const id = setInterval(() => { | ||||||||||||||||||||||||||||||||||||||
| elapsed += interval; | ||||||||||||||||||||||||||||||||||||||
| if (elapsed > timeout) { | ||||||||||||||||||||||||||||||||||||||
| clearInterval(id); | ||||||||||||||||||||||||||||||||||||||
| callback(new Error(`Timed out waiting for condition: ${conditionFn.toString()}}`)); | ||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| ): void; | ||||||||||||||||||||||||||||||||||||||
| waitForCondition( | ||||||||||||||||||||||||||||||||||||||
| conditionFn: () => boolean | Promise<boolean>, | ||||||||||||||||||||||||||||||||||||||
| timeoutMs = 10000, | ||||||||||||||||||||||||||||||||||||||
| intervalMsOrCallback: number | ((err?: Error) => void) = 50, | ||||||||||||||||||||||||||||||||||||||
| callback?: (err?: Error) => void, | ||||||||||||||||||||||||||||||||||||||
| ): Promise<void> | void { | ||||||||||||||||||||||||||||||||||||||
| let intervalMs = 50; | ||||||||||||||||||||||||||||||||||||||
| let cb: ((err?: Error) => void) | undefined; | ||||||||||||||||||||||||||||||||||||||
| if (typeof intervalMsOrCallback === "function") { | ||||||||||||||||||||||||||||||||||||||
| cb = intervalMsOrCallback; | ||||||||||||||||||||||||||||||||||||||
| } else if (typeof intervalMsOrCallback === "number") { | ||||||||||||||||||||||||||||||||||||||
| intervalMs = intervalMsOrCallback; | ||||||||||||||||||||||||||||||||||||||
| cb = callback; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if (conditionFn()) { | ||||||||||||||||||||||||||||||||||||||
| clearInterval(id); | ||||||||||||||||||||||||||||||||||||||
| callback(); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| }, interval); | ||||||||||||||||||||||||||||||||||||||
| const promise = waitForCondition(conditionFn, timeoutMs, intervalMs); | ||||||||||||||||||||||||||||||||||||||
| if (cb) { | ||||||||||||||||||||||||||||||||||||||
| promise.then( | ||||||||||||||||||||||||||||||||||||||
| () => cb?.(), | ||||||||||||||||||||||||||||||||||||||
| (err: unknown) => cb?.(err instanceof Error ? err : new Error(String(err))), | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+424
to
+430
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the callback
Suggested change
References
|
||||||||||||||||||||||||||||||||||||||
| return promise; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| disableBackgroundTriggers(): Promise<Response> { | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -427,3 +441,29 @@ | |||||||||||||||||||||||||||||||||||||
| return fetch(url, { method: "PUT" }); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||
| * Polls for a condition to be met within a specified timeout. | ||||||||||||||||||||||||||||||||||||||
| * Resolves immediately once predicate returns true. | ||||||||||||||||||||||||||||||||||||||
| * @param predicate A synchronous or asynchronous function returning a boolean. | ||||||||||||||||||||||||||||||||||||||
| * @param timeoutMs Maximum time to wait in milliseconds (default: 10000ms). | ||||||||||||||||||||||||||||||||||||||
| * @param intervalMs Time between predicate evaluations in milliseconds (default: 50ms). | ||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||
| export async function waitForCondition( | ||||||||||||||||||||||||||||||||||||||
| predicate: () => boolean | Promise<boolean>, | ||||||||||||||||||||||||||||||||||||||
| timeoutMs = 10000, | ||||||||||||||||||||||||||||||||||||||
| intervalMs = 50, | ||||||||||||||||||||||||||||||||||||||
| ): Promise<void> { | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+452
to
+456
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default timeout of
Suggested change
References
|
||||||||||||||||||||||||||||||||||||||
| const startTime = Date.now(); | ||||||||||||||||||||||||||||||||||||||
| for (;;) { | ||||||||||||||||||||||||||||||||||||||
| if (await predicate()) { | ||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if (Date.now() - startTime >= timeoutMs) { | ||||||||||||||||||||||||||||||||||||||
| throw new Error( | ||||||||||||||||||||||||||||||||||||||
| `Timed out waiting for condition after ${timeoutMs}ms: ${predicate.toString()}`, | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| await new Promise((resolve) => setTimeout(resolve, intervalMs)); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,15 +2,18 @@ import { expect } from "chai"; | |
| import * as fs from "fs"; | ||
| import * as path from "path"; | ||
|
|
||
| import { FrameworkOptions, TriggerEndToEndTest } from "../integration-helpers/framework"; | ||
| import { | ||
| FrameworkOptions, | ||
| TriggerEndToEndTest, | ||
| waitForCondition, | ||
| } from "../integration-helpers/framework"; | ||
|
|
||
| const FIREBASE_PROJECT = process.env.FBTOOLS_TARGET_PROJECT || ""; | ||
| /* | ||
| * Various delays that are needed because this test spawns | ||
| * parallel emulator subprocesses. | ||
| */ | ||
| const TEST_SETUP_TIMEOUT = process.platform === "win32" ? 180000 : 80000; | ||
| const EMULATORS_WRITE_DELAY_MS = process.platform === "win32" ? 10000 : 5000; | ||
| const EMULATORS_SHUTDOWN_DELAY_MS = process.platform === "win32" ? 30000 : 5000; | ||
|
|
||
| function readConfig(): FrameworkOptions { | ||
|
|
@@ -78,7 +81,7 @@ describe("function triggers with inspect flag", () => { | |
| this.timeout(TEST_SETUP_TIMEOUT); | ||
| 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of importing and calling the global await test.waitForCondition(() => test.authTriggerCount >= 1);References
|
||
| expect(test.authTriggerCount).to.equal(1); | ||
| }); | ||
|
|
||
|
|
@@ -87,7 +90,9 @@ describe("function triggers with inspect flag", () => { | |
|
|
||
| const response = await test.writeToDefaultStorage(); | ||
| expect(response.status).to.equal(200); | ||
| await new Promise((resolve) => setTimeout(resolve, EMULATORS_WRITE_DELAY_MS)); | ||
| await waitForCondition( | ||
| () => test.storageFinalizedTriggerCount >= 1 && test.storageV2FinalizedTriggerCount >= 1, | ||
| ); | ||
|
|
||
| expect(test.storageFinalizedTriggerCount).to.equal(1); | ||
| expect(test.storageV2FinalizedTriggerCount).to.equal(1); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with the global
waitForConditionhelper and to prevent flakiness on slow CI environments, we should increase the defaulttimeoutMsto20000ms here as well.References