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 feature flag to allow VS to use the LSP based editor #49996

Merged
merged 32 commits into from
Mar 3, 2021
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
a4d4605
Add feature flag to allow VS to use the LSP based editor
dibarbet Dec 15, 2020
97c54a2
Add integration test support for LSP based editor
dibarbet Dec 17, 2020
a16ad35
Add new integration test pipeline
dibarbet Jan 6, 2021
2a9b7e4
Merge remote-tracking branch 'upstream/master' into local_lsp
dibarbet Jan 6, 2021
f7444f7
Fix type specification
dibarbet Jan 6, 2021
35e35d8
Change parameter types to string to allow variables to be passed in
dibarbet Jan 6, 2021
c10d1cd
Fix relative reference to pipeline template
dibarbet Jan 6, 2021
34d355d
Fix template parameter access
dibarbet Jan 6, 2021
2b0f6a6
Add LSP logs to test run output
dibarbet Jan 7, 2021
8421485
Merge remote-tracking branch 'upstream/master' into local_lsp
dibarbet Jan 7, 2021
ce45c40
Fix tests
dibarbet Jan 8, 2021
1b9d87a
cleanup
dibarbet Jan 15, 2021
cdad03e
test failing test
dibarbet Jan 15, 2021
2a27f8a
Revert "test failing test"
dibarbet Jan 15, 2021
66c87a3
use intpreview instead of preview to ensure the option only appears for
dibarbet Jan 20, 2021
692c6fc
Specify no sync options
dibarbet Jan 20, 2021
20485ba
Merge remote-tracking branch 'upstream/master' into local_lsp
dibarbet Jan 27, 2021
795ab89
Merge remote-tracking branch 'upstream/master' into local_lsp
dibarbet Feb 17, 2021
36d87a6
Workaround breaking change in 16.9p3-p4 in LSP client
dibarbet Feb 17, 2021
e34dbb9
Merge branch 'master' into local_lsp
dibarbet Feb 18, 2021
7ccdf64
move semantic tokens lsp editor check to tagger impl
dibarbet Feb 23, 2021
d0305d9
Update capabilities provided in alwaysactive server
dibarbet Feb 23, 2021
75f8c04
remove workaround now that integration test machines are updated
dibarbet Feb 23, 2021
b3288f4
Add documentation on why there are LSPspecific tests
dibarbet Feb 23, 2021
f58231c
add comment on integration lsp yml
dibarbet Feb 24, 2021
38179be
move check into code action provider
dibarbet Feb 24, 2021
edf1c86
Merge branch 'master' into local_lsp
dibarbet Feb 24, 2021
39423e8
Feedback
dibarbet Mar 2, 2021
a894943
fix test definition
dibarbet Mar 2, 2021
e1fa552
Fact -> WpfFact
dibarbet Mar 2, 2021
68b9691
Fix tests to use window name instead of view name
dibarbet Mar 2, 2021
bdb3aa5
Remove accidentally added fact...
dibarbet Mar 2, 2021
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
31 changes: 31 additions & 0 deletions azure-pipelines-integration-lsp.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# Separate pipeline from normal integration CI to allow branches to change legs

# Branches that trigger a build on commit
dibarbet marked this conversation as resolved.
Show resolved Hide resolved
trigger:
- master
- master-vs-deps
- release/*
- features/*
- demos/*

# Branches that trigger builds on PR
pr:
- master
- master-vs-deps
- release/*
- features/*
- demos/*

jobs:
- job: VS_Integration_LSP
pool:
name: NetCorePublic-Pool
queue: $(queueName)
timeoutInMinutes: 135

steps:
- template: eng/pipelines/test-integration-job.yml
parameters:
configuration: Debug
oop64bit: true
lspEditor: true
39 changes: 5 additions & 34 deletions azure-pipelines-integration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,37 +37,8 @@ jobs:
timeoutInMinutes: 135

steps:
- template: eng/pipelines/checkout-windows-task.yml

- task: PowerShell@2
displayName: Build and Test
inputs:
filePath: eng/build.ps1
arguments: -ci -restore -build -pack -sign -publish -binaryLog -configuration $(_configuration) -prepareMachine -testVsi -oop64bit:$$(_oop64bit) -collectDumps

- task: PublishTestResults@2
displayName: Publish xUnit Test Results
inputs:
testRunner: XUnit
testResultsFiles: $(Build.SourcesDirectory)\artifacts\TestResults\$(_configuration)\*.xml
mergeTestResults: true
testRunTitle: '$(System.JobAttempt)-Integration $(_configuration) OOP64_$(_oop64bit)'
condition: always()

- task: PublishBuildArtifacts@1
displayName: Publish Logs
inputs:
PathtoPublish: '$(Build.SourcesDirectory)\artifacts\log\$(_configuration)'
ArtifactName: '$(System.JobAttempt)-Logs $(_configuration) OOP64_$(_oop64bit) $(Build.BuildNumber)'
publishLocation: Container
continueOnError: true
condition: not(succeeded())

- task: PublishBuildArtifacts@1
displayName: Publish Screenshots
inputs:
PathtoPublish: '$(Build.SourcesDirectory)\artifacts\bin\Microsoft.VisualStudio.LanguageServices.IntegrationTests\$(_configuration)\net472\xUnitResults'
ArtifactName: '$(System.JobAttempt)-Screenshots $(_configuration) OOP64_$(_oop64bit) $(Build.BuildNumber)'
publishLocation: Container
continueOnError: true
condition: not(succeeded())
- template: eng/pipelines/test-integration-job.yml
parameters:
configuration: $(_configuration)
oop64bit: $(_oop64bit)
lspEditor: false
27 changes: 27 additions & 0 deletions eng/build.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ param (
[switch]$warnAsError = $false,
[switch]$sourceBuild = $false,
[switch]$oop64bit = $true,
[switch]$lspEditor = $false,

# official build settings
[string]$officialBuildId = "",
Expand Down Expand Up @@ -385,6 +386,13 @@ function TestUsingRunTests() {
$args += " --sequential"
$args += " --include '\.IntegrationTests'"
$args += " --include 'Microsoft.CodeAnalysis.Workspaces.MSBuild.UnitTests'"

if ($lspEditor) {
$args += " --testfilter FullyQualifiedName~Roslyn.VisualStudio.IntegrationTests.LanguageServerProtocol|Editor=LanguageServerProtocol"
Copy link
Member Author

Choose a reason for hiding this comment

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

I went with both a separate namespace and additional trait name to enable multiple testing scenarios:

  1. Running selected existing integration tests under the LSP editor (uses trait to select which tests).
  2. Writing LSP only integration tests (uses separate namespace).
  3. Ensure that the tests in 2) do not run in normal integration test runs.

Copy link
Member

@jasonmalinowski jasonmalinowski Mar 2, 2021

Choose a reason for hiding this comment

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

I guess the trait is sufficient right, if you just put the trait on containing types? My strong worry about the magic namespace is that we might rename or move something and not realize that tests aren't running anymore. Having the single method with the trait makes it a bit easier to know everything either works or doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

(see other comment though that the only test currently in it's own namespace I think is better just combined with the existing test which checks if it's in LSP mode or not...)

Copy link
Member

Choose a reason for hiding this comment

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

And yes, we once had an integration test CI that was passing because whatever glob was looking for assemblies to run wasn't updated, and so it was running nothing. That was a "fun" discovery.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasonmalinowski If we ever want to support running a test in LSP only context then we must have a separate namespace or something similar. Otherwise I have no way to filter out LSP-only tests from the normal run (since the normal run includes shared tests with the LSP trait).

I can guarantee that there will be LSP only tests, if not gotodef then completion or another feature. I think even for the gotodef test you mention I must have an LSP version and a non-LSP version, even if I share most of the code as they have different input data (window caption).

Copy link
Member

Choose a reason for hiding this comment

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

I guess as long as we're using environment variables to control this, there's no reason you can just haven't a ConditionalFact that only runs if the environment variable is set. You can also have the test just check the environment variable in a test if you need to change a string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! Today I learned about conditional facts. Ok, I'll use that in a followup instead.

}
else {
$args += " --testfilter FullyQualifiedName!~Roslyn.VisualStudio.IntegrationTests.LanguageServerProtocol"
}
}

if (-not $ci -and -not $testVsi) {
Expand Down Expand Up @@ -437,6 +445,24 @@ function TestUsingRunTests() {
} else {
Write-Host "No ServiceHub logs found to copy"
}

if ($lspEditor) {
$lspLogs = Join-Path $TempDir "VisualStudio\LSP"
$telemetryLog = Join-Path $TempDir "VSTelemetryLog"
if (Test-Path $lspLogs) {
Write-Host "Copying LSP logs to $LogDir"
Copy-Item -Path $lspLogs -Destination (Join-Path $LogDir "LSP") -Recurse
} else {
Write-Host "No LSP logs found to copy"
}

if (Test-Path $telemetryLog) {
Write-Host "Copying telemetry logs to $LogDir"
Copy-Item -Path $telemetryLog -Destination (Join-Path $LogDir "Telemetry") -Recurse
} else {
Write-Host "No telemetry logs found to copy"
}
}
}
}
}
Expand Down Expand Up @@ -575,6 +601,7 @@ function Setup-IntegrationTestRun() {
}

$env:ROSLYN_OOP64BIT = "$oop64bit"
$env:ROSLYN_LSPEDITOR = "$lspEditor"
}

function Prepare-TempDir() {
Expand Down
48 changes: 48 additions & 0 deletions eng/pipelines/test-integration-job.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
parameters:
- name: configuration
type: string
default: 'Debug'
- name: oop64bit
# Why string? Parameters are evaluated at compile time, but all variables are strings at compile time.
# So in order to pass a variable in as a parameter here, it must be string.
dibarbet marked this conversation as resolved.
Show resolved Hide resolved
type: string
default: true
- name: lspEditor
type: string
default: false

steps:
- template: checkout-windows-task.yml

- task: PowerShell@2
displayName: Build and Test
inputs:
filePath: eng/build.ps1
arguments: -ci -restore -build -pack -sign -publish -binaryLog -configuration ${{ parameters.configuration }} -prepareMachine -testVsi -oop64bit:$${{ parameters.oop64bit }} -collectDumps -lspEditor:$${{ parameters.lspEditor }}

- task: PublishTestResults@2
displayName: Publish xUnit Test Results
inputs:
testRunner: XUnit
testResultsFiles: $(Build.SourcesDirectory)\artifacts\TestResults\${{ parameters.configuration }}\*.xml
mergeTestResults: true
testRunTitle: '$(System.JobAttempt)-Integration ${{ parameters.configuration }} OOP64_${{ parameters.oop64bit }}'
condition: always()

- task: PublishBuildArtifacts@1
displayName: Publish Logs
inputs:
PathtoPublish: '$(Build.SourcesDirectory)\artifacts\log\${{ parameters.configuration }}'
ArtifactName: '$(System.JobAttempt)-Logs ${{ parameters.configuration }} OOP64_${{ parameters.oop64bit }} LspEditor_${{ parameters.lspEditor }} $(Build.BuildNumber)'
publishLocation: Container
continueOnError: true
condition: not(succeeded())

- task: PublishBuildArtifacts@1
displayName: Publish Screenshots
inputs:
PathtoPublish: '$(Build.SourcesDirectory)\artifacts\bin\Microsoft.VisualStudio.LanguageServices.IntegrationTests\${{ parameters.configuration }}\net472\xUnitResults'
ArtifactName: '$(System.JobAttempt)-Screenshots ${{ parameters.configuration }} OOP64_${{ parameters.oop64bit }} LspEditor_${{ parameters.lspEditor }} $(Build.BuildNumber)'
publishLocation: Container
continueOnError: true
condition: not(succeeded())
1 change: 1 addition & 0 deletions src/Compilers/Test/Core/Traits/Traits.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ public static class Editors
public const string KeyProcessors = nameof(KeyProcessors);
Copy link
Member

Choose a reason for hiding this comment

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

fyi, i ignored all the yaml/ci stuff. someone else will have to review that.

public const string KeyProcessorProviders = nameof(KeyProcessorProviders);
public const string Preview = nameof(Preview);
public const string LanguageServerProtocol = nameof(LanguageServerProtocol);
}

public const string Feature = nameof(Feature);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Microsoft.CodeAnalysis.CodeRefactorings;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Editor.Host;
using Microsoft.CodeAnalysis.Editor.Shared.Extensions;
using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
using Microsoft.CodeAnalysis.Editor.Tags;
using Microsoft.CodeAnalysis.Host.Mef;
Expand Down Expand Up @@ -76,11 +77,18 @@ public SuggestedActionsSourceProvider(
ImageMonikerServices = ExtensionOrderer.Order(imageMonikerServices).ToImmutableArray();
}

public ISuggestedActionsSource CreateSuggestedActionsSource(ITextView textView, ITextBuffer textBuffer)
public ISuggestedActionsSource? CreateSuggestedActionsSource(ITextView textView, ITextBuffer textBuffer)
{
Contract.ThrowIfNull(textView);
Contract.ThrowIfNull(textBuffer);

// Disable lightbulb points when running under the LSP editor.
// The LSP client will interface with the editor to display our code actions.
if (textBuffer.IsInLspEditorContext())
{
return null;
}

return new SuggestedActionsSource(_threadingContext, this, textView, textBuffer, _suggestedActionCategoryRegistry);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Classification;
using Microsoft.CodeAnalysis.Editor.Shared.Extensions;
using Microsoft.CodeAnalysis.Editor.Shared.Tagging;
using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
using Microsoft.CodeAnalysis.Editor.Tagging;
Expand Down Expand Up @@ -139,6 +140,13 @@ public IEnumerable<ITagSpan<IClassificationTag>> GetAllTags(NormalizedSnapshotSp
var snapshot = firstSpan.Snapshot;
Debug.Assert(snapshot.TextBuffer == _subjectBuffer);

// The LSP client will handle producing tags when running under the LSP editor.
// Our tagger implementation should return nothing to prevent conflicts.
if (snapshot.TextBuffer.IsInLspEditorContext())
dibarbet marked this conversation as resolved.
Show resolved Hide resolved
{
return Array.Empty<ITagSpan<IClassificationTag>>();
}

// We want to classify from the start of the first requested span to the end of the
// last requested span.
var spanToTag = new SnapshotSpan(snapshot,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,14 @@ protected override Task ProduceTagsAsync(TaggerContext<IClassificationTag> conte
return Task.CompletedTask;
}

// The LSP client will handle producing tags when running under the LSP editor.
// Our tagger implementation should return nothing to prevent conflicts.
var workspaceContextService = document?.Project.Solution.Workspace.Services.GetRequiredService<IWorkspaceContextService>();
if (workspaceContextService?.IsInLspEditorContext() == true)
{
return Task.CompletedTask;
}

return SemanticClassificationUtilities.ProduceTagsAsync(context, spanToTag, classificationService, _typeMap);
}
}
Expand Down
8 changes: 3 additions & 5 deletions src/Tools/Source/RunTests/ITestExecutor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,16 @@ internal readonly struct TestExecutionOptions
internal string DotnetFilePath { get; }
internal ProcDumpInfo? ProcDumpInfo { get; }
internal string TestResultsDirectory { get; }
internal string? Trait { get; }
internal string? NoTrait { get; }
internal string? TestFilter { get; }
Copy link
Member Author

Choose a reason for hiding this comment

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

As best as I could tell, the trait and notrait properties did not work properly with dotnet test. They would pass an argument like Trait=LanguageServerProtocol, but per https://docs.microsoft.com/en-us/dotnet/core/testing/selective-unit-tests?pivots=xunit that isn't the correct format. So I modified these to be able to just pass in the xunit test filter (which also allowed me to pass in fully qualified names)

Copy link
Member

Choose a reason for hiding this comment

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

is it possible to extract this out into anotehr pR?

internal bool IncludeHtml { get; }
internal bool Retry { get; }

internal TestExecutionOptions(string dotnetFilePath, ProcDumpInfo? procDumpInfo, string testResultsDirectory, string? trait, string? noTrait, bool includeHtml, bool retry)
internal TestExecutionOptions(string dotnetFilePath, ProcDumpInfo? procDumpInfo, string testResultsDirectory, string? testFilter, bool includeHtml, bool retry)
{
DotnetFilePath = dotnetFilePath;
ProcDumpInfo = procDumpInfo;
TestResultsDirectory = testResultsDirectory;
Trait = trait;
NoTrait = noTrait;
TestFilter = testFilter;
IncludeHtml = includeHtml;
Retry = retry;
}
Expand Down
18 changes: 5 additions & 13 deletions src/Tools/Source/RunTests/Options.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,9 @@ internal class Options
public Display Display { get; set; }

/// <summary>
/// Trait string to pass to xunit.
/// Filter string to pass to xunit.
/// </summary>
public string? Trait { get; set; }

/// <summary>
/// The no-trait string to pass to xunit.
/// </summary>
public string? NoTrait { get; set; }
public string? TestFilter { get; set; }

public string Configuration { get; set; }

Expand Down Expand Up @@ -137,8 +132,7 @@ public Options(
var helix = false;
var helixQueueName = "Windows.10.Amd64.Open";
var retry = false;
string? traits = null;
string? noTraits = null;
string? testFilter = null;
int? timeout = null;
string? resultFileDirectory = null;
string? logFileDirectory = null;
Expand All @@ -158,8 +152,7 @@ public Options(
{ "sequential", "Run tests sequentially", o => sequential = o is object },
{ "helix", "Run tests on Helix", o => helix = o is object },
{ "helixQueueName=", "Name of the Helix queue to run tests on", (string s) => helixQueueName = s },
{ "traits=", "xUnit traits to include (semicolon delimited)", (string s) => traits = s },
{ "notraits=", "xUnit traits to exclude (semicolon delimited)", (string s) => noTraits = s },
{ "testfilter=", "xUnit string to pass to --filter, e.g. FullyQualifiedName~TestClass1|Category=CategoryA", (string s) => testFilter = s },
{ "timeout=", "Minute timeout to limit the tests to", (int i) => timeout = i },
{ "out=", "Test result file directory (when running on Helix, this is relative to the Helix work item directory)", (string s) => resultFileDirectory = s },
{ "logs=", "Log file directory (when running on Helix, this is relative to the Helix work item directory)", (string s) => logFileDirectory = s },
Expand Down Expand Up @@ -241,8 +234,7 @@ public Options(
UseHelix = helix,
HelixQueueName = helixQueueName,
IncludeHtml = includeHtml,
Trait = traits,
NoTrait = noTraits,
TestFilter = testFilter,
Timeout = timeout is { } t ? TimeSpan.FromMinutes(t) : null,
Retry = retry,
};
Expand Down
20 changes: 4 additions & 16 deletions src/Tools/Source/RunTests/ProcessTestExecutor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public string GetCommandLineArguments(AssemblyInfo assemblyInfo, bool useSingleQ
builder.Append($@"test");
builder.Append($@" {sep}{assemblyInfo.AssemblyName}{sep}");
var typeInfoList = assemblyInfo.PartitionInfo.TypeInfoList;
if (typeInfoList.Length > 0 || !string.IsNullOrWhiteSpace(Options.Trait) || !string.IsNullOrWhiteSpace(Options.NoTrait))
if (typeInfoList.Length > 0 || !string.IsNullOrWhiteSpace(Options.TestFilter))
{
builder.Append($@" --filter {sep}");
var any = false;
Expand All @@ -50,22 +50,10 @@ public string GetCommandLineArguments(AssemblyInfo assemblyInfo, bool useSingleQ
}
builder.Append(sep);

if (Options.Trait is object)
if (Options.TestFilter is object)
{
foreach (var trait in Options.Trait.Split(new char[] { ';' }, StringSplitOptions.RemoveEmptyEntries))
{
MaybeAddSeparator();
builder.Append($"Trait={trait}");
}
}

if (Options.NoTrait is object)
{
foreach (var trait in Options.NoTrait.Split(new char[] { ';' }, StringSplitOptions.RemoveEmptyEntries))
{
MaybeAddSeparator('&');
builder.Append($"Trait!~{trait}");
}
MaybeAddSeparator();
builder.Append(Options.TestFilter);
}

void MaybeAddSeparator(char separator = '|')
Expand Down
4 changes: 1 addition & 3 deletions src/Tools/Source/RunTests/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ internal static async Task<int> Main(string[] args)
{
Logger.Log("RunTest command line");
Logger.Log(string.Join(" ", args));

var options = Options.Parse(args);
if (options == null)
{
Expand Down Expand Up @@ -372,8 +371,7 @@ private static ProcessTestExecutor CreateTestExecutor(Options options)
dotnetFilePath: options.DotnetFilePath,
procDumpInfo: options.CollectDumps ? GetProcDumpInfo(options) : null,
testResultsDirectory: options.TestResultsDirectory,
trait: options.Trait,
noTrait: options.NoTrait,
testFilter: options.TestFilter,
includeHtml: options.IncludeHtml,
retry: options.Retry);
Copy link
Member

Choose a reason for hiding this comment

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

i skipped all this.

return new ProcessTestExecutor(testExecutionOptions);
Expand Down
Loading