Start-Sleep: add 'ms' alias #4039
Conversation
|
@Tadas, It will cover your contributions to all Microsoft-managed open source projects. |
|
@Tadas, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
| $watch = [System.Diagnostics.Stopwatch]::StartNew() | ||
| Start-Sleep -ms 1000 | ||
| $watch.Stop() | ||
| $watch.ElapsedMilliseconds | Should BeGreaterThan 950 |
There was a problem hiding this comment.
Seems like you should also make sure it's less than 1050
There was a problem hiding this comment.
Should we test parameter aliases? It seems it is a general feature and we should test the feature in special tests but not every cmdlet and every alias.
There was a problem hiding this comment.
In general, we should validate the alias is correct, but shouldn't repeat every test for the parameter and its alias. -ms is a pretty simple case, so it's hard to not repeat the test (using -TestCases is probably better here). For parameters with more complex behavior, I think it would be sufficient if some cases used the parameter while others used the alias.
There was a problem hiding this comment.
@iSazonov - As a matter of principle - we avoid testing the parameter binder in cmdlet specific tests.
Personally I don't ask for any tests for an alias - the test isn't covering any new code, but I understand the concerns of those asking for the test, so I don't complain about it either.
There was a problem hiding this comment.
If we're worried about parameter's aliases, we could make a common test for all of them, just like we did for the cmdlets and their aliases as < cmdlet, parameter, alias, platform(?) >.
There was a problem hiding this comment.
It's not a big problem, people mostly never remove code anyway and if they did, it should be caught during the PR, but it's a nice idea.
|
What about the |
|
@Tadas Thanks for your contribution! |
6 participants
Fixes #3991