'Run tests|Debug tests' Pester code lens does not show when Describe block is interpolated string

[bergmeister]

I sometimes include test details in the describe block that are sometimes only known at runtime, however, the Pester code lens does not show any more once I use an interpolated string in the Describe block.
Given the following Pester file (foo.tests.ps1):

$version = $PSVersionTable.PSVersion.Major
Describe "foo on PSVersion $version" {
	It "bar $version" {

	}
}

The Run tests|Debug tests code lens does not show, I see that under the hood, the extension creates parameters to Invoke-Pester that look like -PesterOption @{IncludeVSCodeMarker=$true} -TestName 'foo', in this case, the extension would either need to evaluate the interpolated string or just get rid of the -TestName parameter.

[rjmholt]

Evaluating the string will either be tricky or dangerous, so I think if we can pass this off to Pester we should.

[rkeithhill]

What if we created the command line using double quotes e.g. -PesterOption @{IncludeVSCodeMarker=$true} -TestName "foo on PSVersion $version"? Or maybe, if we can pull the string from the Describe block, it would be better to use that string quoting. That way if you used single quotes to prevent interpolation, we'd honor that.

[bergmeister]

Interestingly the single quote is by design, which this issue documents as it used to be double quotes but was not working for interpolated strings in special scenarios where ToString() was resulting in something that Pester could not understand, maybe this is also an issue with Pester...
Maybe the better solution is to just run Invoke-Pester without the -TestName (i.e. on the whole file) if the Describe string contains a dollar sign

[rkeithhill]

@SeeminglyScience and I had a discussion on the Slack where I think we came to the conclusion there is no good way to support interpolated strings. Many times the variables used in the TestName interpolated string are defined in the test file. However, we must provide a string to Invoke-Pester -TestName that gets executed in the PS integrated console. So we have a catch 22, the variable doesn't get defined until the test executes, but we can't successfully execute the test by name because the variable is not defined yet.

I think the best we can do here is change the code lens for the Describe block to offer to run all tests in the file. This would probably be better than silently not offering anything.

[bergmeister]

Yes, I agree to just omit the TestName parameter if and only if the interpolated string contains an expression to be evaluated, I guess just checking that it contains a dollar sign should be enough to detect that.
I'd like to give it a try to do that myself please since I recently figured out how to build and install the extension myself and I'd like to get some more practice with typescript

[SeeminglyScience]

I'm still very uneasy about the idea of some of the code lenses running all tests and some running only the nearest describe block. I'd much rather see a descriptive error message explaining that the test name should be static.

It's probably not a huge deal for just describes, but ideally we'd support a code lens for every it block. I believe @nohwnd mentioned filtering by it block names is potentially coming in the next major release.

[nohwnd]

Yes, there is v5-alpha1 since yesterday that can discover tests and filter them by It, and only run appropriate setups and teardowns, so it should be ideal for running tests selectively.

It already can filter by path, you can see me do someting very similar with re-running failed tests here.

At the moment those published functions do not take the result object + paths (to keep the api smaller and simpler), instead test discovery is done every time from scratch.

But internally the limitation is not there, if I add functions that clean a dirty discover/run result, it could easily take a cached discovery object and run with different filters. Which is useful not only for this scenario, but for example if you want to observe filesystem, discover tests in a single file, join that discovery result to your cached results and re-run all previously failed tests in all files. Or when you have a huge test suite where discovery takes signifcant amount of time, and multiple configurations to run it with.

[rjmholt]

I'm still very uneasy about the idea of some of the code lenses running all tests and some running only the nearest describe block. I'd much rather see a descriptive error message explaining that the test name should be static.

I 100% agree. I think executing the CodeLens should always do the same thing or fail informatively.

The truth is that you can embed arbitrary expressions in PowerShell strings. But executing a Pester CodeLens, you're going to execute arbitrary code anyway. So maybe we should just get PowerShell to evaluate the string when we think it's not static? Except the big problem there is that executing a single test in a vacuum will quite likely not have the variable populated. Which is a scenario I don't think we should, or can, aim to support -- there's nothing easy about trying to reconstruct arbitrary execution states so that variables exist for interpolation.

So personally I would be happy with any of:

• We reject non-static strings, have a nice informative message saying this, and take the philosophy that parameterised tests don't make sense being plucked out of a file and run individually with the CodeLens (the term "Lens" implies you've lost outside context to me as well).
• We have a heuristic to identify non-static strings and try to evaluate them in our currently PowerShell context when we see them to instantiate the Pester CodeLens name. It might give a different string to running the whole test script and people will expect it to work when it doesn't, but we just would take the line that we meet them halfway and can't take it any further because we can't pick apart the mixing of imperative and declarative paradigms. It will work if the user executes the necessary header lines above it, and I think that's in the spirit of the Pester CodeLens -- pick and choose how you execute your tests.

[SeeminglyScience]

So maybe we should just get PowerShell to evaluate the string when we think it's not static? Except the big problem there is that executing a single test in a vacuum will quite likely not have the variable populated.

Yeah, every example I've seen so far has had the variable defined at the top of the test file. More specifically, it's not even going to be defined at the point we run Invoke-Pester.

For example:

# Invoke-MyCommand.Tests.ps1
$CommandName = $MyInvocation.MyCommand.Name.Replace('Tests.ps1', '')

Describe "$CommandName Integration Tests" {
    # ...
}

If we get PowerShell to evaluate:

Invoke-Pester Invoke-MyCommand.Tests.ps1 -TestName "$CommandName Integration Tests"

Then TestName will evaluate to Integration Tests. The only way I could see that being supportable is if the TestName could take a ScriptBlock as an argument (or if the discovery object scenario @nohwnd described would support that)

[rjmholt]

Playing with this a bit more:

$duck = 'Hello'

Describe "My test $duck" {
    It "works" {
        1 | Should -Be 1
    }
}

There's no way to execute the test with Invoke-Pester ./My.Tests.ps1 -TestName <name> here without knowing what the value of $duck is statically. To make Pester execute it, we need Invoke-Pester ./My.Tests.ps1 -TestName 'My test Hello'. Since the CodeLens execute button is executed outside of the file, we can't just ever make it work in general -- and I think it's worth noting that this is a limitation that exists for Pester without our CodeLens too (not saying it should work, just that the problem is one of expecting runtime information to be known statically).

Circling back to the original problem statement:

I sometimes include test details in the describe block that are sometimes only known at runtime

I think "only known at runtime" defines this problem as out of scope for something based on AST analysis. If it's only known at runtime, then there's no way for us to ask Pester to execute this.

[TylerLeonhardt]

From what I gather here (and @nohwnd please confirm if you haven't already) having parameters inside of the string is not a best practice since test strings should be static.

This helps make tests less cryptic and also helps us out because we have a best practice to depend on. This also means that we (the extension) can help enforce the best practice.

What if we do this:

1. Add a Run all tests CodeLens to the top of the file. This helps @bergmeister with a button that will run all the tests.

2. We don't add CodeLens to Describes with variables in them

3. We add a PSSA rule that warns to "not have variables in test names"

We enforce a best practice, but still give users the ability to click a button to run tests

[SeeminglyScience]

2. We don't add CodeLens to Describes with variables in them

I'd say do add the code lens, but write a descriptive error message. A rule works too I suppose but that's more default stuff folks would need to suppress.

[TylerLeonhardt]

Yeah that sounds good. I like the idea of a PSSA rule in addition to the descriptive error message so users can correct the issue before it happens 👍

[rkeithhill]

Does VSCode support adding an error message in place of a code lens?

[nohwnd]

@TylerLeonhardt That's hard to generalize, but yes I would not recommend it when the data can be put into TestCases. But I know that people who generate their tests inside of foreach loops put variables into their test names a lot, so Pester v5 will have to support this, and solve this problem. 🤷‍♀️ ( I also don't want my test runner to fail running my tests just because I did a bad job naming them. That's like my stove not cooking my food because it's not tasty enough. 😁 )

So what I am thinking is that you pass the line on which the opening brace of the test scriptblock is {. The test itself has that scriptblock and the position info is available on it. So instead of giving test name, you could give filepath:line and we avoid the naming issue.

function _it ($name, $sb) {
    if ($sb.startposition.StartLine -eq 7) { 
        & $sb
    }
} 

_it "i1" {
    Write-Host "i1 runs"
}

_it "i2" {
    Write-Host "i2 runs"
}

# output: 
i1 runs

This would not work in unsaved files, is that a problem for you?

edit: In my Pester v5 I am currently matching on scriptblock if I get same names, so maybe combination of scriptblock + some similarity comparison on the provided names would also work quite well for unsaved files.

edit2: I no longer match on name, I use the postion, and it works beautifully.

[TylerLeonhardt]

@rkeithhill if it doesn't, we can just still show the codelens and have it display an error when clicked if there's no way to, on the fly, change it to an error

[nohwnd]

@bergmeister do you want to try implementing this so it filters on path+line? The implementation in Pester should be very simple (plus you already know the codebase), and you'd get some practice in TS. I'd accept a PR for it.

[bergmeister]

Lots of good discussion here :)
I am first working on issue PowerShell/vscode-powershell#1667 atm in order to be able to run all the Pester tests on one test file by adding a command and context menu on the file tab to run Invoke-Pester on one file since I personally find it annoying having to scroll to a describe block. This should be a good alternative to a Run all tests code lens and I already have a working prototype :)

I am not a fan of having a PSSA rule to recommend not putting variable names into describe/it blocks...
I will have to read through this in a more quiet time. I a currently thinking that it might be best for the moment as a short term solution to change it back to double quotes because it will work for most cases if the initialization code has run once.
@nohwnd Thanks for all the info and effort, yes, the long term solution with a new Pester feature would be great. Do you think it would be possible to not only invoke an it/describe block by line but also make Pester invoke the initialization code BEFORE all the describe/context/it block? I'll see how easy it is to add this, I'll give it a try soon (next week is quite busy) but I might ping you if I need more pointers.

[rkeithhill]

I a currently thinking that it might be best for the moment as a short term solution to change it back to double quotes because it will work for most cases if the initialization code has run once.

We had a specific bug we fixed by changing to single quotes. I'd rather not have a regression in this case. It may be necessary to provide info from PSES back to the extension indicating whether we have string constant expr in which case we use single quotes or an expandable string in which case we could use double quotes. But again, as @SeeminglyScience and I came to the conclusion that in likely many cases the variable required to be interpolated will not be defined in the PS integrated console at the time of the first run and maybe even not for subsequent runs. I think having a feature that partialy works (in certain scenarios) does not make for a good user experience. All in all, I think it is better to stick with single quoting and skip attempting to handle expandable strings - other then to display an error in VSCode when the associated code lens is invoked.

[nohwnd]

@bergmeister Not sure what exactly you mean. Invoke some setup that already is in the file? Or some setup that you externally provide? Before All Describe/Context/It you mean once, or once per file, or on every Describe/Context/It?

[rkeithhill]

So what I am thinking is that you pass the line on which the opening brace of the test scriptblock is {

@nohwnd Does Pester support this yet? I'm not seeing that support. When it does, it would be easy for us to make the change over to invoking tests by line number instead of test name.

[nohwnd]

@rkeithhill it does not, yet. If I have some time I will code it for 4.5.1 over the weekend