Handle Pester Describe block strings with single quotes inside it #1729
Conversation
|
I wonder if the fix for this should be in PSES where we get the TestName before we send it back to the extension/client. We also have a method there that does the single quote escaping. It is called PowerShellContext.QuoteEscapeString(). Since this method provides the outer single quotes, we'd need to remove those in the extension. I can see pluses/minuses to both approaches but I do like that PSES in this case declares the string is a single quoted string rather than leaving the interpretation up the client. |
|
The advantage of doing things in PSES would be performance to me but for this case I see it as negligible. I found the var testName = pesterSymbol.TestName;
if (testName.Contains("'"))
{
testName = testName.Replace("'", "''");
}If we decide to do it in PSES we'd still need a PR in the extension itself to remove the single quote outside of the test name that is currently hard-coded in it. The main reason why I would not do it in PSES is because the TestName is correct and does not need escaping by itself, it is the application specific usage in the extension that executes a PS command based on this input that requires escaping |
src/features/PesterTests.ts
Outdated
| @@ -72,6 +72,10 @@ export class PesterTestsFeature implements IFeature { | |||
|
|
|||
| if (describeBlockName) { | |||
| launchConfig.args.push("-TestName"); | |||
| // Escape single quotes inside double quotes | |||
| if (describeBlockName.includes("'")) { | |||
| describeBlockName = describeBlockName.replace("'", "''"); | |||
There was a problem hiding this comment.
You're going to hate JS for this one...
https://davidwalsh.name/javascript-replace
You'll need to change the first "'" to a regex
There was a problem hiding this comment.
Ya, the JS replace function bites me from time to time - coming from .NET where it works like it should. :-)
There was a problem hiding this comment.
Ok, thanks, I did not know that. I played around with it a bit in the console and it seems that replace basically breaks when there is more than 1 occurrence to be replaced. Yes, coming from a .Net world as well. Using .replace(/'/g, "''") seems to do the job
|
@bergmeister Can you check on what happens if I have a TestName like this: Will it still get invoked correctly with this change? |
|
Good point @rkeithhill I'm curious about that too |
|
@TylerLeonhardt Yes, because having 2 single quotes within a single quoted string means that the test name itself has got effectively only 1 single quote in it when being evaluated. PSES send us this 1 single quote, which then gets doubled up and since we put the whole test name into single quotes, when it gets evaluated, the 2 single quotes become 1 quote again, so the test name passed to Pester is correct. If we have double quotes in the describe block then the string effectively contains 2 single quotes, which then get doubled up in the extension but then halved again when we put it into the single quoted test name because it just means that we escape it. Long story short: The fact that we put the test name into single quotes and that PSES sends the test name as-is without any quoting business makes this work in both cases very nicely. I think this is actually the best argument for not doing that logic in PSES @rkeithhill . |
|
OK, couldn't remember if we were grabbing the expression's Value or Extent.Text property. Looks good then. BTW did that t-shirt but imagine how much worse it would be if we were using JavaScript directly instead of TypeScript. |
…werShell#1729) * Handle describe block strings with single quotes inside it. * fix replace call to handle expression with more than one single quote as well
) (#1754) * Handle describe block strings with single quotes inside it. * fix replace call to handle expression with more than one single quote as well
PR Summary
Fixes #1725
Although PR #1713 will fix any problems of running Pester tests with special describe block strings when the latest version of Pester (>4.6.0) is used, this PR fixes the behaviour when an older version of Pester is installed to allow running tests with describe blocks that contain a single quote inside them. I also checked that this does not break running describe blocks with single quotes strings or single quotes inside single quotes. I also tested multiple single quotes inside the string.
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets.Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready