address comments

Author: roger-zhangg

packages/core/src/shared/env/resolveEnv.ts

@@ -114,7 +114,7 @@ export async function getSpawnEnv(
 export async function mergeResolvedShellPath(env: IProcessEnvironment): Promise<typeof process.env> {
     const shellEnv = await getResolvedShellEnv(env)
     // resolve failed or doesn't need to resolve
-    if (!shellEnv || Object.keys(shellEnv).length === 0) {
+    if (!shellEnv) {
         return env
     }
     try {
@@ -139,29 +139,29 @@ export async function mergeResolvedShellPath(env: IProcessEnvironment): Promise<
  * - we hit a timeout of `MAX_SHELL_RESOLVE_TIME`
  * - any other error from spawning a shell to figure out the environment
  */
-export async function getResolvedShellEnv(env?: IProcessEnvironment): Promise<typeof process.env> {
+export async function getResolvedShellEnv(env?: IProcessEnvironment): Promise<typeof process.env | undefined> {
     if (!env) {
         env = process.env
     }
     // Skip if forceResolveEnv is set to false
     if (DevSettings.instance._isSet('forceResolveEnv') && !DevSettings.instance.get('forceResolveEnv', false)) {
         getLogger().debug('resolveShellEnv(): skipped (forceResolveEnv)')
 
-        return {}
+        return undefined
     }
 
     // Skip on windows
     else if (process.platform === 'win32') {
         getLogger().debug('resolveShellEnv(): skipped (Windows)')
 
-        return {}
+        return undefined
     }
 
     // Skip if running from CLI already and forceResolveEnv is not true
     else if (isLaunchedFromCli(env) && !DevSettings.instance.get('forceResolveEnv', false)) {
         getLogger().info('resolveShellEnv(): skipped (VSCODE_CLI is set)')
 
-        return {}
+        return undefined
     }
     // Otherwise resolve (macOS, Linux)
     else {
@@ -181,10 +181,15 @@ export async function getResolvedShellEnv(env?: IProcessEnvironment): Promise<ty
 
                 // Resolve shell env and handle errors
                 try {
-                    resolve(await doResolveUnixShellEnv(timeout))
+                    const shellEnv = await doResolveUnixShellEnv(timeout)
+                    if (shellEnv && Object.keys(shellEnv).length > 0) {
+                        resolve(shellEnv)
+                    } else {
+                        return undefined
+                    }
                 } catch {
                     // failed resolve should not affect other feature.
-                    resolve({})
+                    return undefined
                 }
             })
         }

packages/core/src/test/shared/env/resolveEnv.test.ts

@@ -18,13 +18,15 @@ describe('resolveEnv', async function () {
         sandbox.restore()
     })
 
-    // a copy of resolveEnv.mergeResolvedShellPath for stubbing
+    // a copy of resolveEnv.mergeResolvedShellPath for stubbing getResolvedShellEnv
+    // mergeResolvedShellPath is calling getResolvedShellEnv within the same file
+    // thus we need a copy to stub getResolvedShellEnv correctly
     const testMergeResolveShellPath = async function mergeResolvedShellPath(
         env: resolveEnv.IProcessEnvironment
     ): Promise<typeof process.env> {
         const shellEnv = await resolveEnv.getResolvedShellEnv(env)
         // resolve failed or doesn't need to resolve
-        if (!shellEnv || Object.keys(shellEnv).length === 0) {
+        if (!shellEnv) {
             return env
         }
         try {
@@ -40,7 +42,7 @@ describe('resolveEnv', async function () {
         }
     }
 
-    describe('resolveWindows', async function () {
+    describe('windows', async function () {
         beforeEach(function () {
             sandbox.stub(process, 'platform').value('win32')
         })
@@ -52,11 +54,11 @@ describe('resolveEnv', async function () {
         })
     })
 
-    describe('resolveUnix', async function () {
+    describe('unix', async function () {
         const originalEnv = { ...process.env }
         // skip mac test on windows
         if (process.platform !== 'win32') {
-            it('mergeResolvedShellPath should get path on mac/unix', async function () {
+            it('mergeResolvedShellPath should get path on mac/linux', async function () {
                 sandbox.stub(process.env, 'PATH').value('')
                 // stub the resolve Env logic cause this is platform sensitive.
                 sandbox.stub(resolveEnv, 'getResolvedShellEnv').resolves(originalEnv)

packages/toolkit/.changes/next-release/Bug Fix-268ceb41-7bec-4b57-b195-d72f19d00faa.json

@@ -1,4 +1,4 @@
 {
 	"type": "Bug Fix",
-	"description": "getSpawnEnv always return correct env on windows"
+	"description": "System Path parsing should ignore Windows and only parse Mac/Linux system, Sam Local Invoke should get correct system Path on windows"
 }