Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Remove errors in counter tests on non-Windows#2914

Closed
iSazonov wants to merge 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:countertestsiSazonov/PowerShell:countertestsCopy head branch name to clipboard
Closed

Remove errors in counter tests on non-Windows#2914
iSazonov wants to merge 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:countertestsiSazonov/PowerShell:countertestsCopy head branch name to clipboard

Conversation

@iSazonov

@iSazonov iSazonov commented Dec 21, 2016 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Errors in Travis CI log is Get-ItemProperty : Cannot find a provider with the name 'Registry'.
(Get-Counter, Import-Counter and Export-Counter is only Windows cmdlets.)

@iSazonov iSazonov force-pushed the countertests branch 5 times, most recently from 984c536 to 30df1a8 Compare December 21, 2016 11:41
@iSazonov
Remove errors in counter tests
b1a425b 
Errors is Get-ItemProperty : Cannot find a provider with the name
'Registry'.
@iSazonov iSazonov changed the title Skip counter tests on non-Windows Remove errors in counter tests on non-Windows Dec 21, 2016
@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Dec 21, 2016
@lzybkr

lzybkr commented Dec 21, 2016

Copy link
Copy Markdown
Contributor

The changes look fine.

I thought I would make more general comments on these tests (I think @Francisco-Gamino is the original author).

I think the tests would be a bit cleaner if the test used BeforeAll and set the default parameter for It -Skip in BeforeAll. The test for skipping can be simpler as well, in most cases, it's just !$IsWindows which is much faster than calling a function.

The last thing I noticed - the ValidateParameters function shouldn't exist, the -TestsCases parameter to It should be used instead. This will also be faster to skip on non-Windows platforms as It will be called once instead of once per testcase.

@iSazonov

iSazonov commented Dec 22, 2016

Copy link
Copy Markdown
Collaborator Author

@lzybkr Thanks for comments.
I made some performance optimizations (using $isWindows and set -MaxSamples 2). The savings amounted to about 4 seconds.
With regard to the "ValidateParameters function shouldn't exist" I believe that this is not acceptable because there is a request from @JamesWTruher #2887 - we must explicitly skip every test to retain the correct stats.

@iSazonov

iSazonov commented Jan 6, 2017

Copy link
Copy Markdown
Collaborator Author

I close the PR since others (#2958 and #2952) have already addressed these changes.

@iSazonov iSazonov closed this Jan 6, 2017
@iSazonov iSazonov deleted the countertests branch January 17, 2017 11:09
@iSazonov iSazonov removed the Review - Needed The PR is being reviewed label Mar 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Francisco-Gamino Francisco-Gamino Awaiting requested review from Francisco-Gamino

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@iSazonov @lzybkr @PowerShellTeam @msftclas
Morty Proxy This is a proxified and sanitized view of the page, visit original site.