Do not use core.exportVariable in unit tests - #3930
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
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.
| File | Description |
|---|---|
| src/environment.ts | Adds isInTestMode and the exportVariable wrapper around core.exportVariable. |
| src/util.ts | Switches env-var exports to the wrapper and re-exports isInTestMode. |
| src/upload-lib.ts | Uses wrapper for sentinel env vars and deprecation warning env var. |
| src/status-report.ts | Uses wrapper for job-status/workflow-start/test-environment env vars. |
| src/setup-codeql-action.ts | Uses wrapper for job UUID and “has run” env vars. |
| src/init.ts | Uses wrapper for the file-coverage deprecation sentinel env var. |
| src/init-action.ts | Uses wrapper for job UUID, feature toggles, and forwarded env vars. |
| src/init-action-post.ts | Uses wrapper to set final JOB_STATUS. |
| src/autobuild.ts | Uses wrapper for autobuild feature env vars and Go autobuild sentinel. |
| src/autobuild-action.ts | Uses wrapper for autobuild success sentinel. |
| src/api-client.ts | Uses wrapper for persisting computed ANALYSIS_KEY. |
| src/analyze-action.ts | Uses wrapper for SARIF output dir and analyze success sentinel. |
| src/debug-artifacts.ts | Uses wrapper for artifact scan sentinel in test mode. |
| src/codeql.ts | Uses wrapper for suppressing “deprecated soon” warning env var. |
| src/config-utils.ts | Uses wrapper for TRAP caching env var. |
| src/overlay/caching.test.ts | Updates stubbing to target environment.isInTestMode (instead of util). |
| src/init.test.ts | Updates stubbing to target environment.exportVariable (instead of core.exportVariable). |
| eslint.config.mjs | Adds a no-restricted-syntax rule to ban core.exportVariable usage outside src/environment.ts. |
| lib/entry-points.js | Generated build output reflecting the TypeScript changes (not reviewed). |
Copilot's findings
- Files reviewed: 18/19 changed files
- Comments generated: 1
Sorry, something went wrong.
| * 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") { |
There was a problem hiding this comment.
Shouldn't this just use the isInTestMode() function declared earlier? why the difference?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
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:
Which use cases does this change impact?
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist