Fix the issue Get-Help does not support string pattern under Unix #3852
Conversation
| #endif | ||
| // On Linux, file names are case sensitive, so we need to put both string to lower case. | ||
| // We can not use StringComparison.OrdinalIgnoreCase here as api supports OrdinalIgnoreCase won't support pattern | ||
| return Directory.GetFiles(path.ToLower(), pattern.ToLower()); |
There was a problem hiding this comment.
ToLower() is not required for path, as GetFiles considers it as case insensitive.
| { | ||
| # Create at least one help file matches "about*" pattern | ||
| $null = New-Item -ItemType File -Path $helpFilePath -Value "about_test" -ErrorAction SilentlyContinue | ||
| (Get-ChildItem $PSHOME\$helpFileWithPattern -Recurse).Count | Should Be (Get-Help about*).Count |
There was a problem hiding this comment.
Can you also verify that the about_testCase.help.txt is one of the files that is returned by Get-Help?
There was a problem hiding this comment.
resolved by only get the specified files start with about_testcase*
| { | ||
| # Create at least one help file matches "about*" pattern | ||
| $null = New-Item -ItemType File -Path $helpFilePath -Value "about_test" -ErrorAction SilentlyContinue | ||
| (Get-ChildItem $PSHOME\$helpFileWithPattern -Recurse).Count | Should Be (Get-Help about*).Count |
There was a problem hiding this comment.
Get-Help can get help content from other paths under PSModulePath. So if there are other about help topics not under $PSHome, the counts would not match.
|
@chunqingchen The PR title and body should not be a copy of the issue. Please update the PR title and body based on the contribution.md. General Guidelines
|
9881659 to
92a2b51
Compare
|
@daxian-dbw your comment is resolved |
| private string[] GetFiles(string path, string pattern) | ||
| { | ||
| private string[] GetFiles(string path, string pattern) | ||
| { |
There was a problem hiding this comment.
looks like a formatting mistake here
There was a problem hiding this comment.
The formatting is fixed. It shows normal under IDE, I have to delete and re tab to fix it.
| } | ||
| } | ||
| return (String[])result.ToArray(typeof(string)); | ||
| #else | ||
| return Directory.GetFiles(path, pattern); | ||
| return Directory.GetFiles(path, pattern); |
| string regexPattern = pattern.Replace(".", @"\."); | ||
| regexPattern = regexPattern.Replace("*", ".*"); | ||
| regexPattern = regexPattern.Replace("?", ".?"); | ||
| Regex r = new Regex(regexPattern); |
There was a problem hiding this comment.
Since this matching is infrequent, you should use an interpreted regex instead of a compiled one
https://msdn.microsoft.com/en-us/library/gg578045(v=vs.110).aspx
There was a problem hiding this comment.
You can also use RegEx.IsMatch(filePath, regexPattern). This avoids instantiating the an RegEx object and RegEx engine caches the compiled version for reuse.
| # Create at least one help file matches "about*" pattern | ||
| $null = New-Item -ItemType File -Path $helpFilePath1 -Value "about_test1" -ErrorAction SilentlyContinue | ||
| $null = New-Item -ItemType File -Path $helpFilePath2 -Value "about_test2" -ErrorAction SilentlyContinue | ||
| (Get-Help about_testCase*).Count | Should Be 2 |
There was a problem hiding this comment.
This test case doesn't cover the code change, you explicitly handle *, ?, and .
Seems like you should also have a test case with multiple wildcards
| #if UNIX | ||
| // On Linux, file names are case sensitive, so we need to add | ||
| // extra logic to select the files that match the given pattern. | ||
| ArrayList result = new ArrayList(); | ||
| string[] files = Directory.GetFiles(path); | ||
|
|
||
| string regexPattern = pattern.Replace(".", @"\."); |
There was a problem hiding this comment.
RegEx.Escape(pattern) should be used first, before any replaces.
There was a problem hiding this comment.
This actually should not be used. RegEx.Escape will also escape the '*' and '?' which we want them as normal character to replace later.
| string regexPattern = pattern.Replace(".", @"\."); | ||
| regexPattern = regexPattern.Replace("*", ".*"); | ||
| regexPattern = regexPattern.Replace("?", ".?"); | ||
| Regex r = new Regex(regexPattern); |
There was a problem hiding this comment.
You can also use RegEx.IsMatch(filePath, regexPattern). This avoids instantiating the an RegEx object and RegEx engine caches the compiled version for reuse.
| regexPattern = regexPattern.Replace("*", ".*"); | ||
| regexPattern = regexPattern.Replace("?", ".?"); | ||
| Regex r = new Regex(regexPattern); | ||
|
|
||
| foreach (string filePath in files) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can all this code be replaced by:
return Directory.GetFiles(path).Where(f => Regex.IsMatch(f, regExPattern.ToString())).ToArray();
There was a problem hiding this comment.
return Directory.GetFiles(path).Where(f => Regex.IsMatch(f, regExPattern.ToString())).ToArray(); is not implementing the logic of string which is not using a pattern
950584f to
b82e76b
Compare
|
@adityapatwardhan @SteveL-MSFT Thanks Aditya and Steve. Your comments are resolved. I have spent much time try to figure out why there's time out exception on Unix test runs on my pull request. It ended up not related to my code and also the code and tests get passed on local unix os (both Ubuntu and MacOS). See my comments with the test case. I have to put a skip on the test case if it is not windows os. |
| @@ -114,17 +115,31 @@ private void SearchForFiles() | |||
| } | |||
|
|
|||
| private string[] GetFiles(string path, string pattern) | |||
| { | |||
| { | |||
| @@ -225,4 +225,38 @@ Describe "Get-Help should find help info within help files" -Tags @('CI', 'Requi | |||
| Remove-Item $helpFilePath -Force -ErrorAction SilentlyContinue | |||
| } | |||
| } | |||
|
|
|||
| # There is a bug specified to Tranvis CI that hangs the test if "get-help" is used to search pattern string. This doesn't repro locally. | |||
There was a problem hiding this comment.
Typos:
Tranvis -> Travis
specified -> specific
| $helpFilePath1 = Join-Path $helpFolderPath $helpFile1 | ||
| $helpFilePath2 = Join-Path $helpFolderPath $helpFile2 | ||
|
|
||
| if (!(Test-Path $helpFolderPath)) |
There was a problem hiding this comment.
The if block can be replace by the following. If it already exists, it is not modified.
$null = New-Item -ItemType Directory -Path $helpFolderPath -Force| # Create at least one help file matches "about*" pattern | ||
| $null = New-Item -ItemType File -Path $helpFilePath1 -Value "about_test1" -ErrorAction SilentlyContinue | ||
| $null = New-Item -ItemType File -Path $helpFilePath2 -Value "about_test2" -ErrorAction SilentlyContinue | ||
| Get-Help about_testCas?1 | Should Match "about_test1" |
There was a problem hiding this comment.
For all the Shoulds, use Be or BeExactly. Match uses regular expression comparison. It is not required here.
| Get-Help about_testCas?1 | Should Match "about_test1" | ||
| Get-Help about_testCase.? | Should Match "about_test2" | ||
| (Get-Help about_testCase*).Count | Should Be 2 | ||
| Get-Help about_testCas?.2* | Should Match "about_test2" |
There was a problem hiding this comment.
Ideally, each It should have only one Should. This ensures that if the first should fails, the rest are not ignored. Take a look at: http://www.powershellmagazine.com/2015/06/04/pester-triangulation-and-reusing-test-cases/
You can pass the values for Get-Help as testcases and values for Should as expected values. Using testcases parameter of It also allows you to give better names for each variation.
There was a problem hiding this comment.
Agree with @adityapatwardhan, this is a good example to use the -TestCases parameter of Pester. Look at other test code in this repo as a sample.
There was a problem hiding this comment.
resolved with triangulation format. a good way !
|
@adityapatwardhan @SteveL-MSFT your comments are resolved |
| @{command = "Get-Help about_testCas?.2*"; testname = "test ?, * pattern with dot"; result = "about_test2"} | ||
| ) | ||
|
|
||
| It "Get-Help should find pattern help files - <testname>" -TestCases $testcases -skip: (-not $IsWindows){ |
There was a problem hiding this comment.
unless the testcases are used in other tests, I would prefer to have these inline to the It statement as it makes it easier to associate the two
There was a problem hiding this comment.
A good example of using test cases would be:
| @{command = "Get-Help about_testCas?.2*"; testname = "test ?, * pattern with dot"; result = "about_test2"} | ||
| ) | ||
|
|
||
| It "Get-Help should find pattern help files - <testname>" -TestCases $testcases -skip: (-not $IsWindows){ |
There was a problem hiding this comment.
A good example of using test cases would be:
| try | ||
| { | ||
| # Create at least one help file matches "about*" pattern | ||
| $null = New-Item -ItemType File -Path $helpFilePath1 -Value "about_test1" -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
Can these be done in beforeall and cleanup in afterall?
| $command, | ||
| $result | ||
| ) | ||
| Invoke-Expression $command | Should Be $result |
There was a problem hiding this comment.
Don't use Invoke-Expression instead use splatting instead. Look at: 28ec9a3
There was a problem hiding this comment.
In that case (Get-Help about_testCase*).Count would needs to be explicitly written in the test case one more time. If u think this is better than invoke-expression i will update later.
There was a problem hiding this comment.
After talking to Jim, in general Invoke-Expression is not a best practice. An alternative approach would be to make the command as script block. So the test would look like:
$testcases = @(
@{command = {Get-Help about_testCas?1}; testname = "test ? pattern"; result = "about_test1"}
@{command = {Get-Help about_testCase.?}; testname = "test ? pattern with dot"; result = "about_test2"}
@{command = {(Get-Help about_testCase*).Count}; testname = "test * pattern"; result = "2"}
@{command = {Get-Help about_testCas?.2*}; testname = "test ?, * pattern with dot"; result = "about_test2"}
) And replace Invoke-Expression with
$command.Invoke() | Should be $result| @{command = "Get-Help about_testCas?.2*"; testname = "test ?, * pattern with dot"; result = "about_test2"} | ||
| ) | ||
|
|
||
| It "Get-Help should find pattern help files - <testname>" -TestCases $testcases -skip: (-not $IsWindows){ |
There was a problem hiding this comment.
This should be marked as Pending on Non-Windows and not skipped. So we can revisit later.
|
|
||
| # There is a bug specific to Travis CI that hangs the test if "get-help" is used to search pattern string. This doesn't repro locally. | ||
| # This occurs even if Unix system just returns "Directory.GetFiles(path, pattern);" as the windows' code does. | ||
| # Since there's currently no way to get the vm from Tranvis CI and the test PASSES locally on both Ubuntu and MacOS, excluding pattern test under Unix system. |
There was a problem hiding this comment.
Typo: Tranvis -> Travis.
| # Remove the test files | ||
| AfterAll { | ||
| Remove-Item $helpFilePath1 -Force -ErrorAction SilentlyContinue | ||
| Remove-Item $helpFilePath2 -Force -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
$helpFolderPath does not seem to be cleaned up? Maybe you can just delete $helpFolderPath recursively.
There was a problem hiding this comment.
$helpFolderPath is an existed folder contains other help files.
| $command, | ||
| $result | ||
| ) | ||
| Invoke-Expression $command | Should Be $result |
There was a problem hiding this comment.
After talking to Jim, in general Invoke-Expression is not a best practice. An alternative approach would be to make the command as script block. So the test would look like:
$testcases = @(
@{command = {Get-Help about_testCas?1}; testname = "test ? pattern"; result = "about_test1"}
@{command = {Get-Help about_testCase.?}; testname = "test ? pattern with dot"; result = "about_test2"}
@{command = {(Get-Help about_testCase*).Count}; testname = "test * pattern"; result = "2"}
@{command = {Get-Help about_testCas?.2*}; testname = "test ?, * pattern with dot"; result = "about_test2"}
) And replace Invoke-Expression with
$command.Invoke() | Should be $result|
@adityapatwardhan your comment has been resolved |
| // If the input is pattern instead of string, we need to use Regex expression. | ||
| if (pattern.Contains("*") || pattern.Contains("?")) | ||
| { | ||
| if (Regex.IsMatch(filePath, regexPattern)) |
There was a problem hiding this comment.
Why are we using regular expression matching instead of wildcard matching?
FWIW - wildcard matching already converts the wildcard to a regex - so this code is probably doing something similar but subtly different.
There was a problem hiding this comment.
I agree. We basically need to implementation Directory.GetFiles(path, pattern); in a case-insensitive way, and the code at https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs#L5961 is doing the same thing.
Fix issue #2653
Summary of the issue:
Under Unix system, get-help would not support wildcast pattern such as about*
Root cause of the issue:
Under unix, GetFiles() is using filePath.IndexOf() to search string pattern, this api actually doesn't support pattern.
Fix:
Use regex expression to match *, ? if the search string contains such wildcast. which are the only two cases are supported.