fix(tests): centralize test config dir lifecycle to prevent env var pollution#242
Merged
fix(tests): centralize test config dir lifecycle to prevent env var pollution#242
Conversation
…ollution Bun runs test files sequentially in a single thread. Several test files unconditionally deleted process.env.SENTRY_CONFIG_DIR in afterEach hooks, causing the next file's module-level code to read undefined and crash with TypeError. A runner image upgrade changed file discovery order, exposing this latent bug. Introduces useTestConfigDir() helper in test/helpers.ts that creates a unique temp directory in beforeEach and restores (never deletes) the env var in afterEach. Migrates all 11 affected test files to use it.
Contributor
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛Telemetry
Other
🤖 This preview updates automatically when you update the PR. |
The test preload deletes process.env.SENTRY_CLIENT_ID before oauth.ts loads, causing the module-level constant to capture an empty string. Tests that later set SENTRY_CLIENT_ID in beforeEach had no effect since the constant was already captured. This caused refreshAccessToken to throw ConfigError (not AuthError), which the ky afterResponse hook caught silently, preventing 401 retry from working. Replace the module-level constant with a getClientId() function that reads process.env at call time.
The tests mocked globalThis.fetch for /organizations/ and /projects/ endpoints but not /users/me/regions/. When getUserRegions() got a 404 from the mock, ky threw an HTTPError which listOrganizations caught and fell back to the non-region path. On certain CI runner images, this error-fallback code path fails. Adding an explicit regions mock avoids the error path entirely and makes the tests deterministic.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // (or beforeEach hooks) may read the env var and get undefined. | ||
| if (savedConfigDir !== undefined) { | ||
| process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; | ||
| } |
There was a problem hiding this comment.
Config dir not restored when originally unset
Low Severity
useTestConfigDir() only restores process.env[CONFIG_DIR_ENV_VAR] when savedConfigDir !== undefined. If the original value was unset, afterEach leaves SENTRY_CONFIG_DIR pointing to the just-cleaned temp directory, so later tests can inherit a stale, invalid config path.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Tests in resolver.test.ts and errors.test.ts used mock.module() to mock api-client.js, which persists across test files in Bun's single-threaded runner. When these files ran before utils.test.ts, the module mock intercepted listOrganizations/listProjects calls, returning stale mock data instead of going through the test's globalThis.fetch mock. Move these files to test/isolated/dsn/ so they run in the separate test:isolated script, matching the pattern used by resolve-target.test.ts. Also removes debug logging added in previous commits.
Contributor
Codecov Results 📊✅ Patch coverage is 85.71%. Project has 4061 uncovered lines. Files with missing lines (68)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 70.11% 68.74% -1.37%
==========================================
Files 105 105 —
Lines 12918 12992 +74
Branches 0 0 —
==========================================
+ Hits 9057 8931 -126
- Misses 3861 4061 +200
- Partials 0 0 —Generated by Codecov Action |
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
1 participant
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.
Summary
Fixes 37 test failures caused by the Ubuntu 24.04 runner image upgrade (
20260201.15.1→20260209.23.1, kernel 6.11 → 6.14) that changedreaddirordering, exposing a latent env var pollution bug.Root Cause
Bun runs test files sequentially in one thread (load → run all tests → load next file). Several test files unconditionally
delete process.env.SENTRY_CONFIG_DIRin theirafterEachhooks. When the next file loads, its module-level code capturesundefined, causingjoin(undefined, ...)→TypeError.The runner image upgrade changed the kernel which changed file discovery order, putting "deleting" files before "capturing" files.
Changes
test/helpers.ts: AddeduseTestConfigDir()helper that creates a unique temp directory inbeforeEach, pointsSENTRY_CONFIG_DIRat it, and restores (never deletes) the original value inafterEach. Also handlescloseDatabase()and temp dir cleanup.useTestConfigDir(), eliminating:SENTRY_CONFIG_DIR(root cause)process.env[CONFIG_DIR_ENV_VAR]!capturesif (original) restore else deletepatterns (off-by-one on falsy check)afterEachAGENTS.md: Added "Test Environment Isolation" section documenting the pattern and anti-patternsNet: -87 lines (157 added, 244 removed) — less boilerplate, more safety.
Verification
bun run typecheck— passesbun run lint— passesbun run test:unit— 1545 tests pass, 0 failuresSupercedes #240