refactor(env): add env parameter injection for testability #542
Conversation
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
- Add optional env parameter to getEnvVars() and getEnvOptions() in @pgpmjs/env - Add optional env parameter to getGraphQLEnvVars() and getEnvOptions() in @constructive-io/graphql-env - Refactor tests to use env injection instead of fixture-based approach - Delete __fixtures__ directories and test-utils.ts (no longer needed) This enables deterministic testing without mutating process.env and eliminates the maintenance burden of keeping env.keys.json in sync with the actual env vars read by the code.
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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.
refactor(env): add env parameter injection for testability
Summary
Adds an optional
envparameter to environment parsing functions in@pgpmjs/envand@constructive-io/graphql-env, enabling dependency injection for testing. This eliminates the need for fixture files and the complex snapshot/restore pattern that was previously used to isolate tests from the host environment.Changes:
getEnvVars(env?)andgetEnvOptions(overrides?, cwd?, env?)in@pgpmjs/envgetGraphQLEnvVars(env?)andgetEnvOptions(overrides?, cwd?, env?)in@constructive-io/graphql-envprocess.env__fixtures__/directories andtest-utils.ts(no longer needed)The API is backwards compatible - all new parameters have defaults (
process.env).Review & Testing Checklist for Human
"provider": "minio"- this is because tests now use a clean env object, so defaults are applied. Verify this is expected behavior.constructive-dborpgpm-modulesimport these functions and need updates (they shouldn't since the API is backwards compatible, but worth verifying)Test plan: Run
pnpm testinpgpm/envandgraphql/envdirectories to verify tests pass.Notes
env.keys.jsonin sync with actual env vars read by the code - this was a maintenance burden that's now eliminatedLink to Devin run: https://app.devin.ai/sessions/dfe526b099de4d71b5f57dbbaa1e064c
Requested by: Dan Lynch (@pyramation)