Fix issues with running RunKeyVaultTests.ps1. #2509
Conversation
|
Hi @adarce, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
|
|
||
| Start-Sleep -Seconds $timeout |
There was a problem hiding this comment.
revert this back to [Microsoft.WindowsAzure.Testing.TestUtilities]::Wait. We do not want to do sleep waits in playback mode.
There was a problem hiding this comment.
@hovsepm It seems that this issue has come up before (see: #1846 (comment)). I have updated my pull request to use the workaround recommended by @markcowl here: (#1846 (comment)).
|
@adarce please squash your commits. |
|
@pomortaz do you need to sign-off? :) |
|
@hovsepm thanks for the review. I've already signed off. :) |
adarce
force-pushed
the
feature/fix-RunKeyVaultTests
branch
from
5134036 to
1174593
Compare
## Background When attempting to run the Azure Key Vault PowerShell tests via the `RunKeyVaultTests.ps1` script, the following 5 tests would fail: ``` Test_ImportByokWithDefaultAttributes Test_ImportByokWith1024BitKey Test_ImportByokWithCustomAttributes Test_GetAllKeys Test_GetKeyVersions ``` The first 3 BYOK tests failed because they test against dummy `.byok` files that were generated with a specific subscription ID that may not match the subscription ID of whomever is running the tests. The last 2 tests failed because they attempt to reference `[Microsoft.WindowsAzure.Testing.TestUtilities]`, which cannot be found. Moreover, this PowerShell script was difficult to use if we didn't want to provide our own vault to test against for both the data plane and the control plane, but we instead wanted the script to generate its own temporary vault. Also, we sometimes would like to programmatically skip Active Directory related tests for certain environments (instead of having it hardcoded to always skip for Fairfax environments). Finally, the PowerShell script could use some cleanup with respect to formatting whitespace, fleshing out docstrings, and general organization. ## The Change This change addresses the above issues by: - Guarding the 3 BYOK tests with an if condition that tests the subscription ID. - Replacing `Wait($timeout * 1000)` with `Start-Sleep -Seconds $timeout`. - Saving/restoring temporary vault state for both the control plane and data plane tests. - Exposing the `$NoADCmdLetMode` parameter. - Generally improving the coding style.
adarce
force-pushed
the
feature/fix-RunKeyVaultTests
branch
from
1174593 to
c752abc
Compare
|
@hovsepm I have squashed my commits, as requested. Please let me know if there's anything else I can improve! |
| @@ -37,6 +37,8 @@ Param($rgName, $location, $tagName, $tagValue) | |||
| Assert-AreEqual "Standard" $actual.Sku | |||
| Assert-AreEqual $false $actual.EnabledForDeployment | |||
|
|
|||
| if ($global:noADCmdLetMode) {return;} | |||
There was a problem hiding this comment.
I really don't like this. This just skips the test without letting us know that's what happened. A better solution would be to store the needed subscriptionId in the mocks, which is how most tests solve similar issues.
|
@adarce a few comments |
- Stopped skipping Test-CreateNewVault if noADCmdLetMode. - Removed PSHCommon directory and instead updated Common.ps1 from ScenarioTests.Common. - Used TestMockSupport.RunningMocked for Wait-Seconds instead.
adarce
force-pushed
the
feature/fix-RunKeyVaultTests
branch
from
bf65a77 to
367159e
Compare
|
@markcowl I've pushed a commit that hopefully addresses each of your concerns. Let me know if there's anything else I can improve! |
|
on demand run here: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/933/ |
|
LGTM once the on-demand run passes |
Background
When attempting to run the Azure Key Vault PowerShell tests via the
RunKeyVaultTests.ps1script, the following 5 tests would fail:The first 3 BYOK tests failed because they test against dummy
.byokfiles that were generated with a specific subscription ID that may not match the subscription ID of whomever is running the tests. The last 2 tests failed because they attempt to reference[Microsoft.WindowsAzure.Testing.TestUtilities], which cannot be found.Moreover, this PowerShell script was difficult to use if we didn't want to provide our own vault to test against for both the data plane and the control plane, but we instead wanted the script to generate its own temporary vault. Also, we sometimes would like to programmatically skip Active Directory related tests for certain environments (instead of having it hardcoded to always skip for Fairfax environments). Finally, the PowerShell script could use some cleanup with respect to formatting whitespace, fleshing out docstrings, and general organization.
The Change
This change addresses the above issues by:
Wait($timeout * 1000)withStart-Sleep -Seconds $timeout.$NoADCmdLetModeparameter.