Please use ShouldBeErrorId
Sample https://github.com/PowerShell/PowerShell/blob/master/test/powershell/Modules/Microsoft.PowerShell.Core/Job.Tests.ps1#L28

Please use ShouldBeErrorId

Your code may raise null reference exception.
Why do you not use BeOfType?

The same about BeOfType.

The same about throw "No Exception!"

Please use throw "No Exception!" - it is our documented template

Merge fail on my behalf! Will fix.

We shouldn't change a public API.
Preferred way is to add new constructor.

I believe that there is no need to examine the properties, we can get an undesirable exception only when a method is called (ex., $prop.GetType()).
Here we can make the test more graceful and use ShouldBeErrorId too:

$exc = {
                $sshConnectionInfo = [System.Management.Automation.Runspaces.SSHConnectionInfo]::new(
                    "UserName",
                    "localhost",
                    "NoValidKeyFilePath",
                    22)

                $rs = [runspacefactory]::CreateRunspace($sshConnectionInfo)
                $rs.Open()
} | ShouldBeErrorId "PSRemotingDataStructureException"

$exc.Exception.InnerException.InnerException | Should BeOfType System.IO.FileNotFoundException

Also the tests skip a cleanup code. Please add something like:

        AfterEach {
            if ($rs -ne $null) { $rs.Dispose() }
        }

It would appear that ShouldBeErrorId doesn't return the thrown exception, so we would be unable to process $exc further here (ShouldBeErrorId will return result of Should Be - bool).

Based on this, would you prefer for ShouldBeErrorId to be implemented here to check for PSRemotingDataStructureException, or continue to inspect the nested inner exception?

Cleanup code added

I am a little worried about sharing parameters between WinRM and SSH parameter sets. My worry is that PowerShell parameter resolution might resolve to the wrong parameter set for positional parameters (e.g., New-PSSession localhost -Port 8080). Please verify that parameter set resolution does not change (i.e. causes a breaking change to existing behavior).

If this turns out to be a problem we can think about adding a new SSH specific parameter set port parameter (SSHPort), but hopefully we won't have to.

In reply to: 111206290 [](ancestors = 111206290)

Just tested this, confirmed to default to ComputerNameParameterSet:

PS C:\Users\Lee\Documents\PowerShell> New-PSSession localhost -Port 5985

 Id Name            ComputerName    ComputerType    State         ConfigurationName     Availability
 -- ----            ------------    ------------    -----         -----------------     ------------
 11 WinRM11         localhost       RemoteMachine   Opened        Microsoft.PowerShell     Available

Not sure whether DefaultParameterSetName would help here to remove any uncertainty?

It looks like we are Ok. I assume that this works and doesn't result in a "cannot resolve parameter set" error?

New-PSSession -HostName localhost -Port 22

Also please check this again when you implement Invoke-Command SSH parameter sets

Invoke-Command -HostName localhost -Port 22 -ScriptBlock { "Hello" }
Invoke-Command localhost { "Hello" } -Port 5895

I think this will also be Ok but it does have more complex parameter sets.

Parameter set looks to be resolving correctly:

PS C:\Users\Lee\Projects\PowerShell> New-PSSession -HostName localhost -Port 22
Lee@DESKTOP-0I6KRV7@localhost's password:

I've confirmed this to be resolving correctly following implementation for Invoke-Command:

PS C:\Users\Lee\Projects\PowerShell> Invoke-Command -HostName localhost -Port 22 -ScriptBlock { "Hello" }
Lee@DESKTOP-0I6KRV7@localhost's password:

PS C:\Users\Lee\Projects\PowerShell> Invoke-Command localhost { "Hello" } -Port 5985
Hello

Great news. I am looking forward to the next commit.

I think the Port parameter in the hash table should be an integer and not a string. It would be more natural than enclosing the port value in quotes: @{ HostName = 'targetMachine'; UserName = 'UserA'; Port = 8080 }. This would mean updating this parse method to special case the Port parameter.

These creation helper functions are used by Invoke-Command, and this cmdlet will also have to be updated to support the new "Port" parameter. All cmdlets that currently support SSH parameter sets are New-PSSession, Enter-PSSession, Invoke-Command. You covered the first two but missed the last one.

Since this is duplicate code from WSManConnectionInfo, can we put this in a helper method in the base RunspaceConnectionInfo class?

Please use the original constructor so as not to duplicate code:
public SSHConnectionInfo(string userName, string computerName, string keyFilePath, int port) : this(userName, computerName, keyFilePath)
{
if ((port < MinPort) || (port > MaxPort))
{
...
}
this.Port = (port != 0) port : DefaultPort;
}

I didn't like how it was done in code below. We should use the previous style of if-else if.

We could even use C# 7.0 patterns.
/cc @daxian-dbw Are we ready to use the patterns?

Are you referring to the ParseSSHConnectionHashTable() @iSazonov?

I means that you add extra if-else level with the port parameter.

Ahh, I did this as we now need to handle InvalidSSHConnectionParameter for value types (integer), wereas previously we would check whether paramValue (as string) was null.

The only reason I added the extra level of if-else was to handle the integer/string values differently, although I do agree that it isn't ideal :(

I'll take another look at this later.

I agree with @iSazonov, it would be nice to keep the if-else-if blocks. You can create a helper method to get a string or int parameter and throw exception a needed.

private static string GetStringParameter(object param)
{
    var paramValue = param as string;
    if (string.IsNullOrEmpty(paramValue)) { throw new PSArgumentException("..."); }
    return paramValue;
}

private static int GetIntParameter(object param)
{
    int? paramValue = param as int?;
    if (paramValue == null) { throw new PSArgumentException("..."); }
    return paramValue.Value;
}

if (paramName.Equals(PortParameter, StringComparison.OridinalIgnoreCase))
{
    connectionInfo.Port = GetIntParameter(item[paramName]);
}
else if ...

We don't need the Convert.ToInt32() and I believe the integer is only un-boxed once.

I've just added a commit to address this - much cleaner :)

What about C# 7.0 patterns:

       private static string GetSSHConnectionStringParameter(object param)
        {
            if (param is string paramValue && string.IsNullOrEmpty(paramValue))
            {
                return paramValue;
            }

            throw new PSArgumentException(RemotingErrorIdStrings.InvalidSSHConnectionParameter);
        }

       private static int GetSSHConnectionIntParameter(object param)
        {
            if (param is int paramValue)
            {
                return paramValue;
            }

            throw new PSArgumentException(RemotingErrorIdStrings.InvalidSSHConnectionParameter);
        }

I like it, but is this supported in are targeted runtimes?

I can not still build locally after move to Core2.0. Maybe @daxian-dbw confirm.

I am able to build with Core 2.0 and this pattern builds successfully. So I think we are good to use it.

C#7 is supported by the latest dotnet-cli and VS2017, so we are good here.

I've added another commit to add C# 7 pattern goodness, sorry for the delay!