Do not use core.exportVariable in unit tests (2nd attempt) - #4212
mbg merged 7 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
🟡 Changes recommended
NODE_ENV-based detection can suppress cross-step exports in real workflows that set NODE_ENV=test.
1 open finding What changed in this PRIntroduces an environment abstraction that prevents unit tests from exporting variables to subsequent workflow steps.
Changes:
- Adds Env.export and test-environment detection.
- Migrates environment exports away from core.exportVariable.
- Updates tests, linting, and CodeQL query configuration.
| File | Description |
|---|---|
| src/environment.ts | Adds environment export and test-mode abstractions. |
| src/testing-utils.ts | Configures isolated test environments. |
| src/actions-util.ts | Removes exporting from ActionsEnv. |
| src/util.ts | Uses the environment export wrapper. |
| src/upload-lib.ts | Migrates SARIF-related exports. |
| src/status-report.ts | Migrates status and UUID exports. |
| src/setup-codeql.ts | Migrates setup-state export. |
| src/setup-codeql-action.ts | Uses action-state environment exporting. |
| src/init.ts | Migrates deprecation-warning export. |
| src/init-action.ts | Uses action-state environment access and exporting. |
| src/init-action-post.ts | Migrates final-status exports. |
| src/debug-artifacts.ts | Migrates artifact-scan export. |
| src/config-utils.ts | Migrates TRAP-cache exports. |
| src/codeql.ts | Migrates warning-suppression export. |
| src/autobuild.ts | Migrates autobuild exports. |
| src/autobuild-action.ts | Uses action-state environment exporting. |
| src/api-client.ts | Migrates analysis-key export. |
| src/analyze-action.ts | Uses action-state environment exporting. |
| src/overlay/caching.test.ts | Updates the test-mode stub location. |
| src/init.test.ts | Updates environment export stubs. |
| queries/default-setup-environment-variables.ql | Allows NODE_ENV access. |
| eslint.config.mjs | Prohibits direct core.exportVariable use. |
| lib/entry-points.js | Generated bundle update; excluded from review. |
- lib/entry-points.js
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Sorry, something went wrong.
| /** | ||
| * Used by Node and related tools to indicate what kind of environment we are running in. | ||
| */ | ||
| NODE_ENV = "NODE_ENV", | ||
| } |
There was a problem hiding this comment.
I think this can now be cleaned up
Sorry, something went wrong.
| * 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`. |
There was a problem hiding this comment.
Update this JSDoc
Sorry, something went wrong.
There was a problem hiding this comment.
I was worried I might have missed one
Sorry, something went wrong.
|
@henrymercer I'll merge this as-is for now since it's changing quite a few places and I want to avoid merge conflicts arising if we let too much time pass. I'll address your feedback separately. |
Sorry, something went wrong.
This PR revives #3930 as per the discussion at #4198 (comment). This PR aims to accomplish the same goals as #3930, but is updated based on changes that have happened in the codebase since. The original PR description follows.
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).
The main change compared to the previous PR is that we now have ReadOnlyEnv and Env and it makes sense to integrate the logic of our exportVariable wrapper with those.
At first glance, a potential concern with using the export method of the Env class is that, if NODE_ENV is test, it only stores the environment variable in the Env instance. Other functions which may depend on the environment variable, but still use process.env to access it, do not see the value. However, all of the unit tests pass, and so we can rule out that this is an issue affecting the tests.
Outside of unit tests, where NODE_ENV is not expected to be set to test, the implementation calls core.exportVariable as well, which does set the environment variable for the process.
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