Refactor pester script detection #638
Conversation
There was a problem hiding this comment.
You might think about adding a test around here - https://github.com/PowerShell/PowerShellEditorServices/blob/master/test/PowerShellEditorServices.Test/Language/LanguageServiceTests.cs#L305
I always like to see the test fail first, then after I fix the issue the test should pass.
|
@rkeithhill Perfect! Didn't realise we had testing for this! I will write a couple of tests and open a PR on that. |
There was a problem hiding this comment.
Just a couple of questions, otherwise, LGTM! Awesome that you fixed this :) Let's give @SeeminglyScience a day to see if he wants to look at this since I know ASTs are his thing.
| // Find plausible Pester commands | ||
| IEnumerable<Ast> commandAsts = scriptFile.ScriptAst.FindAll(IsNamedCommandWithArguments, true); | ||
|
|
||
| return commandAsts.OfType<CommandAst>() |
There was a problem hiding this comment.
can commandAsts ever be null on accident? It looks like there was that commandant != null check previously
There was a problem hiding this comment.
FindAll promises to return a possibly empty collection, but not null
|
|
||
| return | ||
| commandAst != null && | ||
| ast is CommandAst commandAst && |
There was a problem hiding this comment.
Can ast ever been null on accident? This one I'm pretty sure is no but figured I'd ask.
| } | ||
| // Ensure the first word is a Pester keyword | ||
| if (!(commandAst.CommandElements[0] is StringConstantExpressionAst pesterKeywordAst && | ||
| PesterSymbolReference.PesterKeywords.ContainsKey(pesterKeywordAst.Value))) |
There was a problem hiding this comment.
CommandAst.GetCommandName() might be a little cleaner here.
There was a problem hiding this comment.
Yes I just saw that on something else. Will change it over
| if (stringAst == null) | ||
| // Search for a name for the test | ||
| string testName = null; | ||
| for (int i = 1; i < pesterCommandAst.CommandElements.Count; i++) |
There was a problem hiding this comment.
StaticParameterBinder.BindCommand() is excellent for this type of analysis. Might not be available in PowerShell 3 though.
There was a problem hiding this comment.
Is there an easy way to find out? I don't really know where I could get a PowerShell v3 instance to run
There was a problem hiding this comment.
Is there an easy way to find out?
It'll fail to compile when it tries to build using the v3 reference libs. It won't show any syntax markers because of the compiler directives though, and that's a huge pain.
I don't really know where I could get a PowerShell v3 instance to run
I just have a Windows 7 VM set up in my home lab that I manually upgraded from 2 to 3. It's annoying, but works.
There was a problem hiding this comment.
@rjmholt I've got a Windows Server VM for each major version of PowerShell on my Azure subscription. They're all turned off and deallocated (saving that $$$) until needed.
There was a problem hiding this comment.
@tylerl0706 Excellent!
There was a problem hiding this comment.
If you need me to test something, I can 😄 just give me the script to run. Otherwise, tomorrow I'll add you to those VMs so we can just share them.
|
Ok, I'm going to merge this one |
Enable debugging of untitled script files
Fixes vscode-powershell #1223.
Previously, Pester test parsing by PSES has made a couple of assumptions that didn't hold and as a result it wouldn't recognise tests with tags.
Now we do all of that, and are even set up to add something like tag-awareness in future more easily.