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

[bergmeister]

This is the 1st PR needed as a pre-requisite of the implementation of PowerShell/vscode-powershell#1710
The extension needs to know whether Pester 4.6.0 is available and the line number of the describe to be able to call new Pester syntax that will enable to execute a describe block by line number, therefore enabling the run of describe blocks with an interpolated string.
The work to determine the version is only done once in a background job and once it has finished, it will be cached. Is there a better way to persist such information to the file system if Pester >4.6.0 was available (which is unlikely to change)
Note that this change does not break the vs-code extension because we only ADD arguments

[rkeithhill]

LGTM other than minor default(int?) nit.

[SeeminglyScience]

As long as @rjmholt and @rkeithhill comments get resolved, LGTM. A few style nits as well.

Thanks for adding this @bergmeister!

[bergmeister]

Ok, I did some analysis/debugging and it seems that PesterCodeLensProvider gets constructed when opening any .ps1 file. Although it would be nice to pre-initialise this in the background, I removed the async task from the constructor (I was thinking if we start adding more of that stuff, vscode will soon turn into a startup monster like Visual Studio). Therefore I changed it to a lazy evaluated and cached property that gets called in GetPesterLens, which will only get called once someone has a Pester tests file with a describe block. The Codacy error is a red herring because the tool does not seem to be aware of new C# syntax.

[rjmholt]

LGTM! Left a couple of suggestions

[bergmeister]

I have replied and reacted to the PR comments. I think this PR is ready to be merged now, since its parent PR in the extension is now also approved as well. The Codacy issue is a bug in the tool itself where it does not recognize C# 7 syntax

[TylerLeonhardt]

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

[bergmeister]

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.

[TylerLeonhardt]

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.

[bergmeister]

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...)?

[TylerLeonhardt]

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…}

[TylerLeonhardt]

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.

[bergmeister]

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...

[TylerLeonhardt]

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

[rkeithhill]

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.

[TylerLeonhardt]

@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.

[bergmeister]

@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.

[bergmeister]

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?

[TylerLeonhardt]

You made this function async, but didn't add any await statements so async isn't needed here.

[bergmeister]

I had to change it here as well due to the signature change in the interface. The PesterCodeLensProvider properly implements async-await, which is what counts

[TylerLeonhardt]

Interesting! Typically the compiler yells if you have an async but no await but I guess since the interface requires it, the compiler doesn't yell.

[bergmeister]

Yep, it emits a warning though that this function won't run async as expected

[TylerLeonhardt]

You could changed the method to return:

return await Task.FromResult(acc.ToArray());

to make it happy I think.

[bergmeister]

I don't think this is a good idea because you might as well just suppress the warning. The compiler is happy, it is just kind to let us know that this other implementation will actually run synchronously despite the async declaration, Task.FromResult will still make it run sync but add unnecessary overhead. The warning is good to have because it is a way of tracking possible future improvement and also letting methods being called from this method know that they will actually need to adapt on a higher level if someone made them truly async as well

[TylerLeonhardt]

@bergmeister just so I'm 100% clear. This PR tests if the Describe block is valid to show CodeLens at the file open time?

Is there any reason for doing that over just displaying the CodeLens for all Describe blocks and then show errors (or better recommendations saying "go get 4.6" 😄 ) for unsupported ones accordingly?

[bergmeister]

@TylerLeonhardt It is about sending additional information to the extension so that it knows how to call invoke-pester
As far as I understand:
When returning the code lens, it will show in the editor. Somehow, showing the code lens also requires providing the information about the line number and the boolean if Pester is compatible upfront, which is information being sent to the vscode extension. From a high level, yes, you'd want to separate the showing of the code lens from gathering the information last minute when someone actually clicks on it, but this would require a re-architecture.
At the moment, the typescript code knows all this information already when the user clicks on the test button. We'd need to send a request to PSES to gather the compatibility information last minute. Can someone comment on how much involved this would be? WDYT @rkeithhill ?
P.S. I'll be at the MVP summit in March if you guys want to have a chat in person or hack on it together?

[rkeithhill]

I propose we close this in favor of PR #873