Skip to content

Refactor pester script detection #638

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.

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

Merged
merged 4 commits into from
Mar 26, 2018
Merged
Changes from 2 commits
Commits
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
153 changes: 96 additions & 57 deletions src/PowerShellEditorServices/Symbols/PesterDocumentSymbolProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,63 +27,104 @@ IEnumerable<SymbolReference> IDocumentSymbolProvider.ProvideDocumentSymbols(
return Enumerable.Empty<SymbolReference>();
}

var commandAsts = scriptFile.ScriptAst.FindAll(ast =>
{
// Find plausible Pester commands
IEnumerable<Ast> commandAsts = scriptFile.ScriptAst.FindAll(IsNamedCommandWithArguments, true);

return commandAsts.OfType<CommandAst>()
Copy link
Member

Choose a reason for hiding this comment

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

can commandAsts ever be null on accident? It looks like there was that commandant != null check previously

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FindAll promises to return a possibly empty collection, but not null

.Where(IsPesterCommand)
.Select(ast => ConvertPesterAstToSymbolReference(scriptFile, ast))
.Where(pesterSymbol => pesterSymbol?.TestName != null);
}

/// <summary>
/// Test if the given Ast is a regular CommandAst with arguments
/// </summary>
/// <param name="ast">the PowerShell Ast to test</param>
/// <returns>true if the Ast represents a PowerShell command with arguments, false otherwise</returns>
private static bool IsNamedCommandWithArguments(Ast ast)
{
CommandAst commandAst = ast as CommandAst;

return
commandAst != null &&
commandAst.InvocationOperator != TokenKind.Dot &&
PesterSymbolReference.GetCommandType(commandAst.GetCommandName()).HasValue &&
commandAst.CommandElements.Count >= 2;
},
true);
}

return commandAsts.Select(
ast =>
{
// By this point we know the Ast is a CommandAst with 2 or more CommandElements
int testNameParamIndex = 1;
CommandAst testAst = (CommandAst)ast;
/// <summary>
/// Test whether the given CommandAst represents a Pester command
/// </summary>
/// <param name="commandAst">the CommandAst to test</param>
/// <returns>true if the CommandAst represents a Pester command, false otherwise</returns>
private static bool IsPesterCommand(CommandAst commandAst)
{
if (commandAst == null)
{
return false;
}

// The -Name parameter
for (int i = 1; i < testAst.CommandElements.Count; i++)
{
CommandParameterAst paramAst = testAst.CommandElements[i] as CommandParameterAst;
if (paramAst != null &&
paramAst.ParameterName.Equals("Name", StringComparison.OrdinalIgnoreCase))
{
testNameParamIndex = i + 1;
break;
}
}
// Ensure the first word is a Pester keyword
if (!(commandAst.CommandElements[0] is StringConstantExpressionAst pesterKeywordAst &&
PesterSymbolReference.PesterKeywords.ContainsKey(pesterKeywordAst.Value)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

CommandAst.GetCommandName() might be a little cleaner here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I just saw that on something else. Will change it over

{
return false;
}

if (testNameParamIndex > testAst.CommandElements.Count - 1)
{
return null;
}
// Ensure that the last argument of the command is a scriptblock
if (!(commandAst.CommandElements[commandAst.CommandElements.Count-1] is ScriptBlockExpressionAst))
{
return false;
}

StringConstantExpressionAst stringAst =
testAst.CommandElements[testNameParamIndex] as StringConstantExpressionAst;
return true;
}

/// <summary>
/// Convert a CommandAst known to represent a Pester command and a reference to the scriptfile
/// it is in into symbol representing a Pester call for code lens
/// </summary>
/// <param name="scriptFile">the scriptfile the Pester call occurs in</param>
/// <param name="pesterCommandAst">the CommandAst representing the Pester call</param>
/// <returns>a symbol representing the Pester call containing metadata for CodeLens to use</returns>
private static PesterSymbolReference ConvertPesterAstToSymbolReference(ScriptFile scriptFile, CommandAst pesterCommandAst)
{
string testLine = scriptFile.GetLine(pesterCommandAst.Extent.StartLineNumber);
string commandName = (pesterCommandAst.CommandElements[0] as StringConstantExpressionAst)?.Value;

if (stringAst == null)
// Search for a name for the test
string testName = null;
for (int i = 1; i < pesterCommandAst.CommandElements.Count; i++)
Copy link
Collaborator

Choose a reason for hiding this comment

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

StaticParameterBinder.BindCommand() is excellent for this type of analysis. Might not be available in PowerShell 3 though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there an easy way to find out? I don't really know where I could get a PowerShell v3 instance to run

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tylerl0706 Excellent!

Copy link
Member

Choose a reason for hiding this comment

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

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.

{
CommandElementAst currentCommandElement = pesterCommandAst.CommandElements[i];

// Check for an explicit "-Name" parameter
if (currentCommandElement is CommandParameterAst parameterAst)
{
i++;
if (parameterAst.ParameterName == "Name" && i < pesterCommandAst.CommandElements.Count)
{
return null;
testName = (pesterCommandAst.CommandElements[i] as StringConstantExpressionAst)?.Value;
break;
}
continue;
}

string testDefinitionLine =
scriptFile.GetLine(
ast.Extent.StartLineNumber);

return
new PesterSymbolReference(
scriptFile,
testAst.GetCommandName(),
testDefinitionLine,
stringAst.Value,
ast.Extent);
// Otherwise, if an argument is given with no parameter, we assume it's the name
if (pesterCommandAst.CommandElements[i] is StringConstantExpressionAst testNameStrAst)
{
testName = testNameStrAst.Value;
break;
}
}

}).Where(s => s != null);
return new PesterSymbolReference(
scriptFile,
commandName,
testLine,
testName,
pesterCommandAst.Extent
);
}
}

Expand Down Expand Up @@ -114,6 +155,17 @@ public enum PesterCommandType
/// </summary>
public class PesterSymbolReference : SymbolReference
{
/// <summary>
/// Lookup for Pester keywords we support. Ideally we could extract these from Pester itself
/// </summary>
internal static readonly IReadOnlyDictionary<string, PesterCommandType> PesterKeywords =
new Dictionary<string, PesterCommandType>(StringComparer.OrdinalIgnoreCase)
{
{ "Describe", PesterCommandType.Describe },
{ "Context", PesterCommandType.Context },
{ "It", PesterCommandType.It }
};

private static char[] DefinitionTrimChars = new char[] { ' ', '{' };

/// <summary>
Expand Down Expand Up @@ -145,25 +197,12 @@ internal PesterSymbolReference(

internal static PesterCommandType? GetCommandType(string commandName)
{
if (commandName == null)
PesterCommandType pesterCommandType;
if (!PesterKeywords.TryGetValue(commandName, out pesterCommandType))
{
return null;
}

switch (commandName.ToLower())
{
case "describe":
return PesterCommandType.Describe;

case "context":
return PesterCommandType.Context;

case "it":
return PesterCommandType.It;

default:
return null;
}
return pesterCommandType;
}
}
}