Do not use core.exportVariable in unit tests (2nd attempt)

eslint.config.mjs

@@ -140,6 +140,18 @@ export default [
       "no-async-foreach/no-async-foreach": "error",
       "no-sequences": "error",
       "no-shadow": "off",
+
+      // A basic check that we don't use `exportVariable` from `@actions/core`. This rule depends on
+      // the module being imported as `core`, but that is a good enough check for us.
+      "no-restricted-syntax": [
+        "error",
+        {
+          selector:
+            "MemberExpression[object.name='core'][property.name='exportVariable']",
+          message: "Use `exportVariable` from `environment.ts` instead.",
+        },
+      ],
+
       // This is overly restrictive with unsetting `EnvVar`s
       "@typescript-eslint/no-dynamic-delete": "off",
       "@typescript-eslint/no-shadow": "error",
@@ -157,6 +169,15 @@ export default [
       ],
     },
   },
+  {
+    files: ["src/environment.ts"],
+
+    // We allow `exportVariable` from `@actions/core` to be used in this file
+    // since it defines the wrapper around it that other modules use.
+    rules: {
+      "no-restricted-syntax": "off",
+    },
+  },
   {
     files: ["**/*.ts", "**/*.js"],
 

queries/default-setup-environment-variables.ql

@@ -23,7 +23,8 @@ predicate isSafeForDefaultSetup(string envVar) {
       "GITHUB_BASE_REF", "GITHUB_EVENT_NAME", "GITHUB_JOB", "GITHUB_RUN_ATTEMPT", "GITHUB_RUN_ID",
       "GITHUB_SHA", "GITHUB_REPOSITORY", "GITHUB_SERVER_URL", "GITHUB_TOKEN", "GITHUB_WORKFLOW",
       "GITHUB_WORKSPACE", "GOFLAGS", "ImageVersion", "JAVA_TOOL_OPTIONS", "RUNNER_ARCH",
-      "RUNNER_ENVIRONMENT", "RUNNER_NAME", "RUNNER_OS", "RUNNER_TEMP", "RUNNER_TOOL_CACHE"
+      "RUNNER_ENVIRONMENT", "RUNNER_NAME", "RUNNER_OS", "RUNNER_TEMP", "RUNNER_TOOL_CACHE",
+      "NODE_ENV"
     ]
 }
 

src/actions-util.ts

@@ -30,7 +30,6 @@ declare const __CODEQL_ACTION_VERSION__: string;
 export interface ActionsEnv {
   getRequiredInput: (name: string) => string;
   getOptionalInput: (name: string) => string | undefined;
-  exportVariable: (name: string, value: string) => void;
 }
 
 /**
@@ -40,7 +39,6 @@ export function getActionsEnv(): ActionsEnv {
   return {
     getRequiredInput,
     getOptionalInput,
-    exportVariable: core.exportVariable,
   };
 }
 

src/analyze-action.ts

@@ -214,9 +214,10 @@ async function runAutobuildIfLegacyGoWorkflow(config: Config, logger: Logger) {
 
 async function run({
   startedAt,
+  env,
   logger,
   actions,
-}: ActionState<["Base", "Logger", "Actions"]>) {
+}: ActionState<["Base", "Env", "Logger", "Actions"]>) {
   // To capture errors appropriately, keep as much code within the try-catch as
   // possible, and only use safe functions outside.
 
@@ -287,7 +288,7 @@ async function run({
 
     const apiDetails = getApiDetails();
     const outputDir = actionsUtil.getRequiredInput("output");
-    core.exportVariable(EnvVar.SARIF_RESULTS_OUTPUT_DIR, outputDir);
+    env.export(EnvVar.SARIF_RESULTS_OUTPUT_DIR, outputDir);
     const threads = util.getThreadsFlag(
       actionsUtil.getOptionalInput("threads") || process.env["CODEQL_THREADS"],
       logger,
@@ -456,7 +457,7 @@ async function run({
         `expect-error input was set to true but no error was thrown.`,
       );
     }
-    core.exportVariable(EnvVar.ANALYZE_DID_COMPLETE_SUCCESSFULLY, "true");
+    env.export(EnvVar.ANALYZE_DID_COMPLETE_SUCCESSFULLY, "true");
   } catch (unwrappedError) {
     const error = util.wrapError(unwrappedError);
     if (

src/api-client.ts

@@ -17,6 +17,7 @@ import {
   ReadOnlyEnv,
   RegistryProxyVars,
   getEnv,
+  exportVariable,
 } from "./environment";
 import { Logger } from "./logging";
 import { getRepositoryNwo, RepositoryNwo } from "./repository";
@@ -316,7 +317,7 @@ export async function getAnalysisKey(): Promise<string> {
   const jobName = getRequiredEnvParam("GITHUB_JOB");
 
   analysisKey = `${workflowPath}:${jobName}`;
-  core.exportVariable(EnvVar.ANALYSIS_KEY, analysisKey);
+  exportVariable(EnvVar.ANALYSIS_KEY, analysisKey);
   return analysisKey;
 }
 

src/autobuild-action.ts

@@ -68,7 +68,11 @@ async function sendCompletedStatusReport(
   }
 }
 
-async function run({ startedAt, logger }: ActionState<["Base", "Logger"]>) {
+async function run({
+  startedAt,
+  env,
+  logger,
+}: ActionState<["Base", "Env", "Logger"]>) {
   // To capture errors appropriately, keep as much code within the try-catch as
   // possible, and only use safe functions outside.
 
@@ -135,7 +139,7 @@ async function run({ startedAt, logger }: ActionState<["Base", "Logger"]>) {
     return;
   }
 
-  core.exportVariable(EnvVar.AUTOBUILD_DID_COMPLETE_SUCCESSFULLY, "true");
+  env.export(EnvVar.AUTOBUILD_DID_COMPLETE_SUCCESSFULLY, "true");
 
   await sendCompletedStatusReport(config, logger, startedAt, languages ?? []);
 }

src/autobuild.ts

@@ -1,11 +1,9 @@
-import * as core from "@actions/core";
-
 import { getTemporaryDirectory, getWorkflowEventName } from "./actions-util";
 import { getGitHubVersion } from "./api-client";
 import { CodeQL, getCodeQL } from "./codeql";
 import * as configUtils from "./config-utils";
 import { DocUrl } from "./doc-url";
-import { ActionsEnvVars, EnvVar } from "./environment";
+import { ActionsEnvVars, EnvVar, exportVariable } from "./environment";
 import { Feature, featureConfig, initFeatures } from "./feature-flags";
 import { BuiltInLanguage, Language } from "./languages";
 import { Logger } from "./logging";
@@ -136,16 +134,16 @@ export async function setupCppAutobuild(codeql: CodeQL, logger: Logger) {
             : ""
         }`,
       );
-      core.exportVariable(envVar, "false");
+      exportVariable(envVar, "false");
     } else {
       logger.info(
         `Enabling ${featureName}. This can be disabled by setting the ${envVar} environment variable to 'false'. See ${DocUrl.DEFINE_ENV_VARIABLES} for more information.`,
       );
-      core.exportVariable(envVar, "true");
+      exportVariable(envVar, "true");
     }
   } else {
     logger.info(`Disabling ${featureName}.`);
-    core.exportVariable(envVar, "false");
+    exportVariable(envVar, "false");
   }
 }
 
@@ -165,7 +163,7 @@ export async function runAutobuild(
     await codeQL.runAutobuild(config, language);
   }
   if (language === BuiltInLanguage.go) {
-    core.exportVariable(EnvVar.DID_AUTOBUILD_GOLANG, "true");
+    exportVariable(EnvVar.DID_AUTOBUILD_GOLANG, "true");
   }
   logger.endGroup();
 }

src/codeql.ts

@@ -17,7 +17,7 @@ import type { VersionInfo } from "./cli/types";
 import { CliError, wrapCliConfigurationError } from "./cli-errors";
 import { appendExtraQueryExclusions, type Config } from "./config-utils";
 import { DocUrl } from "./doc-url";
-import { EnvVar, getEnv } from "./environment";
+import { EnvVar, getEnv, exportVariable } from "./environment";
 import {
   CodeQLDefaultVersionInfo,
   Feature,
@@ -1054,7 +1054,7 @@ async function getCodeQLForCmd(
         }' by 'github/codeql-action/*@v${getActionVersion()}' in your code scanning workflow to ` +
         "continue using this version of the CodeQL Action.",
     );
-    core.exportVariable(EnvVar.SUPPRESS_DEPRECATED_SOON_WARNING, "true");
+    exportVariable(EnvVar.SUPPRESS_DEPRECATED_SOON_WARNING, "true");
   }
   return codeql;
 }

src/config-utils.ts

@@ -2,7 +2,6 @@ import * as fs from "fs";
 import * as path from "path";
 import { performance } from "perf_hooks";
 
-import * as core from "@actions/core";
 import * as yaml from "js-yaml";
 
 import { ActionState } from "./action-common";
@@ -46,7 +45,7 @@ import {
   makeTelemetryDiagnostic,
 } from "./diagnostics";
 import { prepareDiffInformedAnalysis } from "./diff-informed-analysis-utils";
-import { EnvVar } from "./environment";
+import { EnvVar, exportVariable } from "./environment";
 import * as errorMessages from "./error-messages";
 import { Feature, FeatureEnablement, FeatureWithoutCLI } from "./feature-flags";
 import {
@@ -965,10 +964,10 @@ async function setCppTrapCachingEnvironmentVariables(
       );
     } else if (config.trapCaches[BuiltInLanguage.cpp]) {
       logger.info("Enabling TRAP caching for C/C++.");
-      core.exportVariable(envVar, "true");
+      exportVariable(envVar, "true");
     } else {
       logger.debug(`Disabling TRAP caching for C/C++.`);
-      core.exportVariable(envVar, "false");
+      exportVariable(envVar, "false");
     }
   }
 }

src/debug-artifacts.ts

@@ -11,7 +11,7 @@ import { dbIsFinalized } from "./analyze";
 import { scanArtifactsForTokens } from "./artifact-scanner";
 import { type CodeQL } from "./codeql";
 import { Config } from "./config-utils";
-import { EnvVar } from "./environment";
+import { EnvVar, exportVariable } from "./environment";
 import * as json from "./json";
 import { Language } from "./languages";
 import { Logger, withGroup } from "./logging";
@@ -330,7 +330,7 @@ export async function uploadArtifacts(
   // some issues early.
   if (isInTestMode()) {
     await scanArtifactsForTokens(toUpload, logger);
-    core.exportVariable("CODEQL_ACTION_ARTIFACT_SCAN_FINISHED", "true");
+    exportVariable("CODEQL_ACTION_ARTIFACT_SCAN_FINISHED", "true");
   }
 
   const suffix = getArtifactSuffix(getOptionalInput("matrix"));

src/environment.ts

@@ -1,3 +1,5 @@
+import * as core from "@actions/core";
+
 /**
  * Environment variables used by Default Setup to communicate the private registry proxy configuration.
  */
@@ -211,8 +213,20 @@ export enum ActionsEnvVars {
   RUNNER_TOOL_CACHE = "RUNNER_TOOL_CACHE",
 }
 
+/** Environment variables which are not specific to CodeQL. */
+export enum SystemEnvVar {
+  /**
+   * Used by Node and related tools to indicate what kind of environment we are running in.
+   */
+  NODE_ENV = "NODE_ENV",
+}
+
 /** A type representing all known environment variables. */
-export type KnownEnvVar = EnvVar | ActionsEnvVars | RegistryProxyVars;
+export type KnownEnvVar =
+  | EnvVar
+  | ActionsEnvVars
+  | RegistryProxyVars
+  | SystemEnvVar;
 
 /**
  * Gets an environment variable, but throws an error if it is not set.
@@ -289,6 +303,26 @@ export class ReadOnlyEnv<T extends string | undefined = string | undefined> {
   public entries(): Array<[string, T]> {
     return Object.entries(this.vars);
   }
+
+  /**
+   * Gets a value indicating whether we should skip uploads of
+   * all kinds (SARIF results, status reports, DBs, ...).
+   *
+   * This is not guaranteed to be set in all test environments.
+   */
+  public isSkippingUploadsInTests(): boolean {
+    return this.getOptional(EnvVar.TEST_MODE) === "true";
+  }
+
+  /**
+   * Gets a value indicative of whether we are in a testing environment
+   * by testing whether the value of the `NODE_ENV` variable is "test".
+   * This is expected to be the case if e.g. `ava` is running the tests
+   * or if this instance was constructed by `getTestEnv`.
+   */
+  public isTestingEnv(): boolean {
+    return this.getOptional(SystemEnvVar.NODE_ENV) === "test";
+  }
 }
 
 /**
@@ -307,6 +341,26 @@ export class Env<
     this.changed = true;
   }
 
+  /**
+   * Wrapper around `core.exportVariable` which does not call `core.exportVariable`
+   * when running unit tests. This is important, because otherwise `core.exportVariable`
+   * sets environment variables for other steps in a workflow when we run unit tests in CI.
+   *
+   * @param name The name of the environment variable to set and export.
+   * @param val The value to set and export for the environment variable.
+   */
+  public export(name: string, val: T): void {
+    // Setting the environment variable for this instance is always OK, including
+    // in tests, since we use fresh `Env` instances whenever needed. This allows
+    // tests to pass that rely on that part of the `core.exportVariable` behaviour.
+    this.set(name, val);
+
+    // Call `core.exportVariable` whenever we are not in a test environment.
+    if (!this.isTestingEnv()) {
+      core.exportVariable(name, val);
+    }
+  }
+
   /** Gets a value indicating whether `set` was called at least once. */
   public hasChanged(): boolean {
     return this.changed;
@@ -317,3 +371,34 @@ export class Env<
 export function getEnv(env: NodeJS.ProcessEnv = process.env): Env {
   return new Env(env);
 }
+
+/**
+ * Returns whether we are in test mode. This is used by CodeQL Action PR checks.
+ *
+ * In test mode, we skip several uploads (SARIF results, status reports, DBs, ...).
+ *
+ * @deprecated
+ *  The purpose of this function is ambiguous. Use `isSkippingUploadsInTests` on
+ *  a `ReadOnlyEnv` instance instead for equivalent behaviour. Use `isTestingEnv`
+ *  to determine if we are running in a unit test.
+ */
+export function isInTestMode(): boolean {
+  return getEnv().isSkippingUploadsInTests();
+}
+
+/**
+ * Wrapper around `core.exportVariable` which does not call `core.exportVariable`
+ * when running unit tests. This is important, because otherwise `core.exportVariable`
+ * sets environment variables for other steps in a workflow when we run unit tests in CI.
+ *
+ * @deprecated Use `export` on an `Env` instance instead.
+ */
+export function exportVariable(name: string, val: any): void {
+  const env = getEnv();
+
+  if (typeof val === "string") {
+    env.export(name, val);
+  } else {
+    env.export(name, JSON.stringify(val));
+  }
+}

src/init-action-post.ts

@@ -21,7 +21,7 @@ import {
   DependencyCachingUsageReport,
   getDependencyCacheUsage,
 } from "./dependency-caching";
-import { EnvVar, getEnv } from "./environment";
+import { EnvVar, getEnv, exportVariable } from "./environment";
 import { initFeatures } from "./feature-flags";
 import * as gitUtils from "./git-utils";
 import * as initActionPostHelper from "./init-action-post-helper";
@@ -166,7 +166,7 @@ function getFinalJobStatus(config: Config | undefined): JobStatus {
   let jobStatus: JobStatus;
 
   if (process.env[EnvVar.ANALYZE_DID_COMPLETE_SUCCESSFULLY] === "true") {
-    core.exportVariable(EnvVar.JOB_STATUS, JobStatus.SuccessStatus);
+    exportVariable(EnvVar.JOB_STATUS, JobStatus.SuccessStatus);
     jobStatus = JobStatus.SuccessStatus;
   } else if (config !== undefined) {
     // - We have computed a CodeQL config
@@ -191,7 +191,7 @@ function getFinalJobStatus(config: Config | undefined): JobStatus {
 
   // This shouldn't be necessary, but in the odd case that we run more than one
   // `init` post step, ensure the job status is consistent between them.
-  core.exportVariable(EnvVar.JOB_STATUS, jobStatus);
+  exportVariable(EnvVar.JOB_STATUS, jobStatus);
   return jobStatus;
 }