Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add simple integration test, with an LSP client PowerShell module #944

Merged
merged 47 commits into from
May 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
c708a1b
Add first parts of ps pses client
Apr 19, 2019
e36001a
Make message reading work
Apr 19, 2019
9e67dea
Start pipe module
rjmholt Apr 22, 2019
b66a9ac
Add simple startup test
rjmholt Apr 22, 2019
6711016
Add shutdown test
rjmholt Apr 22, 2019
be4587e
Add pester installation to build step
rjmholt Apr 22, 2019
2158e3b
Response to Codacy bot's feedback
rjmholt Apr 22, 2019
4952bad
Add classes and types to psm1
rjmholt Apr 22, 2019
7ba21c2
Fix build logic, add return types
rjmholt Apr 23, 2019
558abc0
Rename pester test file to be more specific
rjmholt Apr 23, 2019
5c35561
Kill process properly
rjmholt Apr 23, 2019
81631a1
Fix process kill
rjmholt Apr 23, 2019
adcb527
Add diagnostic line;
rjmholt Apr 23, 2019
b16429d
Attempt to exit
rjmholt Apr 23, 2019
0435cb6
Add some documentation comments
rjmholt Apr 24, 2019
7e9a76d
Ensure pipe is closed during disposal
rjmholt Apr 24, 2019
9b66b61
Fix pipe disposal
rjmholt Apr 24, 2019
d949558
Change cancellation method
rjmholt Apr 24, 2019
b061b65
Try invoking pester in proc
rjmholt Apr 25, 2019
6b13e22
Add doc comments
rjmholt Apr 25, 2019
29ccd29
Use better exceptions
rjmholt Apr 25, 2019
2896933
Comply with some of Codacy's draconian views
rjmholt Apr 25, 2019
7b3ee70
Publish Pester results in CI
rjmholt Apr 26, 2019
5a2b3d0
Remove .vscode folder
rjmholt Apr 26, 2019
3bb6df4
Merge branch 'master' into integration-tools
rjmholt Apr 26, 2019
228b5eb
Fix typo
rjmholt Apr 26, 2019
516cbb0
Fix NRE in test
rjmholt Apr 26, 2019
beeef02
Improve client module building
rjmholt Apr 26, 2019
dae7511
Pass through dotnet location
rjmholt Apr 26, 2019
f8654d6
Ensure new enough Pester is installed
rjmholt Apr 26, 2019
14b8884
Add -Force flag
rjmholt Apr 26, 2019
4069a40
Update tools/PsesPsClient/PsesPsClient.psd1
TylerLeonhardt Apr 30, 2019
a6e2e99
Rework module, address @TylerLeonhardt's feedback
rjmholt Apr 30, 2019
e8529dd
Update PowerShellEditorServices.build.ps1
TylerLeonhardt May 3, 2019
b0d704c
Update tools/PsesPsClient/Client.cs
rjmholt May 9, 2019
1b90683
Add server output to integration test
May 14, 2019
c0bcefb
Try stderr recording
May 14, 2019
d9b3a0b
Try procinfo
May 14, 2019
c48b586
Don't use stream when it doesn't exist
May 14, 2019
e2e3c9b
Try again with error file
May 14, 2019
a7e599c
Use log parser to find errors
May 14, 2019
47cdb90
Add test exception for pwsh pipe close error
May 14, 2019
d5a67c2
Improve error message
May 14, 2019
dc2506e
Test without skipping crash log exception
May 15, 2019
135d47d
Try ending server stream first
May 15, 2019
37ddedf
Improve testing
May 15, 2019
9f5666f
Add comments about test ordering
May 15, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .vsts-ci/templates/ci-general.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ steps:
testRunner: VSTest
testResultsFiles: '**/*.trx'
condition: succeededOrFailed()
- task: PublishTestResults@2
inputs:
testRunner: NUnit
testResultsFiles: '**/TestResults.xml'
condition: succeededOrFailed()
- task: PublishBuildArtifacts@1
inputs:
ArtifactName: PowerShellEditorServices
Expand Down
31 changes: 29 additions & 2 deletions PowerShellEditorServices.build.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ param(

#Requires -Modules @{ModuleName="InvokeBuild";ModuleVersion="3.2.1"}

$script:IsCIBuild = $env:TF_BUILD -ne $null
$script:IsUnix = $PSVersionTable.PSEdition -and $PSVersionTable.PSEdition -eq "Core" -and !$IsWindows
$script:TargetPlatform = "netstandard2.0"
$script:TargetFrameworksParam = "/p:TargetFrameworks=`"$script:TargetPlatform`""
$script:RequiredSdkVersion = (Get-Content (Join-Path $PSScriptRoot 'global.json') | ConvertFrom-Json).sdk.version
$script:MinimumPesterVersion = '4.7'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just add this to the #Requires?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of #requires in general, but I think here we might want to build without testing in some circumstances

$script:NugetApiUriBase = 'https://www.nuget.org/api/v2/package'
$script:ModuleBinPath = "$PSScriptRoot/module/PowerShellEditorServices/bin/"
$script:VSCodeModuleBinPath = "$PSScriptRoot/module/PowerShellEditorServices.VSCode/bin/"
Expand Down Expand Up @@ -329,12 +329,17 @@ task Build {
exec { & $script:dotnetExe build -c $Configuration .\src\PowerShellEditorServices.VSCode\PowerShellEditorServices.VSCode.csproj $script:TargetFrameworksParam }
}

task BuildPsesClientModule SetupDotNet,{
Write-Verbose 'Building PsesPsClient testing module'
& $PSScriptRoot/tools/PsesPsClient/build.ps1 -DotnetExe $script:dotnetExe
}

function DotNetTestFilter {
# Reference https://docs.microsoft.com/en-us/dotnet/core/testing/selective-unit-tests
if ($TestFilter) { @("--filter",$TestFilter) } else { "" }
}

task Test TestServer,TestProtocol
task Test TestServer,TestProtocol,TestPester

task TestServer {
Set-Location .\test\PowerShellEditorServices.Test\
Expand Down Expand Up @@ -372,6 +377,28 @@ task TestHost {
exec { & $script:dotnetExe test -f $script:TestRuntime.Core (DotNetTestFilter) }
}

task TestPester Build,BuildPsesClientModule,EnsurePesterInstalled,{
$testParams = @{}
if ($env:TF_BUILD)
{
$testParams += @{
OutputFormat = 'NUnitXml'
OutputFile = 'TestResults.xml'
}
}
$result = Invoke-Pester "$PSScriptRoot/test/Pester/" @testParams -PassThru

if ($result.FailedCount -gt 0)
{
throw "$($result.FailedCount) tests failed."
}
}

task EnsurePesterInstalled -If (-not (Get-Module Pester -ListAvailable | Where-Object Version -ge $script:MinimumPesterVersion)) {
Write-Warning "Required Pester version not found, installing Pester to current user scope"
Install-Module -Scope CurrentUser Pester -Force -SkipPublisherCheck
}

task LayoutModule -After Build {
# Copy Third Party Notices.txt to module folder
Copy-Item -Force -Path "$PSScriptRoot\Third Party Notices.txt" -Destination $PSScriptRoot\module\PowerShellEditorServices
Expand Down
86 changes: 86 additions & 0 deletions test/Pester/EditorServices.Integration.Tests.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@

$script:ExceptionRegex = [regex]::new('\s*Exception: (.*)$', 'Compiled,Multiline,IgnoreCase')
function ReportLogErrors
{
param(
[Parameter()][string]$LogPath,

[Parameter()][ref]<#[int]#>$FromIndex = 0,

[Parameter()][string[]]$IgnoreException = @()
)

$logEntries = Parse-PsesLog $LogPath |
Where-Object Index -ge $FromIndex.Value

# Update the index to the latest in the log
$FromIndex.Value = ($FromIndex.Value,$errorLogs.Index | Measure-Object -Maximum).Maximum

$errorLogs = $logEntries |
Where-Object LogLevel -eq Error |
Where-Object {
$match = $script:ExceptionRegex.Match($_.Message.Data)

(-not $match) -or ($match.Groups[1].Value.Trim() -notin $IgnoreException)
}

if ($errorLogs)
{
$errorLogs | ForEach-Object { Write-Error "ERROR from PSES log: $($_.Message.Data)" }
}
}

Describe "Loading and running PowerShellEditorServices" {
BeforeAll {
Import-Module -Force "$PSScriptRoot/../../module/PowerShellEditorServices"
Import-Module -Force "$PSScriptRoot/../../tools/PsesPsClient/out/PsesPsClient"
Import-Module -Force "$PSScriptRoot/../../tools/PsesLogAnalyzer"

$logIdx = 0
$psesServer = Start-PsesServer
$client = Connect-PsesServer -PipeName $psesServer.SessionDetails.languageServicePipeName
}

# This test MUST be first
It "Starts and responds to an initialization request" {
$request = Send-LspInitializeRequest -Client $client
$response = Get-LspResponse -Client $client -Id $request.Id
$response.Id | Should -BeExactly $request.Id

ReportLogErrors -LogPath $psesServer.LogPath -FromIndex ([ref]$logIdx)
rjmholt marked this conversation as resolved.
Show resolved Hide resolved
}

# This test MUST be last
Copy link
Member

@TylerLeonhardt TylerLeonhardt May 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is the right approach. Maybe it should be 2 Context's:

  1. "Message handling"
  2. "Startup and shutdown"

So the tests would be structured like this:

Describe "Loading and running PowerShellEditorServices" {
    BeforeAll {
        Import-Module -Force "$PSScriptRoot/../../module/PowerShellEditorServices"
        Import-Module -Force "$PSScriptRoot/../../tools/PsesPsClient/out/PsesPsClient"
        Import-Module -Force "$PSScriptRoot/../../tools/PsesLogAnalyzer"

        $logIdx = 0
    }
    Context "Startup and shutdown" {
        It "Can start up and shutdown" {
            # Start up
            $psesServer = Start-PsesServer
            $client = Connect-PsesServer -PipeName $psesServer.SessionDetails.languageServicePipeName

            # Shutdown
            $request = Send-LspShutdownRequest -Client $client
            $response = Get-LspResponse -Client $client -Id $request.Id
            $response.Id | Should -BeExactly $request.Id
            $response.Result | Should -BeNull
            # TODO: The server seems to stay up waiting for the debug connection
            # $psesServer.PsesProcess.HasExited | Should -BeTrue
           
            try
            {
                $psesServer.PsesProcess.Kill()
            }
            finally
            {
                try
                {
                    $psesServer.PsesProcess.Dispose()
                }
                finally
                {
                    $client.Dispose()
                }
            }

            ReportLogErrors -LogPath $psesServer.LogPath -FromIndex ([ref]$logIdx)
        }
    }

    Context "LSP messages" {
        BeforeAll {
            # Start up
            $psesServer = Start-PsesServer
            $client = Connect-PsesServer -PipeName $psesServer.SessionDetails.languageServicePipeName
        }
        AfterAll {
            try
            {
                $psesServer.PsesProcess.Kill()
            }
            finally
            {
                try
                {
                    $psesServer.PsesProcess.Dispose()
                }
                finally
                {
                    $client.Dispose()
                }
            }
        }
        It "Can send initialize request" {
            $request = Send-LspInitializeRequest -Client $client
            $response = Get-LspResponse -Client $client -Id $request.Id
            $response.Id | Should -BeExactly $request.Id

            ReportLogErrors -LogPath $psesServer.LogPath -FromIndex ([ref]$logIdx)
        }
        #
        # As time goes on we add more It's here for the other messages
        #
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That way the order doesn't matter and we can easily add new tests for other messages.

For the future:
I think the Send-LspInitializeRequest will actually probably need to be moved to the BeforeAll eventually once we add other messages that depend on that one having gone through.

We'll get there. Gotta figure out which messages depend on what and align the tests as much as possible to that back and forth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't think there's any particularly obvious approach available at the moment. I think given that we're not planning impending development on this module that we should merge what we have since it works and is less code and then continue as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely don't disagree with you though -- "unit" testing is very bad at sequential logic with dependent states.

I think we should refactor as you say, but will defer that to another PR.

It "Shuts down the process properly" {
$request = Send-LspShutdownRequest -Client $client
$response = Get-LspResponse -Client $client -Id $request.Id
$response.Id | Should -BeExactly $request.Id
$response.Result | Should -BeNull
# TODO: The server seems to stay up waiting for the debug connection
# $psesServer.PsesProcess.HasExited | Should -BeTrue

# We close the process here rather than in an AfterAll
# since errors can occur and we want to test for them.
# Naturally this depends on Pester executing tests in order.

# We also have to dispose of everything properly,
# which means we have to use these cascading try/finally statements
try
{
$psesServer.PsesProcess.Kill()
}
finally
{
try
{
$psesServer.PsesProcess.Dispose()
}
finally
{
$client.Dispose()
}
}

ReportLogErrors -LogPath $psesServer.LogPath -FromIndex ([ref]$logIdx)
}
}
Loading