Add sane defaults to Start-EditorServices and fix logs

We do not need to require so many CLI flags.

This continues to run the existing Emacs and Vim tests utilizing those
flags as a regression scenario, and adds an additional pair of tests
that launch PSES with a minimal set of flags.

This also fixes the log file path to be unique per process, and to use `LogPath`
as a directory instead of a file.

Author: andyleejordan

.github/workflows/emacs-test.yml

@@ -41,7 +41,12 @@ jobs:
         with:
           version: '28.2'
 
-      - name: Run ERT
+      - name: Run ERT with full CLI
         run: |
           emacs -Q --batch -f package-refresh-contents --eval "(package-install 'eglot)"
           emacs -Q --batch -l test/emacs-test.el -f ert-run-tests-batch-and-exit
+
+      - name: Run ERT with simple CLI
+        run: |
+          emacs -Q --batch -f package-refresh-contents --eval "(package-install 'eglot)"
+          emacs -Q --batch -l test/emacs-simple-test.el -f ert-run-tests-batch-and-exit

.github/workflows/vim-test.yml

@@ -61,7 +61,12 @@ jobs:
           repository: thinca/vim-themis
           path: vim-themis
 
-      - name: Run Themis
+      - name: Run Themis with full CLI
         env:
           THEMIS_VIM: ${{ steps.vim.outputs.executable }}
         run: ./vim-themis/bin/themis ./test/vim-test.vim
+
+      - name: Run Themis with simple CLI
+        env:
+          THEMIS_VIM: ${{ steps.vim.outputs.executable }}
+        run: ./vim-themis/bin/themis ./test/vim-simple-test.vim

module/PowerShellEditorServices/Start-EditorServices.ps1

@@ -27,17 +27,14 @@
 #>
 [CmdletBinding(DefaultParameterSetName="NamedPipe")]
 param(
-    [Parameter(Mandatory=$true)]
     [ValidateNotNullOrEmpty()]
     [string]
     $HostName,
 
-    [Parameter(Mandatory=$true)]
     [ValidateNotNullOrEmpty()]
     [string]
     $HostProfileId,
 
-    [Parameter(Mandatory=$true)]
     [ValidateNotNullOrEmpty()]
     [string]
     $HostVersion,
@@ -52,7 +49,6 @@ param(
     [ValidateSet("Diagnostic", "Verbose", "Normal", "Warning", "Error")]
     $LogLevel,
 
-	[Parameter(Mandatory=$true)]
 	[ValidateNotNullOrEmpty()]
 	[string]
 	$SessionDetailsPath,
@@ -78,20 +74,17 @@ param(
     [switch]
     $WaitForDebugger,
 
-    [switch]
-    $ConfirmInstall,
-
-    [Parameter(ParameterSetName="Stdio", Mandatory=$true)]
+    [Parameter(ParameterSetName="Stdio", Mandatory)]
     [switch]
     $Stdio,
 
     [Parameter(ParameterSetName="NamedPipe")]
     [string]
-    $LanguageServicePipeName = $null,
+    $LanguageServicePipeName,
 
     [Parameter(ParameterSetName="NamedPipe")]
     [string]
-    $DebugServicePipeName = $null,
+    $DebugServicePipeName,
 
     [Parameter(ParameterSetName="NamedPipeSimplex")]
     [switch]
@@ -107,11 +100,11 @@ param(
 
     [Parameter(ParameterSetName="NamedPipeSimplex")]
     [string]
-    $DebugServiceInPipeName = $null,
+    $DebugServiceInPipeName,
 
     [Parameter(ParameterSetName="NamedPipeSimplex")]
     [string]
-    $DebugServiceOutPipeName = $null
+    $DebugServiceOutPipeName
 )
 
 Import-Module -Name "$PSScriptRoot/PowerShellEditorServices.psd1"

src/PowerShellEditorServices.Hosting/Commands/StartEditorServicesCommand.cs

@@ -33,6 +33,10 @@ public sealed class StartEditorServicesCommand : PSCmdlet
 
         private HostLogger _logger;
 
+        // NOTE: Ignore the suggestion to use Environment.ProcessId as it doesn't work for
+        // .NET 4.6.2 (for Windows PowerShell), and this won't be caught in CI.
+        private static readonly int s_currentPID = Process.GetCurrentProcess().Id;
+
         public StartEditorServicesCommand()
         {
             // Sets the distribution channel to "PSES" so starts can be distinguished in PS7+ telemetry
@@ -44,30 +48,30 @@ public StartEditorServicesCommand()
         /// <summary>
         /// The name of the EditorServices host to report.
         /// </summary>
-        [Parameter(Mandatory = true)]
+        [Parameter]
         [ValidateNotNullOrEmpty]
-        public string HostName { get; set; }
+        public string HostName { get; set; } = "PSES";
 
         /// <summary>
         /// The ID to give to the host's profile.
         /// </summary>
-        [Parameter(Mandatory = true)]
+        [Parameter]
         [ValidateNotNullOrEmpty]
-        public string HostProfileId { get; set; }
+        public string HostProfileId { get; set; } = "PSES";
 
         /// <summary>
         /// The version to report for the host.
         /// </summary>
-        [Parameter(Mandatory = true)]
+        [Parameter]
         [ValidateNotNullOrEmpty]
-        public Version HostVersion { get; set; }
+        public Version HostVersion { get; set; } = new Version(0, 0, 0);
 
         /// <summary>
         /// Path to the session file to create on startup or startup failure.
         /// </summary>
-        [Parameter(Mandatory = true)]
+        [Parameter]
         [ValidateNotNullOrEmpty]
-        public string SessionDetailsPath { get; set; }
+        public string SessionDetailsPath { get; set; } = "PowerShellEditorServices.json";
 
         /// <summary>
         /// The name of the named pipe to use for the LSP transport.
@@ -120,14 +124,16 @@ public StartEditorServicesCommand()
         /// </summary>
         [Parameter]
         [ValidateNotNullOrEmpty]
-        public string BundledModulesPath { get; set; }
+        public string BundledModulesPath { get; set; } = Path.GetFullPath(Path.Combine(
+            Path.GetDirectoryName(typeof(StartEditorServicesCommand).Assembly.Location),
+            "..", "..", ".."));
 
         /// <summary>
-        /// The absolute path to the where the editor services log file should be created and logged to.
+        /// The absolute path to the folder where logs will be saved.
         /// </summary>
         [Parameter]
         [ValidateNotNullOrEmpty]
-        public string LogPath { get; set; }
+        public string LogPath { get; set; } = Path.Combine(Path.GetTempPath(), "PowerShellEditorServices");
 
         /// <summary>
         /// The minimum log level that should be emitted.
@@ -266,34 +272,32 @@ private void StartLogging()
             }
 
             string logDirPath = GetLogDirPath();
-            string logPath = Path.Combine(logDirPath, "StartEditorServices.log");
+            string logPath = Path.Combine(logDirPath, $"StartEditorServices-{s_currentPID}.log");
 
-            // Temp debugging sessions may try to reuse this same path,
-            // so we ensure they have a unique path
             if (File.Exists(logPath))
             {
                 int randomInt = new Random().Next();
-                logPath = Path.Combine(logDirPath, $"StartEditorServices-temp{randomInt.ToString("X", CultureInfo.InvariantCulture.NumberFormat)}.log");
+                logPath = Path.Combine(logDirPath, $"StartEditorServices-{s_currentPID}-{randomInt.ToString("X", CultureInfo.InvariantCulture.NumberFormat)}.log");
             }
 
             StreamLogger fileLogger = StreamLogger.CreateWithNewFile(logPath);
             _disposableResources.Add(fileLogger);
             IDisposable fileLoggerUnsubscriber = _logger.Subscribe(fileLogger);
             fileLogger.AddUnsubscriber(fileLoggerUnsubscriber);
             _loggerUnsubscribers.Add(fileLoggerUnsubscriber);
-
             _logger.Log(PsesLogLevel.Diagnostic, "Logging started");
         }
 
+        // Sanitizes user input and ensures the directory is created.
         private string GetLogDirPath()
         {
-            string logDir = !string.IsNullOrEmpty(LogPath)
-                ? Path.GetDirectoryName(LogPath)
-                : Path.GetDirectoryName(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location));
+            string logDir = LogPath;
+            if (string.IsNullOrEmpty(logDir))
+            {
+                logDir = Path.Combine(Path.GetTempPath(), "PowerShellEditorServices");
+            }
 
-            // Ensure logDir exists
             Directory.CreateDirectory(logDir);
-
             return logDir;
         }
 

src/PowerShellEditorServices/Hosting/EditorServicesServerFactory.cs

@@ -43,11 +43,15 @@ internal sealed class EditorServicesServerFactory : IDisposable
         /// constructor? In the end it returns an instance (albeit a "singleton").
         /// </para>
         /// </remarks>
-        /// <param name="logPath">The path of the log file to use.</param>
+        /// <param name="logDirectoryPath">The path of the log file to use.</param>
         /// <param name="minimumLogLevel">The minimum log level to use.</param>
         /// <param name="hostLogger">The host logger?</param>
-        public static EditorServicesServerFactory Create(string logPath, int minimumLogLevel, IObservable<(int logLevel, string message)> hostLogger)
+        public static EditorServicesServerFactory Create(string logDirectoryPath, int minimumLogLevel, IObservable<(int logLevel, string message)> hostLogger)
         {
+            // NOTE: Ignore the suggestion to use Environment.ProcessId as it doesn't work for
+            // .NET 4.6.2 (for Windows PowerShell), and this won't be caught in CI.
+            int currentPID = Process.GetCurrentProcess().Id;
+            string logPath = Path.Combine(logDirectoryPath, $"PowerShellEditorServices-{currentPID}.log");
             Log.Logger = new LoggerConfiguration()
                 .Enrich.FromLogContext()
                 .WriteTo.Async(config => config.File(logPath))

test/emacs-simple-test.el

@@ -0,0 +1,63 @@
+;;; emacs-test.el --- Integration testing script          -*- lexical-binding: t; -*-
+
+;; Copyright (c) Microsoft Corporation.
+;; Licensed under the MIT License.
+
+;; Author: Andy Jordan <[email protected]>
+;; Keywords: PowerShell, LSP
+
+;;; Code:
+
+;; Avoid using old packages.
+(setq load-prefer-newer t)
+
+;; Improved TLS Security.
+(with-eval-after-load 'gnutls
+  (custom-set-variables
+   '(gnutls-verify-error t)
+   '(gnutls-min-prime-bits 3072)))
+
+;; Package setup.
+(require 'package)
+(add-to-list 'package-archives
+             '("melpa" . "https://melpa.org/packages/") t)
+(package-initialize)
+(package-refresh-contents)
+
+(require 'ert)
+
+(require 'flymake)
+
+(unless (package-installed-p 'powershell)
+  (package-install 'powershell))
+(require 'powershell)
+
+(unless (package-installed-p 'eglot)
+  (package-install 'eglot))
+(require 'eglot)
+
+(ert-deftest powershell-editor-services ()
+  "Eglot should connect to PowerShell Editor Services."
+  (let* ((repo (project-root (project-current)))
+         (start-script (expand-file-name "module/PowerShellEditorServices/Start-EditorServices.ps1" repo))
+         (test-script (expand-file-name "test/PowerShellEditorServices.Test.Shared/Debugging/VariableTest.ps1" repo))
+         (eglot-sync-connect t))
+    (add-to-list
+     'eglot-server-programs
+     `(powershell-mode
+       . ("pwsh" "-NoLogo" "-NoProfile" "-Command" ,start-script "-Stdio")))
+    (with-current-buffer (find-file-noselect test-script)
+      (should (eq major-mode 'powershell-mode))
+      (should (apply #'eglot--connect (eglot--guess-contact)))
+      (should (eglot-current-server))
+      (let ((lsp (eglot-current-server)))
+        (should (string= (eglot--project-nickname lsp) "PowerShellEditorServices"))
+        (should (member (cons 'powershell-mode "powershell") (eglot--languages lsp))))
+      (sleep-for 5) ; TODO: Wait for "textDocument/publishDiagnostics" instead
+      (flymake-start)
+      (goto-char (point-min))
+      (flymake-goto-next-error)
+      (should (eq 'flymake-warning (face-at-point))))))
+
+(provide 'emacs-test)
+;;; emacs-test.el ends here

test/vim-simple-test.vim

@@ -0,0 +1,40 @@
+let s:suite = themis#suite('pses')
+let s:assert = themis#helper('assert')
+
+function s:suite.before()
+  let l:pses_path = g:repo_root . '/module'
+  let g:LanguageClient_serverCommands = {
+    \ 'ps1': [ 'pwsh', '-NoLogo', '-NoProfile', '-Command',
+    \   l:pses_path . '/PowerShellEditorServices/Start-EditorServices.ps1', '-Stdio' ]
+    \ }
+  let g:LanguageClient_serverStderr = 'DEBUG'
+  let g:LanguageClient_loggingFile = g:repo_root . '/LanguageClient.log'
+  let g:LanguageClient_serverStderr = g:repo_root . '/LanguageServer.log'
+endfunction
+
+function s:suite.has_language_client()
+  call s:assert.includes(&runtimepath, g:repo_root . '/LanguageClient-neovim')
+  call s:assert.cmd_exists('LanguageClientStart')
+  call s:assert.not_empty(g:LanguageClient_serverCommands)
+  call s:assert.true(LanguageClient#HasCommand('ps1'))
+endfunction
+
+function s:suite.analyzes_powershell_file()
+  view test/vim-test.ps1 " This must not use quotes!
+
+  let l:bufnr = bufnr('vim-test.ps1$')
+  call s:assert.not_equal(l:bufnr, -1)
+  let l:bufinfo = getbufinfo(l:bufnr)[0]
+
+  call s:assert.equal(l:bufinfo.name, g:repo_root . '/test/vim-test.ps1')
+  call s:assert.includes(getbufline(l:bufinfo.name, 1), 'function Do-Work {}')
+  " TODO: This shouldn't be necessary, vim-ps1 works locally but not in CI.
+  call setbufvar(l:bufinfo.bufnr, '&filetype', 'ps1')
+  call s:assert.equal(getbufvar(l:bufinfo.bufnr, '&filetype'), 'ps1')
+
+  execute 'LanguageClientStart'
+  execute 'sleep' 5
+  call s:assert.equal(getbufvar(l:bufinfo.name, 'LanguageClient_isServerRunning'), 1)
+  call s:assert.equal(getbufvar(l:bufinfo.name, 'LanguageClient_projectRoot'), g:repo_root)
+  call s:assert.equal(getbufvar(l:bufinfo.name, 'LanguageClient_statusLineDiagnosticsCounts'), {'E': 0, 'W': 1, 'H': 0, 'I': 0})
+endfunction

test/vim-test.vim

@@ -4,7 +4,7 @@ let s:assert = themis#helper('assert')
 function s:suite.before()
   let l:pses_path = g:repo_root . '/module'
   let g:LanguageClient_serverCommands = {
-    \ 'ps1': ['pwsh', '-NoLogo', '-NoProfile', '-Command',
+    \ 'ps1': [ 'pwsh', '-NoLogo', '-NoProfile', '-Command',
     \   l:pses_path . '/PowerShellEditorServices/Start-EditorServices.ps1',
     \   '-HostName', 'vim', '-HostProfileId', 'vim', '-HostVersion', '1.0.0',
     \   '-BundledModulesPath', l:pses_path, '-Stdio',