do not use Deno.env.set indiscriminately to avoid races

Author: cscheid

src/quarto.ts

@@ -77,14 +77,10 @@ const checkReconfiguration = async () => {
   }
 };
 
-const passThroughPandoc = async (
-  args: string[],
-  env?: Record<string, string>,
-) => {
+const passThroughPandoc = async (args: string[]) => {
   const result = await execProcess(
     {
       cmd: [pandocBinaryPath(), ...args.slice(1)],
-      env,
     },
     undefined,
     undefined,
@@ -94,10 +90,7 @@ const passThroughPandoc = async (
   Deno.exit(result.code);
 };
 
-const passThroughTypst = async (
-  args: string[],
-  env?: Record<string, string>,
-) => {
+const passThroughTypst = async (args: string[]) => {
   if (args[1] === "update") {
     error(
       "The 'typst update' command is not supported.\n" +
@@ -107,28 +100,26 @@ const passThroughTypst = async (
   }
   const result = await execProcess({
     cmd: [typstBinaryPath(), ...args.slice(1)],
-    env,
   });
   Deno.exit(result.code);
 };
 
 export async function quarto(
   args: string[],
   cmdHandler?: (command: Command) => Command,
-  env?: Record<string, string>,
 ) {
   await checkReconfiguration();
   checkVersionRequirement();
   if (args[0] === "pandoc" && args[1] !== "help") {
-    await passThroughPandoc(args, env);
+    await passThroughPandoc(args);
   }
   if (args[0] === "typst") {
-    await passThroughTypst(args, env);
+    await passThroughTypst(args);
   }
 
   // passthrough to run handlers
   if (args[0] === "run" && args[1] !== "help" && args[1] !== "--help") {
-    const result = await runScript(args.slice(1), env);
+    const result = await runScript(args.slice(1));
     Deno.exit(result.code);
   }
 
@@ -153,13 +144,6 @@ export async function quarto(
 
   debug("Quarto version: " + quartoConfig.version());
 
-  const oldEnv: Record<string, string | undefined> = {};
-  for (const [key, value] of Object.entries(env || {})) {
-    const oldV = Deno.env.get(key);
-    oldEnv[key] = oldV;
-    Deno.env.set(key, value);
-  }
-
   const quartoCommand = new Command()
     .name("quarto")
     .help({ colors: false })
@@ -189,13 +173,6 @@ export async function quarto(
 
   try {
     await promise;
-    for (const [key, value] of Object.entries(oldEnv)) {
-      if (value === undefined) {
-        Deno.env.delete(key);
-      } else {
-        Deno.env.set(key, value);
-      }
-    }
     if (commandFailed()) {
       exitWithCleanup(1);
     }

tests/smoke/env/install.test.ts

@@ -13,6 +13,7 @@ testQuartoCmd(
   [
     noErrorsOrWarnings,
     printsMessage({level: "INFO", regex: /tinytex\s+/}),
+    // FIXME WE SHOULD BE ABLE TO TURN THIS BACK ON
     // printsMessage({level: "INFO", regex: /chromium\s+/}),
     // temporarily disabled until we get puppeteer back
   ],

tests/smoke/extensions/extension-render-journals.test.ts

@@ -42,6 +42,10 @@ for (const journalRepo of journalRepos) {
     // Sets up the test
     setup: async () => {
       console.log(`using quarto-journals/${journalRepo.repo}`);
+
+      // FIXME THIS DOESN'T WORK
+      // WE CANNOT GUARANTEE THAT CHDIR WILL BE CONSTANT
+      // THROUGHOUT THE ASYNC CALL
       const wd = Deno.cwd();
       Deno.chdir(workingDir);
       await quarto([

tests/smoke/project/project-prepost.test.ts

@@ -8,7 +8,7 @@ import { docs } from "../../utils.ts";
 
 import { join } from "../../../src/deno_ral/path.ts";
 import { existsSync } from "../../../src/deno_ral/fs.ts";
-import { testQuartoCmd } from "../../test.ts";
+import { testQuartoCmd, testQuartoCmdWithEnv } from "../../test.ts";
 import { fileExists, noErrors, printsMessage, verifyNoPath, verifyPath } from "../../verify.ts";
 import { normalizePath, safeRemoveIfExists } from "../../../src/core/path.ts";
 
@@ -78,22 +78,22 @@ testQuartoCmd(
     }
   });
 
-  testQuartoCmd(
-    "render",
-    [docs("project/prepost/issue-10828")],
-    [],
-    {
-      env: {
-        "QUARTO_USE_FILE_FOR_PROJECT_INPUT_FILES": normalizePath(docs("project/prepost/issue-10828/input-files.txt")),
-        "QUARTO_USE_FILE_FOR_PROJECT_OUTPUT_FILES": normalizePath(docs("project/prepost/issue-10828/output-files.txt"))
-      },
-      teardown: async () => {
-        const inputPath = normalizePath(docs("project/prepost/issue-10828/input-files.txt"));
-        const outputPath = normalizePath(docs("project/prepost/issue-10828/output-files.txt"));
-        verifyPath(inputPath);
-        safeRemoveIfExists(inputPath);
-        verifyPath(outputPath);
-        safeRemoveIfExists(outputPath);
-      }
+testQuartoCmdWithEnv(
+  "render",
+  [docs("project/prepost/issue-10828")],
+  [],
+  {
+    "QUARTO_USE_FILE_FOR_PROJECT_INPUT_FILES": normalizePath(docs("project/prepost/issue-10828/input-files.txt")),
+    "QUARTO_USE_FILE_FOR_PROJECT_OUTPUT_FILES": normalizePath(docs("project/prepost/issue-10828/output-files.txt"))
+  },
+  {
+    teardown: async () => {
+      const inputPath = normalizePath(docs("project/prepost/issue-10828/input-files.txt"));
+      const outputPath = normalizePath(docs("project/prepost/issue-10828/output-files.txt"));
+      verifyPath(inputPath);
+      safeRemoveIfExists(inputPath);
+      verifyPath(outputPath);
+      safeRemoveIfExists(outputPath);
     }
-  )
+  }
+)

tests/test.ts

@@ -17,6 +17,7 @@ import { runningInCI } from "../src/core/ci-info.ts";
 import { relative, fromFileUrl } from "../src/deno_ral/path.ts";
 import { quartoConfig } from "../src/core/quarto.ts";
 import { isWindows } from "../src/deno_ral/platform.ts";
+import { execProcess } from "../src/core/process.ts";
 
 export interface TestDescriptor {
   // The name of the test
@@ -55,9 +56,6 @@ export interface TestContext {
 
   // control if test is ran or skipped
   ignore?: boolean;
-
-  // environment to pass to downstream processes
-  env?: Record<string, string>;
 }
 
 // Allow to merge test contexts in Tests helpers
@@ -95,11 +93,44 @@ export function mergeTestContexts(baseContext: TestContext, additionalContext?:
     },
     // override ignore if provided
     ignore: additionalContext.ignore ?? baseContext.ignore,
-    // merge env with additional context taking precedence
-    env: { ...baseContext.env, ...additionalContext.env },
   };
 }
 
+// for Quarto tests that new to change the environment,
+// we use a subprocess call to run the command, or else we risk
+// race conditions with environment variables overwriting
+// each other
+export function testQuartoCmdWithEnv(
+  cmd: string,
+  args: string[],
+  verify: Verify[],
+  env: Record<string, string>,
+  context?: TestContext,
+  name?: string
+) {
+  if (name === undefined) {
+    name = `quarto ${cmd} ${args.join(" ")}`;
+  }
+  test({
+    name,
+    execute: async () => {
+      const timeout = new Promise((_resolve, reject) => {
+        setTimeout(reject, 600000, "timed out after 10 minutes");
+      });
+      await Promise.race([
+        execProcess({
+          cmd: [quartoConfig.binPath(), cmd, ...args],
+          env
+        }),
+        timeout,
+      ]);
+    },
+    verify,
+    context: context || {},
+    type: "smoke",
+  });
+}
+
 export function testQuartoCmd(
   cmd: string,
   args: string[],
@@ -117,7 +148,7 @@ export function testQuartoCmd(
         setTimeout(reject, 600000, "timed out after 10 minutes");
       });
       await Promise.race([
-        quarto([cmd, ...args], undefined, context?.env),
+        quarto([cmd, ...args]),
         timeout,
       ]);
     },