Setting to Disable Pester Code Lens

[JustinGrote]

Allows the Pester Codelens to be disabled so as not to confuse users who want to use an alternate Pester Test Runner

Correlated vscode-powershell PR: PowerShell/vscode-powershell#3613

[JustinGrote]

@andschwa not sure why this is failing the test, something in the way you mock the settings maybe?

[andyleejordan]

Is there a way to add a test checking that the provider is (and isn't) enabled via the flag? We might not actually have tests around settings yet, but I'm trying to be intentional about covering all new changes in tests.

[andyleejordan]

@andschwa not sure why this is failing the test, something in the way you mock the settings maybe?

It looks like the actual test itself wasn't fixed. At a quick glance, there's an assertion failing. The input was changed, but not the tests on the output.

[JustinGrote]

@andschwa That's the thing though, by default it shouldn't be changing the behavior, I didn't write a new test yet, so I'm not sure why it thinks it's not registering the codelens.

[andyleejordan]

Oh I see, we do have an existing test that asserts the code lens is there (yay!):

PowerShellEditorServices/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

Lines 747 to 825 in f9bde73

	CodeLensContainer codeLenses = await PsesLanguageClient
	.SendRequest<CodeLensParams>(
	"textDocument/codeLens",
	new CodeLensParams
	{
	TextDocument = new TextDocumentIdentifier
	{
	Uri = new Uri(filePath)
	}
	})
	.Returning<CodeLensContainer>(CancellationToken.None).ConfigureAwait(false);
	Assert.Collection(codeLenses,
	codeLens =>
	{
	Range range = codeLens.Range;
	Assert.Equal(1, range.Start.Line);
	Assert.Equal(0, range.Start.Character);
	Assert.Equal(7, range.End.Line);
	Assert.Equal(1, range.End.Character);
	Assert.Equal("Run tests", codeLens.Command.Title);
	},
	codeLens =>
	{
	Range range = codeLens.Range;
	Assert.Equal(1, range.Start.Line);
	Assert.Equal(0, range.Start.Character);
	Assert.Equal(7, range.End.Line);
	Assert.Equal(1, range.End.Character);
	Assert.Equal("Debug tests", codeLens.Command.Title);
	},
	codeLens =>
	{
	Range range = codeLens.Range;
	Assert.Equal(2, range.Start.Line);
	Assert.Equal(4, range.Start.Character);
	Assert.Equal(6, range.End.Line);
	Assert.Equal(5, range.End.Character);
	Assert.Equal("Run tests", codeLens.Command.Title);
	},
	codeLens =>
	{
	Range range = codeLens.Range;
	Assert.Equal(2, range.Start.Line);
	Assert.Equal(4, range.Start.Character);
	Assert.Equal(6, range.End.Line);
	Assert.Equal(5, range.End.Character);
	Assert.Equal("Debug tests", codeLens.Command.Title);
	},
	codeLens =>
	{
	Range range = codeLens.Range;
	Assert.Equal(3, range.Start.Line);
	Assert.Equal(8, range.Start.Character);
	Assert.Equal(5, range.End.Line);
	Assert.Equal(9, range.End.Character);
	Assert.Equal("Run test", codeLens.Command.Title);
	},
	codeLens =>
	{
	Range range = codeLens.Range;
	Assert.Equal(3, range.Start.Line);
	Assert.Equal(8, range.Start.Character);
	Assert.Equal(5, range.End.Line);
	Assert.Equal(9, range.End.Character);
	Assert.Equal("Debug test", codeLens.Command.Title);
	});

And it looks like the way its written is making for a really bad failure message, but in short from here:

PowerShellEditorServices/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

Line 759 in f9bde73

Assert.Collection(codeLenses,

I think the collection is empty, which means that this failed:

PowerShellEditorServices/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

Lines 747 to 757 in f9bde73

	CodeLensContainer codeLenses = await PsesLanguageClient
	.SendRequest<CodeLensParams>(
	"textDocument/codeLens",
	new CodeLensParams
	{
	TextDocument = new TextDocumentIdentifier
	{
	Uri = new Uri(filePath)
	}
	})
	.Returning<CodeLensContainer>(CancellationToken.None).ConfigureAwait(false);

[JustinGrote]

Thanks for eyeballing it, I'll investigate but again it shouldn't have changed the default behavior so either I goofed or something wrong in the test.

[andyleejordan]

Looking at the rest of the settings file, the Pester class is weird:

PowerShellEditorServices/src/PowerShellEditorServices/Services/Workspace/LanguageServerSettings.cs

Lines 385 to 389 in f9bde73

	public class PesterSettings
	{
	public PesterSettings()
	{
	}

This should be:

    internal class PesterSettings
    {

That is, none of the others are public just internal, and this empty constructor is useless. Probably not breaking anything but want to change it while we're in this code?

[andyleejordan]

Two other things to note is that in the tests, the test you're having to touch is unnecessarily (I think?) parsing JSON in a string into a Settings object, but there are other places in these tests that are just creating the Settings directly. That would at least help make the test more readable if you wanted to mirror that way of doing it:

PowerShellEditorServices/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

Lines 251 to 266 in f9bde73

	PsesLanguageClient.SendNotification("workspace/didChangeConfiguration",
	new DidChangeConfigurationParams
	{
	Settings = JToken.FromObject(new LanguageServerSettingsWrapper
	{
	Files = new EditorFileSettings(),
	Search = new EditorSearchSettings(),
	Powershell = new LanguageServerSettings
	{
	ScriptAnalysis = new ScriptAnalysisSettings
	{
	Enable = false
	}
	}
	})
	});

The other thing in here is that the test I just linked is explicitly sending a workspace/didChangeConfiguration notification (and then waiting a bit) instead of calling PsesLanguageClient.Workspace.DidChangeConfiguration. I actually think the latter makes more sense because it removes having to wait, but it's two avenues to dig into and see what's happening to the failing test.

[JustinGrote]

Looking at the rest of the settings file, the Pester class is weird:

PowerShellEditorServices/src/PowerShellEditorServices/Services/Workspace/LanguageServerSettings.cs

Lines 385 to 389 in f9bde73

	public class PesterSettings
	{
	public PesterSettings()
	{
	}

This should be:

    internal class PesterSettings
    {

That is, none of the others are public just internal, and this empty constructor is useless. Probably not breaking anything but want to change it while we're in this code?

I agree I thought this was super bizarre but with lack of comments as to why it was this way I assumed there was a good reason.

[andyleejordan]

I agree I thought this was super bizarre but with lack of comments as to why it was this way I assumed there was a good reason.

I checked the Git blame and saw no good reason. Looked like the code was added in a community PR and so was probably just checked as thoroughly as we're doing now.

[andyleejordan]

Looks like it's working now. Can we add a simple test that asserts thing work as expected with Pester.CodeLens = false? Also would be happy to have included the change to the Pester settings class that has a dummy constructor and is for some reason public.

[JustinGrote]

@andschwa I'll try to work on this tonight or tomorrow, no guarantees though :)

[JustinGrote]

@andschwa banged on this for a while, everything looks fine after making some additional adjustments, but the Pester Code lens test fails for me even on master with the same error, can you confirm it fails on master for you too?

[andyleejordan]

@andschwa banged on this for a while, everything looks fine after making some additional adjustments, but the Pester Code lens test fails for me even on master with the same error, can you confirm it fails on master for you too?

It certainly shouldn't be! Here's the most recent master CI run and it's passing: https://dev.azure.com/powershell/PowerShellEditorServices/_build/results?buildId=87315&view=ms.vss-test-web.build-test-results-tab&runId=1676954&resultId=100001&paneView=debug

Sounds like an environment issue?

[JustinGrote]

@andschwa OK now that I've got both-sides debugging working, it looks like the config change via DidChangeConfiguration is not taking effect and that's why the legacyCodeLens test is failing

[JustinGrote]

@andschwa should pass but it's in "awaiting approval" status for the MacOS build.

[andyleejordan]

Hey, this looks fantastic, I'm excited to get this into a preview. @rjmholt Do you have a moment to review?

[JustinGrote]

@rjmholt @andschwa thank you! And thanks for putting up with my low level of C# experience.

[rjmholt]

So I'm late back to this, but I thought it looked fishy when I reviewed it...

I think we want to use ITestOutputHelper for this

[JustinGrote]

The intent is to have a indicator for when I breakpoint right past this which process to attach to for dual-debugging, would the ITestOutputHelper still output that to the vscode debug console at that point?