Repository: stateful/vscode-runme
Pull request #257 (2023-03-01 23:30:29)

Improve path configuration/resolution
Implements the feedback I gave in #245 .

In summary:

 - Binary path configuration now points to the binary itself, rather than the enclosing folder
 - There is one singular function for finding the binary path, `getBinaryPath`
 - The extension root Uri is passed down as a Uri, rather than as a string (avoids unnecessary (re)-parsing)
 - Relative binary paths now act relative to the current workspace folder, rather than relative to extension folder

sourishkrout: > Seems to work as intended on OSX, @sourishkrout if you could test on Windows - I don't have access to a windows PC at the moment.

Will do. Will report back before EoD.

Patch:
diff --git a/__mocks__/vscode.ts b/__mocks__/vscode.ts
index f120a2d1..93cc305e 100644
--- a/__mocks__/vscode.ts
+++ b/__mocks__/vscode.ts
@@ -1,3 +1,4 @@
+import path from 'node:path'
 import { readFileSync } from 'node:fs'
 import { join } from 'node:path'
 
@@ -16,10 +17,14 @@ export const notebooks = {
   }),
 }
 
-export const Uri = {
-  joinPath: vi.fn().mockReturnValue('/foo/bar'),
-  parse: vi.fn((uri: string) => URI.parse(uri)),
-  file: vi.fn().mockReturnValue('')
+
+export class Uri extends URI {
+  static file = vi.fn(super.file)
+  static parse = vi.fn(super.parse)
+
+  static joinPath = vi.fn((uri: Uri, ...paths: string[]) => {
+    return Uri.file(path.join(uri.fsPath, ...paths))
+  })
 }
 
 export const workspace = {
diff --git a/package.json b/package.json
index 48b138c6..840cbcd3 100644
--- a/package.json
+++ b/package.json
@@ -273,7 +273,6 @@
           "runme.server.binaryPath": {
             "type": "string",
             "scope": "machine",
-            "default": "bin",
             "markdownDescription": "Runme server path to binary. (It requires the GRPC interface enabled)"
           },
           "runme.server.enableLogger": {
diff --git a/src/extension/extension.ts b/src/extension/extension.ts
index cad1ccd2..ca6f5b2d 100644
--- a/src/extension/extension.ts
+++ b/src/extension/extension.ts
@@ -31,7 +31,7 @@ export class RunmeExtension {
     const kernel = new Kernel(context)
     const grpcSerializer = kernel.hasExperimentEnabled('grpcSerializer')
     const grpcServer = kernel.hasExperimentEnabled('grpcServer')
-    const server = new RunmeServer(context.extensionUri.fsPath, {
+    const server = new RunmeServer(context.extensionUri, {
       retryOnFailure: true,
       maxNumberOfIntents: 2,
     })
diff --git a/src/extension/server/runmeServer.ts b/src/extension/server/runmeServer.ts
index c6345f4b..04d1888c 100644
--- a/src/extension/server/runmeServer.ts
+++ b/src/extension/server/runmeServer.ts
@@ -2,10 +2,10 @@ import { ChildProcessWithoutNullStreams, spawn } from 'node:child_process'
 import fs from 'node:fs/promises'
 import EventEmitter from 'node:events'
 
-import { Disposable } from 'vscode'
+import { Disposable, Uri } from 'vscode'
 
 import { SERVER_ADDRESS } from '../../constants'
-import { enableServerLogs, getBinaryLocation, getPath, getPortNumber } from '../../utils/configuration'
+import { enableServerLogs, getBinaryPath, getPortNumber } from '../../utils/configuration'
 import { initParserClient } from '../grpc/client'
 import { DeserializeRequest } from '../grpc/serializerTypes'
 
@@ -25,7 +25,7 @@ class RunmeServer implements Disposable {
 
     #runningPort: number
     #process: ChildProcessWithoutNullStreams | undefined
-    #binaryPath: string
+    #binaryPath: Uri
     #retryOnFailure: boolean
     #maxNumberOfIntents: number
     #loggingEnabled: boolean
@@ -35,10 +35,10 @@ class RunmeServer implements Disposable {
     #acceptsInterval: number
     events: EventEmitter
 
-    constructor(extBasePath: string, options: IServerConfig) {
+    constructor(extBasePath: Uri, options: IServerConfig) {
         this.#runningPort = getPortNumber()
         this.#loggingEnabled = enableServerLogs()
-        this.#binaryPath = getPath(extBasePath)
+        this.#binaryPath = getBinaryPath(extBasePath, process.platform)
         this.#retryOnFailure = options.retryOnFailure || false
         this.#maxNumberOfIntents = options.maxNumberOfIntents
         this.#intent = 0
@@ -76,7 +76,7 @@ class RunmeServer implements Disposable {
           return this.#address
         }
 
-        const binaryLocation = getBinaryLocation(this.#binaryPath, process.platform)
+        const binaryLocation = this.#binaryPath.fsPath
 
         const binaryExists = await fs.access(binaryLocation)
             .then(() => true, () => false)
diff --git a/src/utils/configuration.ts b/src/utils/configuration.ts
index df92bdfa..69c265fe 100644
--- a/src/utils/configuration.ts
+++ b/src/utils/configuration.ts
@@ -1,3 +1,5 @@
+import path from 'node:path'
+
 import { Uri, workspace } from 'vscode'
 import { z } from 'zod'
 
@@ -13,9 +15,7 @@ const configurationSchema = {
             .default(7863),
         binaryPath: z
             .string()
-            .transform((schema) => {
-                return schema || 'bin'
-            }),
+            .optional(),
         enableLogger: z
             .boolean()
             .default(false)
@@ -36,27 +36,22 @@ const getPortNumber = (): number => {
     return getServerConfigurationValue<number>('port', 7863)
 }
 
-const getPath = (basePath: string): string => {
-    const baseUri = Uri.file(basePath)
-    const binaryPath = getServerConfigurationValue<string>('binaryPath', 'bin')
-
-    // relative path
-    if (
-      !binaryPath.startsWith('/') &&
-      binaryPath.length > 2 &&
-      binaryPath[1] !== ':'
-    ) {
-      return Uri.joinPath(baseUri, binaryPath).fsPath
-    }
-
-    return binaryPath
-}
+const getBinaryPath = (extensionBaseUri: Uri, platform: string): Uri => {
+    const userPath = getServerConfigurationValue<string|undefined>('binaryPath', undefined)
 
-const getBinaryLocation = (binaryPath: string, platform: string): string => {
     const isWin = platform.toLowerCase().startsWith('win')
     const binName = isWin ? 'runme.exe' : 'runme'
-    const pathUri = Uri.file(binaryPath)
-    return Uri.joinPath(pathUri, binName).fsPath
+    const bundledPath = Uri.joinPath(extensionBaseUri, 'bin', binName)
+
+    if(userPath) {
+      if (path.isAbsolute(userPath)) {
+        return Uri.file(userPath)
+      } else if(workspace.workspaceFolders && workspace.workspaceFolders.length > 0) {
+        return Uri.joinPath(workspace.workspaceFolders[0].uri, userPath)
+      }
+    }
+
+    return bundledPath
 }
 
 const enableServerLogs = (): boolean => {
@@ -64,8 +59,8 @@ const enableServerLogs = (): boolean => {
 }
 
 export {
-    getBinaryLocation,
     getPortNumber,
-    getPath,
-    enableServerLogs
+    getBinaryPath,
+    enableServerLogs,
+    getServerConfigurationValue
 }


Test patch:
diff --git a/tests/extension/configuration.test.ts b/tests/extension/configuration.test.ts
index d7f03e28..e9943d99 100644
--- a/tests/extension/configuration.test.ts
+++ b/tests/extension/configuration.test.ts
@@ -1,8 +1,14 @@
+import path from 'node:path'
+
 import { suite, test, expect, vi, beforeEach, afterEach } from 'vitest'
-import { Uri } from 'vscode'
-import { URI } from 'vscode-uri'
+import { Uri, workspace } from 'vscode'
 
-import { getPath, getPortNumber, enableServerLogs, getBinaryLocation } from '../../src/utils/configuration'
+import {
+  getPortNumber,
+  enableServerLogs,
+  getBinaryPath,
+  getServerConfigurationValue
+} from '../../src/utils/configuration'
 
 const FAKE_UNIX_EXT_PATH = '/Users/user/.vscode/extension/stateful.runme'
 const FAKE_WIN_EXT_PATH = 'C:\\Users\\.vscode\\extensions\\stateful.runme'
@@ -18,30 +24,48 @@ const SETTINGS_MOCK:
     enableLogger: undefined
 }
 
-const winJoinPath = (uri: any, fragment: string) => {
-    const winFsPath = `${uri.fsPath}\\${fragment}`
-    return { fsPath: winFsPath } as any
-}
-
 beforeEach(() => {
-    vi.mock('vscode', () => ({
-        workspace: {
-            getConfiguration: vi.fn().mockReturnValue({
-                get: (configurationName) => {
-                    return SETTINGS_MOCK[configurationName]
-                }
-            })
-        },
-        Uri: {
-            joinPath: vi.fn().mockImplementation((uri: any, fragment: string) => {
-                return URI.parse(`${uri.fsPath}/${fragment}`)
-            }),
-            parse: vi.fn((uri: string) => URI.parse(uri)),
-            file: vi.fn((uri: string) => URI.file(uri))
-        }
-    }))
+  vi.mock('vscode', async () => {
+    const mocked = await import('../../__mocks__/vscode')
+
+    return ({
+      workspace: {
+        getConfiguration: vi.fn().mockReturnValue({
+          get: (configurationName) => {
+            return SETTINGS_MOCK[configurationName]
+          }
+        }),
+      },
+      Uri: mocked.Uri,
+    })
+  })
+
+
+  vi.mock('node:path', async () => {
+    const p = await vi.importActual('node:path') as typeof import('node:path')
+
+    const pathMock = {
+      win32: p.win32,
+      posix: p.posix,
+    }
+
+    mockedPathMethods().forEach(m => pathMock[m] = vi.fn(p[m] as any))
+
+    return ({
+      ...pathMock,
+      default: pathMock,
+    })
+  })
 })
 
+function mockedPathMethods() {
+  return ['join', 'isAbsolute'] as const
+}
+
+function platformPathMocks(platform: path.PlatformPath) {
+  mockedPathMethods().forEach(m => vi.mocked(path[m]).mockImplementation(platform[m] as any))
+}
+
 afterEach(() => {
     Object.keys(SETTINGS_MOCK).forEach(key => {
         SETTINGS_MOCK[key] = undefined
@@ -60,56 +84,90 @@ suite('Configuration', () => {
         expect(portNumber).toStrictEqual(8080)
     })
 
-
-    test('Should default to a valid binaryPath', () => {
-        const path = getPath(FAKE_UNIX_EXT_PATH)
-        expect(path).toStrictEqual('/Users/user/.vscode/extension/stateful.runme/bin')
+    test('Should disable server logs with an invalid value', () => {
+      SETTINGS_MOCK.enableLogger = undefined
+      const path = enableServerLogs()
+      expect(path).toBeFalsy()
     })
 
-    test('Should default to a valid relative binaryPath when specified', () => {
-        SETTINGS_MOCK.binaryPath = 'newBin'
-        const path = getPath(FAKE_UNIX_EXT_PATH)
-        expect(path).toStrictEqual('/Users/user/.vscode/extension/stateful.runme/newBin')
+    test('Should disable server logs with an invalid string', () => {
+        SETTINGS_MOCK.enableLogger = 'true'
+        const path = enableServerLogs()
+        expect(path).toBeFalsy()
     })
 
-    test('Should default to a valid absolute binaryPath when specified', () => {
-        SETTINGS_MOCK.binaryPath = '/opt/homebrew/bin'
-        const path = getPath(FAKE_UNIX_EXT_PATH)
-        expect(path).toStrictEqual('/opt/homebrew/bin')
-    })
+    test('getServerConfigurationValue Should default to undefined binaryPath', () => {
+      SETTINGS_MOCK.binaryPath = undefined
 
-    test('Should use runme for non-windows platforms', () => {
-        SETTINGS_MOCK.binaryPath = '/opt/homebrew/bin'
-        const path = getPath(FAKE_UNIX_EXT_PATH)
-        const binLoc = getBinaryLocation(path, 'darwin')
-        expect(binLoc).toStrictEqual('/opt/homebrew/bin/runme')
+      expect(
+        getServerConfigurationValue<string | undefined>('binaryPath', undefined)
+      ).toStrictEqual(undefined)
     })
 
-    test('Should use runme.exe for windows platforms with absolute path', () => {
-        vi.mocked(Uri.joinPath).mockImplementation(winJoinPath)
-        SETTINGS_MOCK.binaryPath = 'C:\\Users\\.vscode\\extensions\\stateful.runme\\bin'
-        const path = getPath(FAKE_WIN_EXT_PATH)
-        const binLoc = getBinaryLocation(path, 'win32')
-        expect(binLoc).toStrictEqual('c:\\Users\\.vscode\\extensions\\stateful.runme\\bin\\runme.exe')
-    })
+    test('getServerConfigurationValue Should give proper binaryPath if defined', () => {
+      SETTINGS_MOCK.binaryPath = '/binary/path'
 
-    test('Should use runme.exe for windows platforms with relative path', () => {
-        vi.mocked(Uri.joinPath).mockImplementation(winJoinPath)
-        SETTINGS_MOCK.binaryPath = 'newBin'
-        const path = getPath(FAKE_WIN_EXT_PATH)
-        const binLoc = getBinaryLocation(path, 'win32')
-        expect(binLoc).toStrictEqual('c:\\Users\\.vscode\\extensions\\stateful.runme\\newBin\\runme.exe')
+      expect(
+        getServerConfigurationValue<string | undefined>('binaryPath', undefined)
+      ).toStrictEqual('/binary/path')
     })
 
-    test('Should disable server logs with an invalid value', () => {
-        SETTINGS_MOCK.enableLogger = undefined
-        const path = enableServerLogs()
-        expect(path).toBeFalsy()
+    suite('posix', () => {
+      beforeEach(() => {
+        platformPathMocks(path.posix)
+      })
+
+      test('Should default to a valid binaryPath', () => {
+          const binary = getBinaryPath(Uri.file(FAKE_UNIX_EXT_PATH), 'linux')
+          expect(binary.fsPath).toStrictEqual('/Users/user/.vscode/extension/stateful.runme/bin/runme')
+      })
+
+      test('Should default to a valid relative binaryPath when specified', () => {
+          SETTINGS_MOCK.binaryPath = 'newBin'
+          // @ts-expect-error
+          workspace.workspaceFolders = [{ uri: Uri.file('/Users/user/Projects/project') }]
+          const binary = getBinaryPath(Uri.file(FAKE_UNIX_EXT_PATH), 'linux')
+          expect(binary.fsPath).toStrictEqual('/Users/user/Projects/project/newBin')
+      })
+
+      test('Should default to a valid absolute binaryPath when specified', () => {
+        SETTINGS_MOCK.binaryPath = '/opt/homebrew/bin/runme'
+        const binary = getBinaryPath(Uri.file(FAKE_UNIX_EXT_PATH), 'linux')
+        expect(binary.fsPath).toStrictEqual('/opt/homebrew/bin/runme')
+      })
+
+      test('Should use runme for non-windows platforms', () => {
+          SETTINGS_MOCK.binaryPath = '/opt/homebrew/bin/runme'
+          const binary = getBinaryPath(Uri.file(FAKE_UNIX_EXT_PATH), 'darwin')
+          expect(binary.fsPath).toStrictEqual('/opt/homebrew/bin/runme')
+      })
     })
 
-    test('Should disable server logs with an invalid string', () => {
-        SETTINGS_MOCK.enableLogger = 'true'
-        const path = enableServerLogs()
-        expect(path).toBeFalsy()
+    suite('win32', () => {
+      beforeEach(() => {
+        platformPathMocks(path.win32)
+      })
+
+      test('Should default to a valid binaryPath exe on windows', () => {
+        const binary = getBinaryPath(Uri.file(FAKE_WIN_EXT_PATH), 'win')
+        expect(binary.fsPath).toStrictEqual(
+          'c:\\Users\\.vscode\\extensions\\stateful.runme\\bin\\runme.exe'
+        )
+      })
+
+      test('Should use runme.exe for windows platforms with absolute path', () => {
+        SETTINGS_MOCK.binaryPath = 'C:\\custom\\path\\to\\bin\\runme.exe'
+
+        const binary = getBinaryPath(Uri.file(FAKE_WIN_EXT_PATH), 'win32')
+        expect(binary.fsPath).toStrictEqual('c:\\custom\\path\\to\\bin\\runme.exe')
+      })
+
+      test('Should use runme.exe for windows platforms with relative path', () => {
+          SETTINGS_MOCK.binaryPath = 'newBin.exe'
+          // @ts-expect-error
+          workspace.workspaceFolders = [{ uri: Uri.file('c:\\Users\\Projects\\project') }]
+          const binary = getBinaryPath(Uri.file(FAKE_WIN_EXT_PATH), 'win32')
+          expect(binary.fsPath).toStrictEqual('c:\\Users\\Projects\\project\\newBin.exe')
+      })
     })
 })
diff --git a/tests/extension/extension.test.ts b/tests/extension/extension.test.ts
index 3fff496d..08a7a94e 100644
--- a/tests/extension/extension.test.ts
+++ b/tests/extension/extension.test.ts
@@ -1,5 +1,5 @@
 import { test, expect, vi } from 'vitest'
-import { notebooks, workspace, commands, window } from 'vscode'
+import { notebooks, workspace, commands, window, Uri } from 'vscode'
 
 import { RunmeExtension } from '../../src/extension/extension'
 
@@ -20,6 +20,7 @@ test('initializes all providers', async () => {
   vi.mocked(workspace.getConfiguration).mockReturnValue({
     get: vi.fn((config: string) => configValues[config])
   } as any)
+  vi.mocked(Uri.joinPath).mockReturnValue('/foo/bar' as any)
   const context: any = { subscriptions: [], extensionUri: { fsPath: '/foo/bar' } }
   const ext = new RunmeExtension()
   await ext.initialize(context)
diff --git a/tests/extension/handler/uri.test.ts b/tests/extension/handler/uri.test.ts
index caf4ceed..243c6963 100644
--- a/tests/extension/handler/uri.test.ts
+++ b/tests/extension/handler/uri.test.ts
@@ -6,7 +6,7 @@ import { TelemetryReporter } from 'vscode-telemetry'
 
 import { RunmeUriHandler } from '../../../src/extension/handler/uri'
 import {
-    getProjectDir, waitForProjectCheckout, getSuggestedProjectName, writeBootstrapFile, parseParams
+    getProjectDir, waitForProjectCheckout, getSuggestedProjectName, writeBootstrapFile, parseParams, getTargetDirName
 } from '../../../src/extension/handler/utils'
 
 vi.mock('vscode')
@@ -103,11 +103,12 @@ describe('RunmeUriHandler', () => {
         it('should not run if project dir or suggested name can not be identified', async () => {
             await handler['_setupProject']('README.md', 'foo')
             expect(window.showInformationMessage).toHaveBeenCalledTimes(0)
-            vi.mocked(getProjectDir).mockResolvedValueOnce('foobar' as any)
+            vi.mocked(getProjectDir).mockResolvedValueOnce(Uri.file('foobar'))
             await handler['_setupProject']('README.md', 'foo')
             expect(window.showInformationMessage).toHaveBeenCalledTimes(0)
-            vi.mocked(getProjectDir).mockResolvedValueOnce('foobar' as any)
-            vi.mocked(getSuggestedProjectName).mockResolvedValueOnce('stateful/runme')
+            vi.mocked(getProjectDir).mockResolvedValueOnce(Uri.file('foobar'))
+            vi.mocked(getSuggestedProjectName).mockReturnValueOnce('stateful/runme')
+            vi.mocked(getTargetDirName).mockResolvedValueOnce('stateful/runme')
             await handler['_setupProject']('README.md', 'foo')
             expect(window.showInformationMessage).toHaveBeenCalledTimes(1)
             expect(window.withProgress).toHaveBeenCalledTimes(1)
@@ -132,14 +133,14 @@ describe('RunmeUriHandler', () => {
         it('should fail gracefully', async () => {
             vi.mocked(got.get).mockRejectedValueOnce(new Error('ups'))
             vi.mocked(Uri.parse).mockReturnValue({ query: { command: 'setup' }, fsPath: '/foo/bar.md'} as any)
-            vi.mocked(getProjectDir).mockResolvedValueOnce('foobar' as any)
+            vi.mocked(getProjectDir).mockResolvedValueOnce(Uri.file('foobar'))
             await handler['_setupFile']('/foo/bar.md')
             expect(vi.mocked(window.showErrorMessage).mock.calls[0][0]).toMatch(/Failed to set-up/)
         })
 
         it('should create new dir with file and open VS Code', async () => {
             vi.mocked(Uri.parse).mockReturnValue({ query: { command: 'setup' }, fsPath: '/foo/bar.md'} as any)
-            vi.mocked(getProjectDir).mockResolvedValueOnce('foobar' as any)
+            vi.mocked(getProjectDir).mockResolvedValueOnce(Uri.file('foobar'))
             await handler['_setupFile']('/foo/bar.md')
             expect(workspace.fs.createDirectory).toBeCalledTimes(1)
             expect(workspace.fs.writeFile).toBeCalledTimes(1)
diff --git a/tests/extension/provider/launcher.test.ts b/tests/extension/provider/launcher.test.ts
index 08a102cd..349603a7 100644
--- a/tests/extension/provider/launcher.test.ts
+++ b/tests/extension/provider/launcher.test.ts
@@ -1,5 +1,5 @@
 import { vi, describe, it, expect, beforeEach } from 'vitest'
-import { commands } from 'vscode'
+import { commands, Uri } from 'vscode'
 
 import { RunmeFile, RunmeLauncherProvider } from '../../../src/extension/provider/launcher'
 import { getDefaultWorkspace } from '../../../src/extension/utils'
@@ -64,9 +64,11 @@ describe('Runme Notebooks', () => {
     })
 
     it('should open a file using runme renderer', () => {
+      vi.mocked(Uri.file).mockImplementation((p) => p as any)
+
       RunmeLauncherProvider.openFile({ file: 'README.md', folderPath: 'runme/workspace/src' })
       expect(commands.executeCommand).toBeCalledTimes(1)
-      expect(commands.executeCommand).toBeCalledWith('vscode.openWith', expect.any(String), 'runme')
+      expect(commands.executeCommand).toBeCalledWith('vscode.openWith', 'runme/workspace/src/README.md', 'runme')
     })
   })
 
diff --git a/tests/extension/server/runmeServer.test.ts b/tests/extension/server/runmeServer.test.ts
index 5af97d0f..ea601f52 100644
--- a/tests/extension/server/runmeServer.test.ts
+++ b/tests/extension/server/runmeServer.test.ts
@@ -1,5 +1,5 @@
 import { suite, test, expect, vi, beforeEach } from 'vitest'
-import { workspace } from 'vscode'
+import { Uri, workspace } from 'vscode'
 
 vi.mock('vscode')
 vi.mock('../../../src/extension/grpc/client', () => ({ initParserClient: vi.fn() }))
@@ -28,7 +28,7 @@ suite('Runme server spawn process', () => {
 
     test('Should try 2 times before failing', async () => {
         const server = new Server(
-          '/Users/user/.vscode/extension/stateful.runme',
+          Uri.file('/Users/user/.vscode/extension/stateful.runme'),
           {
             retryOnFailure: true,
             maxNumberOfIntents: 2,
@@ -44,7 +44,7 @@ suite('Runme server accept connections', () => {
     let server: Server
     beforeEach(() => {
         server = new Server(
-          '/Users/user/.vscode/extension/stateful.runme',
+          Uri.file('/Users/user/.vscode/extension/stateful.runme'),
           {
             retryOnFailure: false,
             maxNumberOfIntents: 2,