GeezerVison is showing github.com — Start-Sleep: add 'ms' alias by Tadas · Pull Request #4039 · PowerShell/PowerShell
Conversation
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
mklement0 reacted with thumbs up emoji
|
@Tadas,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.
It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Tadas Medišauskas (@Tadas), thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.
Thanks, Microsoft Pull Request Bot
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
$watch = [System.Diagnostics.Stopwatch]::StartNew()
|
|
|
Start-Sleep -ms 1000
|
|
|
$watch.Stop()
|
|
|
$watch.ElapsedMilliseconds | Should BeGreaterThan 950
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you should also make sure it's less than 1050
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
iSazonov reacted with thumbs up emoji
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ilya (@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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(?) >.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
iSazonov reacted with thumbs up emoji
|
What about the Describe "Start-Sleep" -Tags "CI" { test, isn't it redundant?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
SteveL-MSFT reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|