Skip to content

Send Pester describe line and info whether Pester 4.6.0 is installed to PowerShell.RunPesterTests command #856

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.

Sign up for GitHub

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

Closed
wants to merge 16 commits into from
Closed

Send Pester describe line and info whether Pester 4.6.0 is installed to PowerShell.RunPesterTests command #856

Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f26755f
Revert "Fix #827 Pester TestName w/expandable str returns nothing (#8…
bergmeister Jan 20, 2019
a034574
Revert "Revert "Fix #827 Pester TestName w/expandable str returns not…
bergmeister Jan 20, 2019
c978a6f
send describe block line and info whether pester 4.6.0 is available t…
bergmeister Jan 22, 2019
725f643
send only describeBlockLineNumber as nullable int over the wire to PS…
bergmeister Jan 22, 2019
62b101b
use clearer way of passing in null when int is nullable
bergmeister Jan 23, 2019
b6b4b4c
address stylistic nitpicks by Patrick
bergmeister Jan 24, 2019
dbd42b5
Use lazy initialisation to query pester version only when it is neede…
bergmeister Jan 26, 2019
61f62f5
Merge branch 'master' of https://github.com/PowerShell/PowerShellEdit…
bergmeister Jan 29, 2019
ba32b20
use pscommand instead
bergmeister Jan 29, 2019
565f2f8
Merge branch 'master' of https://github.com/PowerShell/PowerShellEdit…
bergmeister Feb 1, 2019
32c69aa
Update src/PowerShellEditorServices.Host/CodeLens/PesterCodeLensProvi…
rjmholt Feb 8, 2019
69e7260
Merge branch 'PesterDescribeLine' of https://github.com/bergmeister/P…
bergmeister Feb 9, 2019
e5bddd0
adress style PR comments
bergmeister Feb 9, 2019
39290a2
Merge branch 'master' of https://github.com/PowerShell/PowerShellEdit…
bergmeister Feb 19, 2019
7245bd6
Use Get-Command and make calls to GetPesterCodeLens async/await to th…
bergmeister Feb 20, 2019
4ec3b3f
Oops, I did it again...
bergmeister Feb 20, 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
46 changes: 44 additions & 2 deletions src/PowerShellEditorServices.Host/CodeLens/PesterCodeLensProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Management.Automation;
using System.Threading;
using System.Threading.Tasks;

Expand All @@ -25,6 +26,12 @@ internal class PesterCodeLensProvider : FeatureProviderBase, ICodeLensProvider
/// </summary>
private IDocumentSymbolProvider _symbolProvider;

/// <summary>
/// Pester 4.6.0 introduced a new ScriptblockFilter parameter to be able to run a test based on a line,
/// therefore knowing this information is important.
/// </summary>
private readonly Lazy<bool> _pesterV4_6_0_OrHigherAvailable;

/// <summary>
/// Create a new Pester CodeLens provider for a given editor session.
/// </summary>
Expand All @@ -33,6 +40,7 @@ public PesterCodeLensProvider(EditorSession editorSession)
{
_editorSession = editorSession;
_symbolProvider = new PesterDocumentSymbolProvider();
_pesterV4_6_0_OrHigherAvailable = new Lazy<bool>(() => DeterminePesterVersion());
}

/// <summary>
Expand All @@ -45,6 +53,9 @@ private CodeLens[] GetPesterLens(
PesterSymbolReference pesterSymbol,
ScriptFile scriptFile)
{
// A value of null is a signal to PSES that the available Pester version does not support
// running Describe blocks by name (the test name will used instead then).
int? describeBlockLineNumber = _pesterV4_6_0_OrHigherAvailable.Value ? (int?)pesterSymbol.ScriptRegion.StartLineNumber : null;
var codeLensResults = new CodeLens[]
{
new CodeLens(
Expand All @@ -54,7 +65,7 @@ private CodeLens[] GetPesterLens(
new ClientCommand(
"PowerShell.RunPesterTests",
"Run tests",
new object[] { scriptFile.ClientFilePath, false /* No debug */, pesterSymbol.TestName })),
new object[] { scriptFile.ClientFilePath, false /* No debug */, pesterSymbol.TestName, describeBlockLineNumber })),

new CodeLens(
this,
Expand All @@ -63,12 +74,43 @@ private CodeLens[] GetPesterLens(
new ClientCommand(
"PowerShell.RunPesterTests",
"Debug tests",
new object[] { scriptFile.ClientFilePath, true /* Run in debugger */, pesterSymbol.TestName })),
new object[] { scriptFile.ClientFilePath, true /* Run in debugger */, pesterSymbol.TestName, describeBlockLineNumber })),
};

return codeLensResults;
}

/// <summary>
/// Used to determine the value of <see cref="_pesterV4_6_0_OrHigherAvailable"/> as a background task.
/// </summary>
private bool DeterminePesterVersion()
{
var powerShell = new PSCommand()
.AddCommand("Get-Module")
.AddParameter("ListAvailable")
.AddParameter("Name", "Pester");

IEnumerable<PSObject> result = Task.Run(() =>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you're using .Result instead of using async/await?

Copy link
Contributor Author

@bergmeister bergmeister Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

To make it synchronous since I moved it to the last possible point (when a .tests.ps1 file with a Describe block gets opened). If I did not call .Result (i.e. make it async) then it would default to the old Pester syntax in the first 1-2 seconds until the value has been determined (which people did not like in the first version of the PR). It is still using Task APIs because the PSCommand APIs that were suggested are all async.

{
return _editorSession.PowerShellContext.ExecuteCommandAsync<PSObject>(powerShell);
}).Result;

if (result == null)
{
return false;
}

var minimumPesterVersionSupportingInlineInvocation = new Version(4, 6);
foreach (PSObject module in result)
Copy link
Member

Choose a reason for hiding this comment

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

What I'm worried about here is...

Lets look at the PSModulePath:

/Users/tyler/.local/share/powershell/Modules                       # user level
/usr/local/share/powershell/Modules                                # all users level
/usr/local/microsoft/powershell/6-preview/Modules                  # internal

let's say they have version 4.5 in their user module path ( but 4.6 in their allusers module path

This function will return true but 4.5 is what will get imported when doing:

Import-Module Pester
or during module auto-loading

I think we should only check the first item in the result. This should be the Pester that would get imported.

Copy link
Contributor Author

@bergmeister bergmeister Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Is that really the behaviour of PowerShell to not use the latest available version? Can you please point to some docs or code for my own education. How do you know that just looking at the first item is enough? Get-Module really needs to be improved otherwise to be more useful/predictive...
If that is really the case, then should we not rather call (Get-Command Invoke-pester).Version instead (which has the drawback of being 3 times more expensive for some reason...)?

Copy link
Member

Choose a reason for hiding this comment

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

Yes that's the behavior of PowerShell. I'm unaware of any docs out in the wild (cc @SteveL-MSFT or @joeyaiello maybe?).

The PSModulePath acts the same way that the PATH environment variable does - it goes down the path looking for the command you want to run, then executes the first one it finds.

A simple test:

→ Install-Module Pester -Scope AllUsers -Force

→ Install-Module Pester -Scope CurrentUser -Force -MaximumVersion 4.5

→ Get-Command Invoke-Pester

CommandType     Name                                               Version    Source
-----------     ----                                               -------    ------
Function        Invoke-Pester                                      4.5.0      Pester

→ Get-Module -ListAvailable Pester


    Directory: /Users/tyler/.local/share/powershell/Modules

ModuleType Version    Name                                PSEdition ExportedCommands
---------- -------    ----                                --------- ----------------
Script     4.5.0      Pester                              Desk      {Describe, Context, It, Should…}

    Directory: /usr/local/share/powershell/Modules

ModuleType Version    Name                                PSEdition ExportedCommands
---------- -------    ----                                --------- ----------------
Script     4.6.0      Pester                              Desk      {Describe, Context, It, Should…}

Copy link
Member

Choose a reason for hiding this comment

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

How do you know that just looking at the first item is enough?

Because when you do Get-Module the first item written to the pipeline is first on the PSModulePath. That might not be correct though... Get-Command is probably better for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yes, this sounds like Get-Command is probably the better option. I guess we win the investment back later when Invoke-Pester actually gets called. Personally I would keep it in a non-blocking task though and just have it behave like the old system in the first 2 seconds of opening a Pester file...

Copy link
Member

@TylerLeonhardt TylerLeonhardt Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah... can @rjmholt and @SeeminglyScience help me understand why this should be synchronous? I would think non-blocking would be better here.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT the bulk of the Pester code lens implementation is sync (other than LSP message reading/writing). This code is a bit old so maybe we weren't doing as much async then (??) or it could be that the specific symbol we are looking for results in a "fast enough" AST search.

One issue I have with this approach is that it doesn't take into account that I might have already imported an older version of Pester e.g. Import-Module Pester -RequiredVersion 4.1.1. So we should check Get-Module Pester first to see if the module is already loaded before getting the list of Pester module.

Another thing to consider is this feature seems to be implemented before PS Core was shipped. That is, Windows PowerShell comes with Pester but not PS Core - right? But this feature is still enabled resulting in this user exp:

Invoke-Pester : The term 'Invoke-Pester' is not recognized as the name of a cmdlet, function, script file, or operable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

Perhaps we should consider shipping Pester with the extension like we do with PSSA? That would ensure this feature works on a vanilla PS Core install. This would ensure we have loaded a minimum version of Pester - like we do for PSSA. But we still allow folks to install later versions of Pester and those would get loaded. Again, very similiar to how we load PSSA.

Copy link
Member

Choose a reason for hiding this comment

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

@rkeithhill you touch on a lot of great points.

  1. I think we should make this Async. Lets take the Lazy<bool> and turn it into a Lazy<Task<bool>> and lazily await the value. Shouldn't be too hard to plumb that. The original caller of this function is async already.

we should check Get-Module Pester first to see if the module is already loaded before getting the list of Pester module

This makes sense. But Get-Command Invoke-Pester should cover this if the module is already imported, right? Get-Command Invoke-Pester should be all we need, I think.

Perhaps we should consider shipping Pester with the extension like we do with PSSA?

Some people rely on specific version of Pester. PowerShell itself relied on a specific version of Pester for a while in preparation for Core support from Pester.

I don't think we can add Pester into PSES and grab the latest version on the PSModulePath because I don't want to hurt customers by forcing a newer version of Pester on them and all of a sudden their CI and their local are different.

PSSA does fall in that camp, a bit... but PSSA is more for local analysis than anything else, I feel.

Copy link
Contributor Author

@bergmeister bergmeister Feb 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@TylerLeonhardt I agree with point 2 and 3. Regarding point 2, we could potentially offer to install the latest version of Pester as a 3rd option in the dialog, but I see this as an optional, nice to have feature that can happen after this PR.

Regarding point 1: I am not sure if we are on the same page: At the moment, when GetPesterLens gets called (which is when a .tests.ps1 file gets opened and is therefore as late as possible), then we need to know the version of Pester already and have to wait for the result. When I first opened the PR I ran the check in a background thread (Task.Run without .Result), which meant that until it comes back, it reports false (which is what people criticised at first). I can change it back to this, but I do not need to make it an Lazy<Task<bool>> for that, I'd just remove the blocking call to .Result. Therefore it does not matter at all if the function that we call is already async or not, the reason why I have to use Task.Run in the first place is because the available Execute APIs on the recommended PSCommand class only have async methods,
If you really wanted true async, then we'd have to fix calling methods up to 3 layers above to make it async top to bottom (which is quite a bit more involved for this change...), otherwise you wouldn't get any real benefit from it and it.

Copy link
Contributor Author

@bergmeister bergmeister Feb 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I made it use Get-Command now, which makes the code lens appear a second later due to it being more expensive and I also implemented async-await all the way to the top in a minimum viable way. Can you please re-review?

{
if (module.BaseObject is PSModuleInfo psModuleInfo && psModuleInfo.Version >= minimumPesterVersionSupportingInlineInvocation)
{
return true;
}
}
return false;
}

/// <summary>
/// Get all Pester CodeLenses for a given script file.
/// </summary>
Expand Down