FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Start-Sleep: add 'ms' alias by Tadas · Pull Request #4039 · PowerShell/PowerShell · GitHub

Start-Sleep: add 'ms' alias - #4039

Merged
Dongbo Wang (daxian-dbw) merged 2 commits into
PowerShell:masterfrom
Tadas:start-sleep-ms-alias
Jun 19, 2017
Merged

Start-Sleep: add 'ms' alias#4039
Dongbo Wang (daxian-dbw) merged 2 commits into
PowerShell:masterfrom
Tadas:start-sleep-ms-alias

Conversation

Copy link
Copy Markdown
Contributor

Fixes #3991

@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

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

$watch = [System.Diagnostics.Stopwatch]::StartNew()
Start-Sleep -ms 1000
$watch.Stop()
$watch.ElapsedMilliseconds | Should BeGreaterThan 950

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Seems like you should also make sure it's less than 1050

Ilya (iSazonov) Jun 17, 2017
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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(?) >.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Contributor Author

What about the Describe "Start-Sleep" -Tags "CI" { test, isn't it redundant?

Dongbo Wang (daxian-dbw) merged commit 437d773 into PowerShell:master Jun 19, 2017

Copy link
Copy Markdown
Collaborator

Tadas Medišauskas (@Tadas) Thanks for your contribution!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL