All test reported as skipped if not applicable to the platform by iSazonov · Pull Request #2892 · PowerShell/PowerShell

iSazonov · Dec 15, 2016

Fixed files:

powershell\Modules\Microsoft.PowerShell.Management\Clear-EventLog.Tests.ps1:1:if
($IsWindows -and !$IsCoreCLR) {

powershell\Modules\Microsoft.PowerShell.Management\Get-ComputerInfo.Tests.ps1:1325:
return

powershell\Modules\Microsoft.PowerShell.Management\Get-EventLog.Tests.ps1:1:if
($IsWindows -and !$IsCoreCLR) {

powershell\Modules\Microsoft.PowerShell.Management\New-EventLog.Tests.ps1:1:if
($IsWindows -and !$IsCoreCLR) {
powershell\Modules\Microsoft.PowerShell.Management\Registry.Tests.ps1:7:
if ($IsWindows -eq $false) {

powershell\Modules\Microsoft.PowerShell.Management\Registry.Tests.ps1:167:
if ($IsWindows -eq $false) {

powershell\Modules\Microsoft.PowerShell.Management\Remove-EventLog.Tests.ps1:1:if
($IsWindows -and !$IsCoreCLR) {

powershell\Modules\Microsoft.PowerShell.Management\TimeZone.Tests.ps1:17:if
($IsWindows) {

powershell\Modules\Microsoft.PowerShell.Security\FileCatalog.Tests.ps1:6:if
($IsWindows) {

powershell\engine\Help\HelpSystem.Tests.ps1:112: if ($IsWindows)

Fixed files: powershell\Modules\Microsoft.PowerShell.Management\Clear-EventLog.Tests.ps1:1:if ($IsWindows -and !$IsCoreCLR) { powershell\Modules\Microsoft.PowerShell.Management\Get-ComputerInfo.Tests.ps1:1325: return powershell\Modules\Microsoft.PowerShell.Management\Get-EventLog.Tests.ps1:1:if ($IsWindows -and !$IsCoreCLR) { powershell\Modules\Microsoft.PowerShell.Management\New-EventLog.Tests.ps1:1:if ($IsWindows -and !$IsCoreCLR) { powershell\Modules\Microsoft.PowerShell.Management\Registry.Tests.ps1:7: if ($IsWindows -eq $false) { powershell\Modules\Microsoft.PowerShell.Management\Registry.Tests.ps1:167: if ($IsWindows -eq $false) { powershell\Modules\Microsoft.PowerShell.Management\Remove-EventLog.Tests.ps1:1:if ($IsWindows -and !$IsCoreCLR) { powershell\Modules\Microsoft.PowerShell.Management\TimeZone.Tests.ps1:17:if ($IsWindows) { powershell\Modules\Microsoft.PowerShell.Security\FileCatalog.Tests.ps1:6:if ($IsWindows) { powershell\engine\Help\HelpSystem.Tests.ps1:112: if ($IsWindows)

mirichmo

Thanks for the PR. This is a helpful and needed contribution.

mirichmo · Jan 3, 2017

                helpContext = "[@id='FileSystem' or @ID='FileSystem']"
                verb        = 'Add'
                noun        = 'Content'
+                pending     =  if ($IsWindows) { $false } else { $true }


How about
pending = !$IsWindows
instead?

mirichmo · Jan 3, 2017

-    $originalTimeZoneId
    BeforeAll {
-        $originalTimeZoneId = (Get-TimeZone).Id
+        if ($IsWindows) {


The pattern is inconsistent here. Can you please keep consistency and use $IsNotSkipped here as well?

😕 $IsNotSkipped is not used anywhere in the file. I do not see inconsistent here.

The file is internally consistent, so it passes review. I was thinking about more consistent application of skipping patterns across all test files. The reason I favor $IsNotSkipped over $IsWindows is that $IsNotSkipped enables a single definition of the skipping condition, so any future modifications of it happen via a single line change. Using or modifying $IsWindows requires modifying every use of it throughout the file.

Our direction is porting, i.e. skipping tests must generally be disappear in the future and $IsWindows and $IsNotSkipped should stay in the past :-)

mirichmo · Jan 3, 2017

+
+    BeforeAll {
+        $defaultParamValues = $PSdefaultParameterValues.Clone()
+        $IsNotSkipped = ($IsWindows -and !$IsCoreCLR)


Is IsNotSkipped used anywhere else? If not, it should be removed.

mirichmo · Jan 3, 2017

+
+    BeforeAll {
+        $defaultParamValues = $PSdefaultParameterValues.Clone()
+        $IsNotSkipped = ($IsWindows -and !$IsCoreCLR)


mirichmo · Jan 3, 2017

-        New-Item -Path $testKey > $null
-        New-Item -Path $testKey2 > $null
-        New-ItemProperty -Path $testKey -Name $testPropertyName -Value $testPropertyValue > $null
+        if ($IsWindows) {


I have a preference for $IsNotSkipped here since it is easier to read. It will also maintain consistency across this PR

😕 $IsNotSkipped is not used anywhere in the file. There is no need to create a variable to use it once. Yes?
And in general there is no overall style between these files. Each file has its own style.

Looking at this more closely, I'd like to see the try-finally technique used to surround both Describes (like you did in FileCatalog.Tests.ps1). It'll result in the de-duplication of a few lines and more complete grouping of actions in the BeforeAll and AfterAll blocks.

mirichmo · Jan 3, 2017


    AfterAll {
        #restore the previous environment
        Set-Location -Path $restoreLocation


This is one of the spots I had in mind when I mentioned "more complete grouping of actions...". A principle I'd like the tests to follow: if a Describe is being skipped, it should not do anything. In this case, it is saving and restoring the location; an action that can be bypassed with if ($IsNotSkipped). It won't happen in this specific instance, but I'd like to protect against skipped tests accidentally causing side-effects for other tests.

Clear. Thanks!
(My policy was to preserve original test style and add minimal changes.)

Fixed.

iSazonov · Jan 4, 2017

@mirichmo Thanks for the code review and useful comments!

…Shell#2892) * All test reported as skipped if not applicable to the platform Fixed files: powershell\Modules\Microsoft.PowerShell.Management\Clear-EventLog.Tests.ps1:1:if ($IsWindows -and !$IsCoreCLR) { powershell\Modules\Microsoft.PowerShell.Management\Get-ComputerInfo.Tests.ps1:1325: return powershell\Modules\Microsoft.PowerShell.Management\Get-EventLog.Tests.ps1:1:if ($IsWindows -and !$IsCoreCLR) { powershell\Modules\Microsoft.PowerShell.Management\New-EventLog.Tests.ps1:1:if ($IsWindows -and !$IsCoreCLR) { powershell\Modules\Microsoft.PowerShell.Management\Registry.Tests.ps1:7: if ($IsWindows -eq $false) { powershell\Modules\Microsoft.PowerShell.Management\Registry.Tests.ps1:167: if ($IsWindows -eq $false) { powershell\Modules\Microsoft.PowerShell.Management\Remove-EventLog.Tests.ps1:1:if ($IsWindows -and !$IsCoreCLR) { powershell\Modules\Microsoft.PowerShell.Management\TimeZone.Tests.ps1:17:if ($IsWindows) { powershell\Modules\Microsoft.PowerShell.Security\FileCatalog.Tests.ps1:6:if ($IsWindows) { powershell\engine\Help\HelpSystem.Tests.ps1:112: if ($IsWindows) * Fix test after code review * Move skiping to common 'try'

msftclas added the cla-already-signed label Dec 15, 2016

iSazonov changed the title ~~All test reorted as skipped if not applicable to the platform~~ All test reported as skipped if not applicable to the platform Dec 15, 2016

lzybkr requested a review from JamesWTruher December 15, 2016 19:33

iSazonov force-pushed the testsiswindows branch from 3d16d8d to a2a01e9 Compare December 15, 2016 19:46

PowerShellTeam added the Review - Needed The PR is being reviewed label Dec 15, 2016

iSazonov force-pushed the testsiswindows branch 9 times, most recently from 208bfcd to 1cb266e Compare December 16, 2016 18:18

iSazonov force-pushed the testsiswindows branch from 1cb266e to de1cd67 Compare December 16, 2016 18:33

mirichmo suggested changes Jan 3, 2017

View reviewed changes

Fix test after code review

2e8c9fd

mirichmo reviewed Jan 3, 2017

View reviewed changes

mirichmo self-assigned this Jan 3, 2017

Move skiping to common 'try'

e795e7e

mirichmo approved these changes Jan 4, 2017

View reviewed changes

mirichmo merged commit 124979b into PowerShell:master Jan 4, 2017

iSazonov deleted the testsiswindows branch January 5, 2017 17:43

iSazonov removed the Review - Needed The PR is being reviewed label Mar 30, 2017

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

Conversation

iSazonov commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mirichmo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirichmo Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Jan 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iSazonov commented Dec 15, 2016 •

edited

Loading

mirichmo Jan 3, 2017 •

edited

Loading

iSazonov Jan 4, 2017 •

edited

Loading