Fix get-help online test#3051
Fix get-help online test#3051daxian-dbw merged 7 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom adityapatwardhan:HelpOnlineFixadityapatwardhan/PowerShell:HelpOnlineFixCopy head branch name to clipboard
Conversation
Fix the get-help -online test so that when a default browser is not set, the test does not fail.
| { | ||
| { throw $errorThrown } | Should Not Throw | ||
| } | ||
| } |
There was a problem hiding this comment.
Two thoughts:
- This solution seems overly complex and difficult to parse. Can you simplify it and still retain the same behavior? Maybe use an else within the catch block?
- If you keep it this way then comments are needed to describe what you are doing and why
There was a problem hiding this comment.
Added code to check if default browser is set.
|
|
||
| It "Get-Help get-process -online" -skip:$skipTest { | ||
|
|
||
| { Get-Help get-process -online } | Should Not Throw |
There was a problem hiding this comment.
Would it make more sense to check if the default browser is set? If it is, run the test; otherwise, skip it.
There was a problem hiding this comment.
Good idea. I have added the code similar to the product.
|
|
||
| $skipTest = [System.Management.Automation.Platform]::IsIoT -or | ||
| [System.Management.Automation.Platform]::IsNanoServer | ||
| [System.Management.Automation.Platform]::IsNanoServer |
There was a problem hiding this comment.
Looks like unneeded formating.
|
Are you OK with the changes? |
| if($IsWindows) | ||
| { | ||
| $regKey = "HKEY_CURRENT_USER\Software\Microsoft\Windows\Shell\Associations\UrlAssociations\http\UserChoice" | ||
| $progId = [Microsoft.Win32.Registry]::GetValue($regKey, "ProgId", $null) |
There was a problem hiding this comment.
What about a try-catch here? This can potentially throw
| if($browserKey) | ||
| { | ||
| $browserExe = $browserKey.GetValue($null) | ||
| if($browserExe -notmatch '.exe') |
There was a problem hiding this comment.
Hi @adityapatwardhan. To get the default browser executable path you need to parse the output of $browserKey.GetValue($null). This would look something like this: $browserExe = (($browserKey.GetValue($null) -replace '"' ,"") -split " ")[0].
Also, you could potentially set $skipTest to $false by default which will simplify your logic to something like this:
$skipTest = $true
try
{
$regKey = "HKEY_CURRENT_USER\Software\Microsoft\Windows\Shell\Associations\UrlAssociations\http\UserChoice"
$progId = [Microsoft.Win32.Registry]::GetValue($regKey, "ProgId", $null)
if($progId)
{
$browserKey = [Microsoft.Win32.Registry]::ClassesRoot.OpenSubKey("$progId\shell\open\command", $false)
if($browserKey)
{
$browserExe = (($browserKey.GetValue($null) -replace '"' ,"") -split " ")[0]
if($browserExe.EndsWith('.exe'))
{
$skipTest = $false
}
}
}
}
catch
{
# ignore the failure
}
|
@Francisco-Gamino Do you have any additional comments? |
|
@adityapatwardhan : You should probably squash your commits into one. Other than that, LGTM. |
|
@PowerShell/powershell-maintainers This is ready for merge. |
|
Chatted with @adityapatwardhan offline, and it looks like a bug in the default browser detection code because:
We need to fix this in product code. @Francisco-Gamino: thought? |
|
Perhaps you should put a comment in the code to the effect that this is a workaround for #3079 . This would help if someone is trying to understand the code. |
|
@TravisEz13 Checks have passed. Ready for merge. |
Fix the get-help -online test so that when a default browser is not set,
the test does not fail.