Make deploy config path runtime-based for tests#227
Open
Amanlabh wants to merge 1 commit intometacall:masterfrom
Open
Make deploy config path runtime-based for tests#227Amanlabh wants to merge 1 commit intometacall:masterfrom
Amanlabh wants to merge 1 commit intometacall:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI configuration default-path handling so it’s computed at runtime (allowing tests to override HOME reliably), and adjusts integration tests to run in an isolated temp “home” and to skip auth-dependent cases when credentials aren’t provided.
Changes:
- Replace import-time
defaultPathusage with a runtimegetDefaultPath()in config loading/startup flows. - Update CLI test harness to pass
HOMEto spawned CLI processes from the current runtime env. - Adjust integration tests to set
HOMEto a temporary directory and skip credential-dependent tests when creds are missing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/config.ts |
Introduces getDefaultPath() and uses it as the default for config path resolution at call time. |
src/startup.ts |
Switches startup to load config using runtime default path via getDefaultPath(). |
src/test/cli.ts |
Ensures spawned CLI processes receive HOME from the current process env (or os.homedir() fallback) and changes checkEnvVars to return null instead of failing. |
src/test/cli.integration.spec.ts |
Sets a temp HOME for deploy integration tests and skips the email/password login test when creds aren’t present. |
src/test/login.cli.integration.spec.ts |
Sets temp HOME for login integration tests and skips the suite when creds aren’t present. |
Comment on lines
19
to
+23
| before(async function () { | ||
| await clearCache(); | ||
| checkEnvVars(); | ||
| process.env.HOME = await createTmpDirectory(); | ||
| if (!checkEnvVars()) { | ||
| this.skip(); |
Comment on lines
20
to
+24
| await clearCache(); | ||
| checkEnvVars(); | ||
| process.env.HOME = await createTmpDirectory(); | ||
| if (!checkEnvVars()) { | ||
| this.skip(); | ||
| } |
Comment on lines
16
to
20
| this.timeout(2000000); | ||
| before(async function () { | ||
| process.env.HOME = await createTmpDirectory(); | ||
| }); | ||
|
|
Comment on lines
35
to
+41
| it('Should be able to login using --email & --password flag', async function () { | ||
| await clearCache(); | ||
| const { email, password } = checkEnvVars(); | ||
| const creds = checkEnvVars(); | ||
| if (!creds) { | ||
| this.skip(); | ||
| } | ||
| const { email, password } = creds; |
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.
Summary\n- compute default config path at runtime (so HOME overrides work in tests)\n- update startup to use runtime default path\n- adjust integration tests to use temp HOME and skip auth-only tests when creds are missing\n\n## Testing\n- npm test -- --reporter dot (fails only in auth-required integration tests without creds)