Skip to content

Do not use core.exportVariable in unit tests - #3930

Open
mbg wants to merge 4 commits into
mainfrom
mbg/tests/fix-export-variable
Open

Do not use core.exportVariable in unit tests#3930
mbg wants to merge 4 commits into
mainfrom
mbg/tests/fix-export-variable

Conversation

@mbg

@mbgmbg commented May 22, 2026

Copy link
Copy Markdown
Member

Although we already clear environment variables that are set during unit tests, that does not affect the behaviour of core.exportVariable which additionally sets environment variables for subsequent steps in a workflow. If the unit tests are run in CI, then core.exportVariable sets environment variables for subsequent steps in the workflow job which can interfere with them.

This PR improves the situation by introducing a wrapper around core.exportVariable which does not call core.exportVariable when NODE_ENV is test (which is set automatically by ava).

Risk assessment

For internal use only. Please select the risk level of this change:

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

Environments:

  • Testing/None - This change does not impact any CodeQL workflows in production.

How did/will you validate this change?

  • Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).
  • End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

  • Development/testing only - This change cannot cause any failures in production.

How will you know if something goes wrong after this change is released?

  • Telemetry - I rely on existing telemetry or have made changes to the telemetry.

Are there any special considerations for merging or releasing this change?

  • No special considerations - This change can be merged at any time.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

@mbgmbg self-assigned this May 22, 2026
CopilotAI review requested due to automatic review settings May 22, 2026 14:04
@mbg
mbg requested a review from a team as a code ownerMay 22, 2026 14:04
@github-actionsgithub-actionsBot added the size/M Should be of average difficulty to review label May 22, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a CodeQL Action–local wrapper for setting environment variables so unit tests avoid calling @actions/core.exportVariable, which can leak environment changes to subsequent CI steps. It then migrates internal calls over to the wrapper and adds an ESLint guardrail to prevent reintroducing direct core.exportVariable usage.

Changes:

  • Added exportVariable() wrapper in src/environment.ts and moved isInTestMode() there (with util re-export for compatibility).
  • Replaced many core.exportVariable(...) call sites across the action with the new wrapper.
  • Added an ESLint rule to prohibit core.exportVariable outside src/environment.ts, and updated affected unit tests to stub the wrapper.
Show a summary per file
FileDescription
src/environment.tsAdds isInTestMode and the exportVariable wrapper around core.exportVariable.
src/util.tsSwitches env-var exports to the wrapper and re-exports isInTestMode.
src/upload-lib.tsUses wrapper for sentinel env vars and deprecation warning env var.
src/status-report.tsUses wrapper for job-status/workflow-start/test-environment env vars.
src/setup-codeql-action.tsUses wrapper for job UUID and “has run” env vars.
src/init.tsUses wrapper for the file-coverage deprecation sentinel env var.
src/init-action.tsUses wrapper for job UUID, feature toggles, and forwarded env vars.
src/init-action-post.tsUses wrapper to set final JOB_STATUS.
src/autobuild.tsUses wrapper for autobuild feature env vars and Go autobuild sentinel.
src/autobuild-action.tsUses wrapper for autobuild success sentinel.
src/api-client.tsUses wrapper for persisting computed ANALYSIS_KEY.
src/analyze-action.tsUses wrapper for SARIF output dir and analyze success sentinel.
src/debug-artifacts.tsUses wrapper for artifact scan sentinel in test mode.
src/codeql.tsUses wrapper for suppressing “deprecated soon” warning env var.
src/config-utils.tsUses wrapper for TRAP caching env var.
src/overlay/caching.test.tsUpdates stubbing to target environment.isInTestMode (instead of util).
src/init.test.tsUpdates stubbing to target environment.exportVariable (instead of core.exportVariable).
eslint.config.mjsAdds a no-restricted-syntax rule to ban core.exportVariable usage outside src/environment.ts.
lib/entry-points.jsGenerated build output reflecting the TypeScript changes (not reviewed).

Copilot's findings

  • Files reviewed: 18/19 changed files
  • Comments generated: 1

Comment threadsrc/environment.ts
Comment threadsrc/environment.ts Fixed
Comment threadsrc/environment.ts
* sets environment variables for other steps in a workflow when we run unit tests in CI.
*/
export function exportVariable(name: string, val: any): void {
if (process.env["NODE_ENV"] === "test") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this just use the isInTestMode() function declared earlier? why the difference?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isInTestMode is based on the CODEQL_ACTION_TEST_MODE environment variable, which we primarily set for e2e tests. Historically, it was only about whether or not we should upload SARIF results, but has been used for some other purposes in that context.

The CODEQL_ACTION_TEST_MODE environment variable is not set for the unit tests. I did initially have a version of exportVariable that was based on isInTestMode before I noticed that. However, rather than trying to decide if it would be OK to set CODEQL_ACTION_TEST_MODE for the unit tests or not (since it changes the behaviour), I figured that testing whether we are running the unit tests or not is a safer approach that involves less potential for change.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/MShould be of average difficulty to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@mbg@oscarsj@github-advanced-security