From 45db851cf5aeabbe79b07b28aa10753cce7f651d Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Tue, 9 Jan 2024 16:24:07 -0800 Subject: [PATCH] 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. --- .github/workflows/emacs-test.yml | 7 ++- .github/workflows/vim-test.yml | 7 ++- .../Start-EditorServices.ps1 | 17 ++--- .../Commands/StartEditorServicesCommand.cs | 46 +++++++------- .../Hosting/EditorServicesServerFactory.cs | 8 ++- test/emacs-simple-test.el | 63 +++++++++++++++++++ test/vim-simple-test.vim | 40 ++++++++++++ test/vim-test.vim | 2 +- 8 files changed, 152 insertions(+), 38 deletions(-) create mode 100644 test/emacs-simple-test.el create mode 100644 test/vim-simple-test.vim diff --git a/.github/workflows/emacs-test.yml b/.github/workflows/emacs-test.yml index dcca216e8..d5c51de0e 100644 --- a/.github/workflows/emacs-test.yml +++ b/.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 diff --git a/.github/workflows/vim-test.yml b/.github/workflows/vim-test.yml index 9335a9f47..76e5c5095 100644 --- a/.github/workflows/vim-test.yml +++ b/.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 diff --git a/module/PowerShellEditorServices/Start-EditorServices.ps1 b/module/PowerShellEditorServices/Start-EditorServices.ps1 index 5a9eb2300..3e445679a 100644 --- a/module/PowerShellEditorServices/Start-EditorServices.ps1 +++ b/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" diff --git a/src/PowerShellEditorServices.Hosting/Commands/StartEditorServicesCommand.cs b/src/PowerShellEditorServices.Hosting/Commands/StartEditorServicesCommand.cs index b2b75cfbc..09f866d6a 100644 --- a/src/PowerShellEditorServices.Hosting/Commands/StartEditorServicesCommand.cs +++ b/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() /// /// The name of the EditorServices host to report. /// - [Parameter(Mandatory = true)] + [Parameter] [ValidateNotNullOrEmpty] - public string HostName { get; set; } + public string HostName { get; set; } = "PSES"; /// /// The ID to give to the host's profile. /// - [Parameter(Mandatory = true)] + [Parameter] [ValidateNotNullOrEmpty] - public string HostProfileId { get; set; } + public string HostProfileId { get; set; } = "PSES"; /// /// The version to report for the host. /// - [Parameter(Mandatory = true)] + [Parameter] [ValidateNotNullOrEmpty] - public Version HostVersion { get; set; } + public Version HostVersion { get; set; } = new Version(0, 0, 0); /// /// Path to the session file to create on startup or startup failure. /// - [Parameter(Mandatory = true)] + [Parameter] [ValidateNotNullOrEmpty] - public string SessionDetailsPath { get; set; } + public string SessionDetailsPath { get; set; } = "PowerShellEditorServices.json"; /// /// The name of the named pipe to use for the LSP transport. @@ -120,14 +124,16 @@ public StartEditorServicesCommand() /// [Parameter] [ValidateNotNullOrEmpty] - public string BundledModulesPath { get; set; } + public string BundledModulesPath { get; set; } = Path.GetFullPath(Path.Combine( + Path.GetDirectoryName(typeof(StartEditorServicesCommand).Assembly.Location), + "..", "..", "..")); /// - /// 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. /// [Parameter] [ValidateNotNullOrEmpty] - public string LogPath { get; set; } + public string LogPath { get; set; } = Path.Combine(Path.GetTempPath(), "PowerShellEditorServices"); /// /// The minimum log level that should be emitted. @@ -266,14 +272,12 @@ 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); @@ -281,19 +285,19 @@ private void StartLogging() 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; } diff --git a/src/PowerShellEditorServices/Hosting/EditorServicesServerFactory.cs b/src/PowerShellEditorServices/Hosting/EditorServicesServerFactory.cs index be599b27f..f5cafc206 100644 --- a/src/PowerShellEditorServices/Hosting/EditorServicesServerFactory.cs +++ b/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"). /// /// - /// The path of the log file to use. + /// The path of the log file to use. /// The minimum log level to use. /// The host logger? - 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)) diff --git a/test/emacs-simple-test.el b/test/emacs-simple-test.el new file mode 100644 index 000000000..18e698fa7 --- /dev/null +++ b/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 +;; 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 diff --git a/test/vim-simple-test.vim b/test/vim-simple-test.vim new file mode 100644 index 000000000..fe0adda74 --- /dev/null +++ b/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 diff --git a/test/vim-test.vim b/test/vim-test.vim index 05c8919fd..3d94d174c 100644 --- a/test/vim-test.vim +++ b/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',