Merge branch 'master' into master

Author: rhodgkins

package-lock.json

@@ -1,6 +1,6 @@
 {
   "name": "firebase-functions",
-  "version": "6.3.1",
+  "version": "6.4.0",
   "description": "Firebase SDK for Cloud Functions",
   "keywords": [
     "firebase",

package.json

@@ -0,0 +1,6 @@
+const functions = require("firebase-functions");
+
+// This will cause a syntax error
+exports.broken = functions.https.onRequest((request, response) => {
+  response.send("Hello from Firebase!"
+}); // Missing closing parenthesis

scripts/bin-test/sources/broken-syntax/index.js

@@ -0,0 +1,3 @@
+{
+  "name": "broken-syntax"
+}

scripts/bin-test/sources/broken-syntax/package.json

@@ -1,6 +1,8 @@
 import * as subprocess from "child_process";
 import * as path from "path";
 import { promisify } from "util";
+import * as fs from "fs/promises";
+import * as os from "os";
 
 import { expect } from "chai";
 import * as yaml from "js-yaml";
@@ -105,7 +107,13 @@ const BASE_STACK = {
 interface Testcase {
   name: string;
   modulePath: string;
-  expected: Record<string, any>;
+  expected: Record<string, unknown>;
+}
+
+interface DiscoveryResult {
+  success: boolean;
+  manifest?: Record<string, unknown>;
+  error?: string;
 }
 
 async function retryUntil(
@@ -134,102 +142,134 @@ async function retryUntil(
   await Promise.race([retry, timedOut]);
 }
 
-async function startBin(
-  tc: Testcase,
-  debug?: boolean
-): Promise<{ port: number; cleanup: () => Promise<void> }> {
+async function runHttpDiscovery(modulePath: string): Promise<DiscoveryResult> {
   const getPort = promisify(portfinder.getPort) as () => Promise<number>;
   const port = await getPort();
 
   const proc = subprocess.spawn("npx", ["firebase-functions"], {
-    cwd: path.resolve(tc.modulePath),
+    cwd: path.resolve(modulePath),
     env: {
       PATH: process.env.PATH,
-      GLCOUD_PROJECT: "test-project",
+      GCLOUD_PROJECT: "test-project",
       PORT: port.toString(),
       FUNCTIONS_CONTROL_API: "true",
     },
   });
-  if (!proc) {
-    throw new Error("Failed to start firebase functions");
-  }
-  proc.stdout?.on("data", (chunk: Buffer) => {
-    console.log(chunk.toString("utf8"));
-  });
-  proc.stderr?.on("data", (chunk: Buffer) => {
-    console.log(chunk.toString("utf8"));
-  });
 
-  await retryUntil(async () => {
-    try {
-      await fetch(`http://localhost:${port}/__/functions.yaml`);
-    } catch (e) {
-      if (e?.code === "ECONNREFUSED") {
-        return false;
+  try {
+    // Wait for server to be ready
+    await retryUntil(async () => {
+      try {
+        await fetch(`http://localhost:${port}/__/functions.yaml`);
+        return true;
+      } catch (e: unknown) {
+        const error = e as { code?: string };
+        if (error.code === "ECONNREFUSED") {
+          // This is an expected error during server startup, so we should retry.
+          return false;
+        }
+        // Any other error is unexpected and should fail the test immediately.
+        throw e;
       }
-      throw e;
+    }, TIMEOUT_L);
+
+    const res = await fetch(`http://localhost:${port}/__/functions.yaml`);
+    const body = await res.text();
+
+    if (res.status === 200) {
+      const manifest = yaml.load(body) as Record<string, unknown>;
+      return { success: true, manifest };
+    } else {
+      return { success: false, error: body };
     }
-    return true;
-  }, TIMEOUT_L);
+  } finally {
+    if (proc.pid) {
+      proc.kill(9);
+      await new Promise<void>((resolve) => proc.on("exit", resolve));
+    }
+  }
+}
 
-  if (debug) {
-    proc.stdout?.on("data", (data: unknown) => {
-      console.log(`[${tc.name} stdout] ${data}`);
+async function runFileDiscovery(modulePath: string): Promise<DiscoveryResult> {
+  const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "firebase-functions-test-"));
+  const outputPath = path.join(tempDir, "manifest.json");
+
+  return new Promise((resolve, reject) => {
+    const proc = subprocess.spawn("npx", ["firebase-functions"], {
+      cwd: path.resolve(modulePath),
+      env: {
+        PATH: process.env.PATH,
+        GCLOUD_PROJECT: "test-project",
+        FUNCTIONS_MANIFEST_OUTPUT_PATH: outputPath,
+      },
     });
 
-    proc.stderr?.on("data", (data: unknown) => {
-      console.log(`[${tc.name} stderr] ${data}`);
+    let stderr = "";
+
+    proc.stderr?.on("data", (chunk: Buffer) => {
+      stderr += chunk.toString("utf8");
     });
-  }
 
-  return {
-    port,
-    cleanup: async () => {
-      process.kill(proc.pid, 9);
-      await retryUntil(async () => {
+    const timeoutId = setTimeout(async () => {
+      if (proc.pid) {
+        proc.kill(9);
+        await new Promise<void>((resolve) => proc.on("exit", resolve));
+      }
+      resolve({ success: false, error: `File discovery timed out after ${TIMEOUT_M}ms` });
+    }, TIMEOUT_M);
+
+    proc.on("close", async (code) => {
+      clearTimeout(timeoutId);
+
+      if (code === 0) {
         try {
-          process.kill(proc.pid, 0);
-        } catch {
-          // process.kill w/ signal 0 will throw an error if the pid no longer exists.
-          return Promise.resolve(true);
+          const manifestJson = await fs.readFile(outputPath, "utf8");
+          const manifest = JSON.parse(manifestJson) as Record<string, unknown>;
+          await fs.rm(tempDir, { recursive: true }).catch(() => {
+            // Ignore errors
+          });
+          resolve({ success: true, manifest });
+        } catch (e) {
+          resolve({ success: false, error: `Failed to read manifest file: ${e}` });
         }
-        return Promise.resolve(false);
-      }, TIMEOUT_L);
-    },
-  };
+      } else {
+        const errorLines = stderr.split("\n").filter((line) => line.trim());
+        const errorMessage = errorLines.join(" ") || "No error message found";
+        resolve({ success: false, error: errorMessage });
+      }
+    });
+
+    proc.on("error", (err) => {
+      clearTimeout(timeoutId);
+      // Clean up temp directory on error
+      fs.rm(tempDir, { recursive: true }).catch(() => {
+        // Ignore errors
+      });
+      reject(err);
+    });
+  });
 }
 
 describe("functions.yaml", function () {
   // eslint-disable-next-line @typescript-eslint/no-invalid-this
   this.timeout(TIMEOUT_XL);
 
-  function runTests(tc: Testcase) {
-    let port: number;
-    let cleanup: () => Promise<void>;
-
-    before(async () => {
-      const r = await startBin(tc);
-      port = r.port;
-      cleanup = r.cleanup;
-    });
+  const discoveryMethods = [
+    { name: "http", fn: runHttpDiscovery },
+    { name: "file", fn: runFileDiscovery },
+  ];
 
-    after(async () => {
-      await cleanup?.();
-    });
-
-    it("functions.yaml returns expected Manifest", async function () {
+  function runDiscoveryTests(
+    tc: Testcase,
+    discoveryFn: (path: string) => Promise<DiscoveryResult>
+  ) {
+    it("returns expected manifest", async function () {
       // eslint-disable-next-line @typescript-eslint/no-invalid-this
       this.timeout(TIMEOUT_M);
 
-      const res = await fetch(`http://localhost:${port}/__/functions.yaml`);
-      const text = await res.text();
-      let parsed: any;
-      try {
-        parsed = yaml.load(text);
-      } catch (err) {
-        throw new Error(`Failed to parse functions.yaml: ${err}`);
-      }
-      expect(parsed).to.be.deep.equal(tc.expected);
+      const result = await discoveryFn(tc.modulePath);
+      expect(result.success).to.be.true;
+      expect(result.manifest).to.deep.equal(tc.expected);
     });
   }
 
@@ -320,7 +360,11 @@ describe("functions.yaml", function () {
 
     for (const tc of testcases) {
       describe(tc.name, () => {
-        runTests(tc);
+        for (const discovery of discoveryMethods) {
+          describe(`${discovery.name} discovery`, () => {
+            runDiscoveryTests(tc, discovery.fn);
+          });
+        }
       });
     }
   });
@@ -350,7 +394,33 @@ describe("functions.yaml", function () {
 
     for (const tc of testcases) {
       describe(tc.name, () => {
-        runTests(tc);
+        for (const discovery of discoveryMethods) {
+          describe(`${discovery.name} discovery`, () => {
+            runDiscoveryTests(tc, discovery.fn);
+          });
+        }
+      });
+    }
+  });
+
+  describe("error handling", () => {
+    const errorTestcases = [
+      {
+        name: "broken syntax",
+        modulePath: "./scripts/bin-test/sources/broken-syntax",
+        expectedError: "missing ) after argument list",
+      },
+    ];
+
+    for (const tc of errorTestcases) {
+      describe(tc.name, () => {
+        for (const discovery of discoveryMethods) {
+          it(`${discovery.name} discovery handles error correctly`, async () => {
+            const result = await discovery.fn(tc.modulePath);
+            expect(result.success).to.be.false;
+            expect(result.error).to.include(tc.expectedError);
+          });
+        }
       });
     }
   });

scripts/bin-test/test.ts

@@ -78,9 +78,7 @@ export function runHandler(
 
         const toSend = typeof sendBody === "object" ? JSON.stringify(sendBody) : sendBody;
         const body =
-          typeof this.sentBody === "undefined"
-            ? toSend
-            : this.sentBody + String(toSend || "");
+          typeof this.sentBody === "undefined" ? toSend : this.sentBody + String(toSend || "");
         this.end(body);
       }
 

spec/helper.ts

@@ -118,6 +118,50 @@ describe("logger", () => {
           });
         });
 
+        it("should not detect duplicate object as circular", () => {
+          const obj: any = { a: "foo" };
+          const entry: logger.LogEntry = {
+            severity: "ERROR",
+            message: "testing circular",
+            a: obj,
+            b: obj,
+          };
+          logger.write(entry);
+          expectStderr({
+            severity: "ERROR",
+            message: "testing circular",
+            a: { a: "foo" },
+            b: { a: "foo" },
+          });
+        });
+
+        it("should not detect duplicate object in array as circular", () => {
+          const obj: any = { a: "foo" };
+          const arr: any = [
+            { a: obj, b: obj },
+            { a: obj, b: obj },
+          ];
+          const entry: logger.LogEntry = {
+            severity: "ERROR",
+            message: "testing circular",
+            a: arr,
+            b: arr,
+          };
+          logger.write(entry);
+          expectStderr({
+            severity: "ERROR",
+            message: "testing circular",
+            a: [
+              { a: { a: "foo" }, b: { a: "foo" } },
+              { a: { a: "foo" }, b: { a: "foo" } },
+            ],
+            b: [
+              { a: { a: "foo" }, b: { a: "foo" } },
+              { a: { a: "foo" }, b: { a: "foo" } },
+            ],
+          });
+        });
+
         it("should not break on objects that override toJSON", () => {
           const obj: any = { a: new Date("August 26, 1994 12:24:00Z") };