Codecov Report

Merging #166 into dev will increase coverage by <1%.
The diff coverage is 90%.

@@         Coverage Diff          @@
##            dev   #166    +/-   ##
====================================
+ Coverage    76%    77%   +<1%     
====================================
  Files        11     11            
  Lines      1284   1294    +10     
====================================
+ Hits        988    997     +9     
- Misses      296    297     +1

Hi @dcuomo, thanks for your contribution!

Please fix the failing Pester tests so we can start a review.

Could give a link to the best practice? Atm, I think I would prefer to have it disabled by default (following the New-VMSwitch cmdlet implementation) so this won't be a breaking change (people who need it, would enable explicitly)

Hi @bgelens -

From the output in the failing tests, there's only one that I believe my changes would have affected (Should run without exception while re-creating the VMSwitch). Is there a hint you could give as to why the bandwidth reservation tests are failing?

I'm also having difficulty reviewing why the Mock for VMSwitch is failing A parameter cannot be found that matches parameter name 'EnableIov' at C:\projects\xhyper-v\Tests\Unit\MSFT_xVMSwitch_Id.Tests.ps1: line 265 - I've verified that the resource works on a local system. Is this a test failure that would need to be updated? Appreciate the help...

I'm fine changing the default for Iov to false. It is our best practice albeit not yet documented because it's destructive to fix it later and there's no negative to enabling it. This is a legacy design that has not been updated in the cmdlet.

When I find some time I'll see if I can help you pinpoint the issues. Hopefully somewhere this week

I was able to have all test errors resolved:

I found that MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1 should be updated at line 86. I got the test to pass by having the hashtable look like:

$mockedVMSwitch = @{
    Name = $Name
    SwitchType = 'External'
    AllowManagementOS = $AllowManagementOS
    NetAdapterInterfaceDescription = 'Microsoft Network Adapter Multiplexor Driver'
    IovEnabled = $false
}

MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1 should be updated at line 300 to pass the tests. I got the test to pass by having the hashtable look like:

$global:mockedVMSwitch = @{
    Name = "TestSwitch"
    SwitchType = "External"
    AllowManagementOS = $true
    EmbeddedTeamingEnabled = $true
    Id = [Guid]::NewGuid()
    NetAdapterInterfaceDescriptions = @("Microsoft Network Adapter Multiplexor Driver #1", "Microsoft Network Adapter Multiplexor Driver #2")
    IovEnabled = $false
}

MSFT_xVMSwitch_Id.Tests.ps1 should be updated at line 79 and 147 to pass the tests.

At line 79 a hashtable update should look like this:

$mockedVMSwitch = @{
    Name                            = $Name
    SwitchType                      = 'External'
    AllowManagementOS               = $true
    EmbeddedTeamingEnabled          = $true
    LoadBalancingAlgorithm          = 'HyperVPort'
    BandwidthReservationMode        = 'Default'
    NetAdapterInterfaceDescriptions = @("Microsoft Network Adapter Multiplexor Driver #1", "Microsoft Network Adapter Multiplexor Driver #2")
    IovEnabled                      = $false
}

At line 147, an additional parameter should be included:

Param(
    [Parameter()]
    [String]
    $Name,

    [Parameter()]
    [String[]]
    $NetAdapterName,

    [Parameter()]
    [String]
    $MinimumBandwidthMode,

    [Parameter()]
    [bool]
    $AllowManagementOS,

    [Parameter()]
    [String]
    $SwitchType,

    [Parameter()]
    [bool]
    $EnableEmbeddedTeaming,

    [Parameter()]
    [Guid]
    $Id,

    [Parameter()]
    [Boolean]
    $EnableIov
)

Hope you have enough info to continue working on your PR :) I will expect some additional tests to be included with the PR as well to validate the newly implemented logic (code paths, expected results).

On a side note: I found that at line 289 of MSFT_xVMSwitch.psm1 some style issues are present which should be fixed as well.

Current:

If ($switch.IovEnabled -ne $IovEnabled) {
    Write-Verbose -Message ($LocalizedData.IovEnabledIncorrect -f $Name)
    $removeReaddSwitch = $true
}

Should look like:

if ($switch.IovEnabled -ne $IovEnabled)
{
    Write-Verbose -Message ($LocalizedData.IovEnabledIncorrect -f $Name)
    $removeReaddSwitch = $true
}

Please note that I did not validate your actual contribution to be correct yet. I need a clean test result to start doing that.

If you need more help, please reach out!

I'm not sure, is this ready for review now? If so, could you add some tests for your new functionality and update the changelog please

I implemented your recommendations and it passed all checks - Please review. I'll update the changelog.

Thanks for all the hard work! Please see the comments I made.

Could you also include some tests to verify the logic you introduced is working as expected?
Also, please update the README with the new option for xVMSwitch

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

We have renamed the resource, removing 'x', so please rebase this PR.