Repository navigation
test: set and restore process environment through a shared helper - #3448
Merged
Merged
Conversation
Tests that change process.env for one test or one call each carried their own save/restore: read the old value, assign, then delete or put it back in a t.after hook or a finally block. Three ci-openshell tests and two other files also put a fixture directory first on PATH by hand instead of using prependPath. tests/helpers/environment.mjs adds setEnvironment(t, values), restored when the test ends, and withEnvironment(values, body), restored when body settles. An entry that was unset is unset again; several calls in one test are undone in reverse order. prependPath now uses setEnvironment. The hand-written copies in 11 files switch over with the same values and the same scope.
Open PR #2633 edits the line right after it, so converting it would add a merge conflict there. The other two sites keep using prependPath.
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
Review nit: every call for one test is undone in the after hook the first call registered, so a later call is undone before hooks registered between the two.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Tests that change
process.envfor one test or one call each carry their own save/restore: read the old value, assign, then delete it or put it back in at.afterhook or afinallyblock. The same five-lineif (previous === undefined) delete ... else ...block appears about 20 times in 10 files. Two ci-openshell tests,first-agent-smokeandkubernetes-real-kubectlalso put a fixture directory first onPATHby hand instead of usingprependPath. Those copies restore withprocess.env.PATH = path, which writes the string"undefined"whenPATHwas unset.Change
tests/helpers/environment.mjs:setEnvironment(t, values)sets the entries until the test that ownstends.withEnvironment(values, body)sets them whilebodyruns and restores them when it settles.undefinedunsets an entry. SeveralsetEnvironmentcalls in one test are undone in reverse order, so an entry set twice (asruntime-image-failure-output'sdockerFailuredoes) ends as it was before the first call.prependPathinfake-commands.mjsnow usessetEnvironment.setEnvironment, andfinally-scoped copies usewithEnvironment.container-releasecontainer-resumegithub-app-observerpostgres-connection-authrepository-client-environmentsocket-directoryruntime-image-failure-outputci-openshell(2 of 3 PATH sites, plusEARLY_WAKE_GRACE_MS)first-agent-smokekubernetes-real-kubectlfake-commandsdocker-compute,ssh-computeanddocker-compute-token-retry, because feat(occdev): wire the OpenShell Git route into occ dev up with an installed acceptance #3045 rewrites them.ci-openshellPATH site, because feat(runtime): apply Gateway tool policy to dedicated OCE deployments #2633 edits the next line.ci-impact's whole-environment swap.Verification
Every touched file passes locally on Node 24, pinned to 10 CPUs.
repository-client-environment1,socket-directory2,kubernetes-real-kubectl47,ci-openshell14,container-release22,container-resume1,first-agent-smoke35,github-app-observer13,postgres-connection-auth6,runtime-image-failure-output12.postgres-connection-authskips 2, the cases that need a real database, which don't use the helper. Every converted site ran.prependPathcallers also pass:kubernetes-api-read4 andk3d-failure-diagnostics32.Mutants:
setEnvironmentsets nothingrepository-client-environment1/1,container-release5/22,container-resume1/1,runtime-image-failure-output9/12,ci-openshell3/14,postgres-connection-auth1/6,kubernetes-api-read2/4,k3d-failure-diagnostics1/32;kubernetes-real-kubectlhangswithEnvironmentsets nothingsocket-directory1/2,kubernetes-real-kubectl3/47,ci-openshell1/14,first-agent-smoke1/35,postgres-connection-auth1/6tsc --build tsconfig.json tests/tsconfig.json, ESLint and Prettier are clean on the touched files.A
git merge-treecheck against open PRs that touch these files (test(openshell): exercise native provider authority #1982, Draft implementation: managed outbound HTTPS for dedicated Gateways #2518, feat(runtime): apply Gateway tool policy to dedicated OCE deployments #2633, Type-check maintained Node scripts and tests in the root build #2925) finds no new conflicts.Risks
Test-only. Restores now run at the same point or, for the
container-releaseandcontainer-resumecases, one after-hook later than the temporary-directory removal they shared a hook with.