Fix secrets in CI tests (#13306)

.buildkite/scripts/env.sh

@@ -149,5 +149,3 @@ function export_environment() {
 assert_build
 assert_buildkite_agent
 export_environment
-
-source "$ROOT_DIR/.buildkite/scripts/secrets.sh"

.buildkite/scripts/secrets.sh

@@ -1,33 +0,0 @@
-#!/bin/bash
-
-set -euo pipefail
-
-function ensure_secret() {
-  local name=""
-  local value=""
-  name="$1"
-  value="$(buildkite-agent secret get $name)"
-  # If secret is not found, then we should exit with an error
-  if [ -z "$value" ]; then
-    echo "error: Secret $name not found"
-    exit 1
-  fi
-
-  export "$name"="$value"
-}
-
-function optional_secret() {
-  local name=""
-  local value=""
-  name="$1"
-  value="$(buildkite-agent secret get $name) 2>/dev/null"
-
-  export "$name"="$value"
-}
-
-ensure_secret "TLS_MONGODB_DATABASE_URL"
-ensure_secret "TLS_POSTGRES_DATABASE_URL"
-ensure_secret "TEST_INFO_STRIPE"
-ensure_secret "TEST_INFO_AZURE_SERVICE_BUS"
-optional_secret "SMTP_SENDGRID_KEY"
-optional_secret "SMTP_SENDGRID_SENDER"

scripts/runner.node.mjs

@@ -26,84 +26,6 @@ import { normalize as normalizeWindows } from "node:path/win32";
 import { isIP } from "node:net";
 import { parseArgs } from "node:util";
 
-const secrets = [
-  "TLS_MONGODB_DATABASE_URL",
-  "TLS_POSTGRES_DATABASE_URL",
-  "TEST_INFO_STRIPE",
-  "TEST_INFO_AZURE_SERVICE_BUS",
-  "SMTP_SENDGRID_KEY",
-  "SMTP_SENDGRID_SENDER",
-];
-Promise.withResolvers ??= function () {
-  var resolvers = {
-    resolve: null,
-    reject: null,
-    promise: null,
-  };
-  resolvers.promise = new Promise((resolve, reject) => {
-    resolvers.resolve = resolve;
-    resolvers.reject = reject;
-  });
-
-  return resolvers;
-};
-
-async function getSecret(secret) {
-  if (process.env[secret]) {
-    return process.env[secret];
-  }
-
-  const proc = spawn("buildkite-agent", ["secret", "get", secret], {
-    encoding: "utf-8",
-    stdio: ["inherit", "pipe", "inherit"],
-  });
-  let { resolve, reject, promise } = Promise.withResolvers();
-
-  let stdoutPromise;
-  {
-    let { resolve, reject, promise } = Promise.withResolvers();
-    stdoutPromise = promise;
-    let stdout = "";
-    proc.stdout.setEncoding("utf-8");
-    proc.stdout.on("data", chunk => {
-      stdout += chunk.toString();
-    });
-    proc.stdout.on("end", () => {
-      stdout = stdout.trim();
-      resolve(stdout);
-    });
-  }
-
-  proc.on("exit", (code, signal) => {
-    if (code === 0) {
-      resolve();
-    } else {
-      reject(new Error(`Secret "${secret}" not found with code ${code}, signal ${signal}`));
-    }
-  });
-
-  await promise;
-
-  resolve(await stdoutPromise);
-}
-await Promise.all(
-  secrets.map(async secret => {
-    if (process.env[secret]) {
-      return;
-    }
-
-    try {
-      const value = await getSecret(secret);
-      if (value) {
-        process.env[secret] = value;
-      }
-    } catch (error) {
-      console.warn(error);
-      // We continue to let the individual tests fail.
-    }
-  }),
-);
-
 const spawnTimeout = 5_000;
 const testTimeout = 3 * 60_000;
 const integrationTimeout = 5 * 60_000;

test/harness.ts

@@ -1,4 +1,4 @@
-import { gc as bunGC, unsafe, which } from "bun";
+import { gc as bunGC, spawnSync, unsafe, which } from "bun";
 import { describe, test, expect, afterAll, beforeAll } from "bun:test";
 import { readlink, readFile, writeFile } from "fs/promises";
 import { isAbsolute, join, dirname } from "path";
@@ -1220,25 +1220,47 @@ export function fileDescriptorLeakChecker() {
   };
 }
 
-export function requireCredentials(...envNames: any[]) {
-  const envs = envNames.slice(0, envNames.length - 1);
-  const testFn = envNames[envNames.length - 1];
-  const missing = envs.filter(envName => !process.env[envName]);
-  if (missing.length > 0) {
-    return function (label: string, fn: Function) {
-      return testFn(label, () => {
-        const err = new Error(
-          "Test is missing required credentials: " +
-            missing.map(envName => JSON.stringify(envName)).join(", ") +
-            "\n\nPlease set the following environment variables:\n" +
-            missing.map(envName => "- " + JSON.stringify(envName)).join("\n") +
-            "\n",
-        );
-        err.name = "MissingCredentialError";
-        throw err;
-      });
-    };
+/**
+ * Gets a secret from the environment.
+ *
+ * In Buildkite, secrets must be retrieved using the `buildkite-agent secret get` command
+ * and are not available as an environment variable.
+ */
+export function getSecret(name: string): string | undefined {
+  let value = process.env[name]?.trim();
+
+  // When not running in CI, allow the secret to be missing.
+  if (!isCI) {
+    return value;
   }
 
-  return testFn;
+  // In Buildkite, secrets must be retrieved using the `buildkite-agent secret get` command
+  if (!value && isBuildKite) {
+    const { exitCode, stdout } = spawnSync({
+      cmd: ["buildkite-agent", "secret", "get", name],
+      stdout: "pipe",
+      stderr: "inherit",
+    });
+    if (exitCode === 0) {
+      value = stdout.toString().trim();
+    }
+  }
+
+  // Throw an error if the secret is not found, so the test fails in CI.
+  if (!value) {
+    let hint;
+    if (isBuildKite) {
+      hint = `Create a secret with the name "${name}" in the Buildkite UI.
+https://buildkite.com/docs/pipelines/security/secrets/buildkite-secrets`;
+    } else {
+      hint = `Define an environment variable with the name "${name}".`;
+    }
+
+    throw new Error(`Secret not found: ${name}\n${hint}`);
+  }
+
+  // Set the secret in the environment so that it can be used in tests.
+  process.env[name] = value;
+
+  return value;
 }

test/js/third_party/@azure/service-bus/azure-service-bus.test.ts

@@ -0,0 +1,21 @@
+import { getSecret } from "harness";
+import { describe, test } from "bun:test";
+import { ServiceBusClient } from "@azure/service-bus";
+
+const azureCredentials = getSecret("TEST_INFO_AZURE_SERVICE_BUS");
+
+describe.skipIf(!azureCredentials)("@azure/service-bus", () => {
+  test("works", async () => {
+    const sbClient = new ServiceBusClient(azureCredentials!);
+    const sender = sbClient.createSender("test");
+
+    try {
+      await sender.sendMessages({ body: "Hello, world!" });
+      await sender.close();
+    } finally {
+      await sbClient.close();
+    }
+  }, 10_000);
+  // this takes ~4s locally so increase the time to try and ensure its
+  // not flaky in a higher pressure environment
+});

test/js/third_party/@azure/service-bus/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "@azure/service-bus",
+  "dependencies": {
+    "@azure/service-bus": "7.9.4"
+  }
+}

test/js/third_party/_fixtures/stripe.ts

@@ -1,8 +0,0 @@
-const Stripe = require("stripe");
-const stripe = Stripe(process.env.STRIPE_ACCESS_TOKEN);
-
-await stripe.charges
-  .retrieve(process.env.STRIPE_CHARGE_ID, { stripeAccount: process.env.STRIPE_ACCOUNT_ID })
-  .then(x => {
-    console.log(x);
-  });

test/js/third_party/azure-service-bus.test.ts

@@ -1,60 +0,0 @@
-import { bunExe } from "bun:harness";
-import { bunEnv, requireCredentials, tmpdirSync } from "harness";
-import { expect, test } from "bun:test";
-import * as path from "node:path";
-
-// DO NOT SKIP IN CI.
-const it = requireCredentials("TEST_INFO_AZURE_SERVICE_BUS", test);
-
-it("works", async () => {
-  const package_dir = tmpdirSync("bun-test-");
-
-  let { stdout, stderr, exited } = Bun.spawn({
-    cmd: [bunExe(), "add", "@azure/service-bus@7.9.4"],
-    cwd: package_dir,
-    stdout: "pipe",
-    stdin: "ignore",
-    stderr: "pipe",
-    env: bunEnv,
-  });
-  let err = await new Response(stderr).text();
-  expect(err).not.toContain("panic:");
-  expect(err).not.toContain("error:");
-  expect(err).not.toContain("warn:");
-  let out = await new Response(stdout).text();
-  expect(await exited).toBe(0);
-
-  const fixture_path = path.join(package_dir, "index.ts");
-  const fixture_data = `
-    import { ServiceBusClient } from "@azure/service-bus";
-
-    const connectionString = "${bunEnv.TEST_INFO_AZURE_SERVICE_BUS}";
-    const sbClient = new ServiceBusClient(connectionString);
-    const sender = sbClient.createSender("test");
-    
-    try {
-      await sender.sendMessages({ body: "Hello, world!" });
-      console.log("Message sent");
-      await sender.close();
-    } finally {
-      await sbClient.close();
-    }
-  `;
-  await Bun.write(fixture_path, fixture_data);
-
-  ({ stdout, stderr, exited } = Bun.spawn({
-    cmd: [bunExe(), "run", fixture_path],
-    cwd: package_dir,
-    stdout: "pipe",
-    stdin: "ignore",
-    stderr: "pipe",
-    env: bunEnv,
-  }));
-  err = await new Response(stderr).text();
-  expect(err).toBeEmpty();
-  out = await new Response(stdout).text();
-  expect(out).toEqual("Message sent\n");
-  expect(await exited).toBe(0);
-}, 10_000);
-// this takes ~4s locally so increase the time to try and ensure its
-// not flaky in a higher pressure environment

test/js/third_party/mongodb/mongodb.test.ts

@@ -1,15 +1,12 @@
 import { test, expect, describe } from "bun:test";
-import { requireCredentials } from "harness";
+import { getSecret } from "harness";
 import { MongoClient } from "mongodb";
 
-const CONNECTION_STRING = process.env.TLS_MONGODB_DATABASE_URL;
+const databaseUrl = getSecret("TLS_MONGODB_DATABASE_URL");
 
-// DO NOT SKIP IN CI.
-const it = requireCredentials("TLS_MONGODB_DATABASE_URL", test);
-
-describe("mongodb", () => {
-  it("should connect and inpect", async () => {
-    const client = new MongoClient(CONNECTION_STRING as string);
+describe.skipIf(!databaseUrl)("mongodb", () => {
+  test("should connect and inpect", async () => {
+    const client = new MongoClient(databaseUrl!);
 
     const clientConnection = await client.connect();
 

test/js/third_party/msw/msw.test.ts

@@ -3,5 +3,5 @@ import { expect, it } from "bun:test";
 import * as path from "node:path";
 
 it("works", async () => {
-  expect([path.join(import.meta.dirname, "_fixtures", "msw.ts")]).toRun("2\n");
+  expect([path.join(import.meta.dirname, "msw.fixture.ts")]).toRun("2\n");
 });

test/js/third_party/nodemailer/nodemailer.test.ts

@@ -1,14 +1,14 @@
 import { test, expect, describe } from "bun:test";
-import { bunRun, requireCredentials } from "harness";
+import { bunRun, getSecret } from "harness";
 import path from "path";
 
-// DO NOT SKIP IN CI.
-const it = requireCredentials("SMTP_SENDGRID_KEY", "SMTP_SENDGRID_SENDER", test);
+const smtpKey = getSecret("SMTP_SENDGRID_KEY");
+const smtpSender = getSecret("SMTP_SENDGRID_SENDER");
 
-describe("nodemailer", () => {
-  it("basic smtp", async () => {
+describe.skipIf(!smtpKey || !smtpSender)("nodemailer", () => {
+  test("basic smtp", async () => {
     try {
-      const info = bunRun(path.join(import.meta.dir, "process-nodemailer-fixture.js"), {
+      const info = bunRun(path.join(import.meta.dir, "nodemailer.fixture.js"), {
         SMTP_SENDGRID_SENDER: process.env.SMTP_SENDGRID_SENDER as string,
         SMTP_SENDGRID_KEY: process.env.SMTP_SENDGRID_KEY as string,
       });

test/js/third_party/postgres/package.json

@@ -1,7 +1,6 @@
 {
   "name": "postgres",
   "dependencies": {
-    "pg": "8.11.1", 
     "postgres": "3.3.5",
     "pg-connection-string": "2.6.1"
   }

test/js/third_party/postgres/postgres.test.ts

@@ -1,43 +1,12 @@
 import { test, expect, describe } from "bun:test";
-import { isCI, requireCredentials } from "harness";
-import { Pool, Client } from "pg";
-import { parse } from "pg-connection-string";
+import { getSecret } from "harness";
 import postgres from "postgres";
 
-const CONNECTION_STRING = process.env.TLS_POSTGRES_DATABASE_URL;
+const databaseUrl = getSecret("TLS_POSTGRES_DATABASE_URL");
 
-// DO NOT SKIP IN CI.
-const it = requireCredentials("TLS_POSTGRES_DATABASE_URL", test);
-
-describe("pg", () => {
-  it("should connect using TLS", async () => {
-    const pool = new Pool(parse(CONNECTION_STRING as string));
-    try {
-      const { rows } = await pool.query("SELECT version()", []);
-      const [{ version }] = rows;
-
-      expect(version).toMatch(/PostgreSQL/);
-    } finally {
-      pool.end();
-    }
-  });
-
-  it("should execute big query and end connection", async () => {
-    const client = new Client({
-      connectionString: CONNECTION_STRING,
-      ssl: { rejectUnauthorized: false },
-    });
-
-    await client.connect();
-    const res = await client.query(`SELECT * FROM users LIMIT 1000`);
-    expect(res.rows.length).toBeGreaterThanOrEqual(300);
-    await client.end();
-  }, 5000);
-});
-
-describe("postgres", () => {
-  it("should connect using TLS", async () => {
-    const sql = postgres(CONNECTION_STRING as string);
+describe.skipIf(!databaseUrl)("postgres", () => {
+  test("should connect using TLS", async () => {
+    const sql = postgres(databaseUrl!);
     try {
       const [{ version }] = await sql`SELECT version()`;
       expect(version).toMatch(/PostgreSQL/);
@@ -46,8 +15,8 @@ describe("postgres", () => {
     }
   });
 
-  it("should insert, select and delete", async () => {
-    const sql = postgres(CONNECTION_STRING as string);
+  test("should insert, select and delete", async () => {
+    const sql = postgres(databaseUrl!);
     try {
       await sql`CREATE TABLE IF NOT EXISTS usernames (
             user_id serial PRIMARY KEY,

test/js/third_party/pq/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "pq",
+  "dependencies": {
+    "pg": "8.11.1"
+  }
+}

test/js/third_party/pq/pq.test.ts

@@ -0,0 +1,32 @@
+import { test, expect, describe } from "bun:test";
+import { getSecret } from "harness";
+import { Pool, Client } from "pg";
+import { parse } from "pg-connection-string";
+
+const databaseUrl = getSecret("TLS_POSTGRES_DATABASE_URL");
+
+describe.skipIf(!databaseUrl)("pg", () => {
+  test("should connect using TLS", async () => {
+    const pool = new Pool(parse(databaseUrl!));
+    try {
+      const { rows } = await pool.query("SELECT version()", []);
+      const [{ version }] = rows;
+
+      expect(version).toMatch(/PostgreSQL/);
+    } finally {
+      pool.end();
+    }
+  });
+
+  test("should execute big query and end connection", async () => {
+    const client = new Client({
+      connectionString: databaseUrl!,
+      ssl: { rejectUnauthorized: false },
+    });
+
+    await client.connect();
+    const res = await client.query(`SELECT * FROM users LIMIT 1000`);
+    expect(res.rows.length).toBeGreaterThanOrEqual(300);
+    await client.end();
+  });
+});

test/js/third_party/st/st.fixture.ts

@@ -14,5 +14,5 @@ function listen(server): Promise<URL> {
 }
 await using server = createServer(st(process.cwd()));
 const url = await listen(server);
-const res = await fetch(new URL("/st.ts", url));
+const res = await fetch(new URL("/st.fixture.ts", url));
 console.log(await res.text());

test/js/third_party/st/st.test.ts

@@ -3,7 +3,7 @@ import { expect, it } from "bun:test";
 import { join, dirname } from "node:path";
 
 it("works", async () => {
-  const fixture_path = join(import.meta.dirname, "_fixtures", "st.ts");
+  const fixture_path = join(import.meta.dirname, "st.fixture.ts");
   const fixture_data = await Bun.file(fixture_path).text();
   let { stdout, stderr, exited } = Bun.spawn({
     cmd: [bunExe(), "run", fixture_path],

test/js/third_party/stripe.test.ts

@@ -1,29 +0,0 @@
-import { bunExe } from "bun:harness";
-import { bunEnv, requireCredentials, runBunInstall, tmpdirSync } from "harness";
-import * as path from "node:path";
-import { expect, test } from "bun:test";
-
-// DO NOT SKIP IN CI.
-const it = requireCredentials("TEST_INFO_STRIPE", test);
-
-it("should be able to query a charge", async () => {
-  const [access_token, charge_id, account_id] = process.env.TEST_INFO_STRIPE?.split(",");
-
-  let { stdout, stderr } = Bun.spawn({
-    cmd: [bunExe(), "run", path.join(import.meta.dirname, "_fixtures", "stripe.ts")],
-    cwd: import.meta.dirname,
-    stdout: "pipe",
-    stdin: "ignore",
-    stderr: "pipe",
-    env: {
-      ...bunEnv,
-      STRIPE_ACCESS_TOKEN: access_token,
-      STRIPE_CHARGE_ID: charge_id,
-      STRIPE_ACCOUNT_ID: account_id,
-    },
-  });
-  let out = await new Response(stdout).text();
-  expect(out).toBeEmpty();
-  let err = await new Response(stderr).text();
-  expect(err).toContain(`error: No such charge: '${charge_id}'\n`);
-});

test/js/third_party/stripe/stripe.test.ts

@@ -0,0 +1,16 @@
+import { getSecret } from "harness";
+import { expect, test, describe } from "bun:test";
+import { Stripe } from "stripe";
+
+const stripeCredentials = getSecret("TEST_INFO_STRIPE");
+
+describe.skipIf(!stripeCredentials)("stripe", () => {
+  const [accessToken, chargeId, accountId] = process.env.TEST_INFO_STRIPE?.split(",") ?? [];
+  const stripe = new Stripe(accessToken);
+
+  test("should be able to query a charge", async () => {
+    expect(stripe.charges.retrieve(chargeId, { stripeAccount: accountId })).rejects.toThrow(
+      `No such charge: '${chargeId}'`,
+    );
+  });
+});

test/package.json

@@ -7,6 +7,7 @@
     "@types/utf-8-validate": "5.0.0"
   },
   "dependencies": {
+    "@azure/service-bus": "7.9.4",
     "@grpc/grpc-js": "1.9.9",
     "@grpc/proto-loader": "0.7.10",
     "@napi-rs/canvas": "0.1.47",