refactor(nodejs): resolve SDK test context TODO #458
Conversation
…ad of XDG env vars BREAKING CHANGE: Test harness now provides configDir instead of setting XDG_CONFIG_HOME/XDG_STATE_HOME - Remove XDG_CONFIG_HOME and XDG_STATE_HOME environment variable pollution - Add fs.mkdirSync() to create isolated configDir for tests - Export configDir from test harness for tests to use - Update session.test.ts to use harness-provided configDir - Each test now has explicit, isolated configuration directory via SessionConfig.configDir - Prevents mixing SDK sessions with CLI app sessions - Eliminates global environment pollution Fixes: Resolves TODO comment about SDK configuration isolation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Node.js SDK test harness to provide explicit configDir instead of setting XDG environment variables, aiming to resolve a TODO comment about preventing SDK test sessions from mixing with actual CLI app sessions.
Changes:
- Remove XDG_CONFIG_HOME and XDG_STATE_HOME environment variable pollution from test harness
- Create and export isolated
configDirfor tests to use via SessionConfig - Update one test to use the harness-provided configDir
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| nodejs/test/e2e/harness/sdkTestContext.ts | Creates isolated configDir and exports it instead of setting XDG env vars |
| nodejs/test/e2e/session.test.ts | Destructures configDir and uses it in the "custom config dir" test |
| const customConfigDir = `${homeDir}/custom-config`; | ||
| const session = await client.createSession({ | ||
| configDir: customConfigDir, | ||
| configDir: configDir, |
There was a problem hiding this comment.
This test is supposed to verify that a custom config directory can be used, but it's now passing the same configDir that the test harness creates by default. This means it's not actually testing custom configuration directory functionality. Consider either:
- Creating a different directory like
join(homeDir, "custom-config")to truly test custom config dirs, or - Renaming this test to reflect that it's testing the harness-provided config dir explicitly
| it("should create and destroy sessions", async () => { | ||
| const session = await client.createSession({ model: "fake-test-model" }); |
There was a problem hiding this comment.
The TODO comment that was removed warned about mixing SDK sessions with CLI app sessions. While configDir is now exported from the harness, it's not being used in most test session creations in this file. Without XDG environment variables and without passing configDir explicitly, sessions may use the system's default config location (typically ~/.config), potentially mixing with actual CLI app sessions - the exact scenario the TODO warned against. Consider updating all session creations to pass configDir: configDir for proper isolation.
1 participant
BREAKING CHANGE: Test harness now provides configDir instead of setting XDG_CONFIG_HOME/XDG_STATE_HOME
Fixes: Resolves TODO comment about SDK configuration isolation