Changes to allow tests to be run outside of CI #882

JamesWTruher · 2018-02-10T00:58:53Z

PR Summary

Test and build changes to enable test execution outside of CI environment
Reduce verbosity of buildCore script
Simplify test execution to a single command with a single result file

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
User facing documentation needed
Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
This PR is ready to merge and is not work in progress
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

bergmeister

Great work but maybe the upgrade to Pester v4 should be a separate PR?

bergmeister · 2018-02-10T10:23:05Z

Tests/Engine/CustomizedRule.tests.ps1

@@ -97,19 +97,16 @@ Describe "Test importing correct customized rules" {
 		    $customizedRulePath.Count | Should Be 1
 		}

-        It "will show the custom rule when given a rule folder path with trailing backslash" {
+        It "will show the custom rule when given a rule folder path with trailing backslash" -skip:(!$IsWindows){


I guess you are planning to introduce a Travis build for Linux/Mac as well? I think it would be good if you open an issue with a brief description of the high level plan of PR's to come so that we can see the big picture.

yup, that's the plan, I'll work on that issue
the start is here: #886

Thanks. Looks good to me.

bergmeister · 2018-02-10T10:25:01Z

Tests/Engine/CustomizedRule.tests.ps1

 		}

 		It "will show the custom rules when given a glob" {
 			# needs fixing for Linux
 			$expectedNumRules = 4
-			if (Test-PSEditionCoreCLRLinux)
+			if ( ! $IsWindows )


Very minor but here and in other changes the spacing inside the parenthesis is inconsistent.

sure - will fix

bergmeister · 2018-02-10T10:26:48Z

Tests/Engine/InvokeScriptAnalyzer.tests.ps1

-        $initialTestScript = Get-Content $directory\TestScriptWithFixableWarnings.ps1 -Raw
+    BeforeAll {
+        $scriptName = "TestScriptWithFixableWarnings.ps1"
+        $testSource = join-path $PSScriptRoot ${scriptName}


I think ${scriptName} can be simplified to $scriptName

the "${< name >}" is the formal identifier of the variable. I should have done it to the first variable too. You'll start to see more of these in my PRs. I should note that this is much different than "$($variable)" which is definitely unneeded

I totally agree that braces make sense inside a string for string interpolation and is better than "$($variable)" but can you briefly explain please why one would use curly braces for variables outside of strings (the only use case that I found here is when the variable name itself contains special characters) or is this just a question of style?

in the case above it's inconsistent (since it's not used with $PSScriptRoot so it's a bit confusing. My preference is to use braces all the time (sort of like full commands vs aliases), and it's mostly habit. When I'm in a hurry, I sometimes omit them.
Since most don't have them, I removed them here to avoid the confusion

bergmeister · 2018-02-10T10:30:39Z

Tests/Engine/ModuleHelp.Tests.ps1

@@ -63,7 +63,7 @@ Param
 	$RequiredVersion
 )

-# #Requires -Module @{ModuleName = 'Pester'; ModuleVersion = '3.4.0'}
+# #Requires -Module @{ModuleName = 'Pester'; ModuleVersion = '4.1.1'}


Is there a technical reason to upgrade to v4 as part of this PR? I think it might be better to do this as a separate PR?

the test fixes are sort of entwined, by doing them all at once I won't need to change the tests again.

Ok, I see. Then we also need to update the documented version number of Pester in the test section here of the developer guide.

quite right - fixed

bergmeister · 2018-02-10T10:33:31Z

Tests/Engine/RuleSuppression.tests.ps1

@@ -106,7 +106,7 @@ function SuppressPwdParam()
    }

    Context "Rule suppression within DSC Configuration definition" {
-        It "Suppresses rule" -skip:((Test-PSEditionCoreCLRLinux) -or ($PSVersionTable.PSVersion -lt [Version]'5.0.0')) {
+        It "Suppresses rule" -skip:((!$IsWindows) -or ($PSVersionTable.PSVersion -lt [Version]'5.0.0')) {


$PSVersionTable.PSVersion.Major -lt 5 looks more elegant to me but this might be just a question of style and therefore opinionated.

that's fine - I have no preference

bergmeister · 2018-02-10T10:34:15Z

Tests/Engine/RuleSuppression.tests.ps1

Is this a change of the file's encoding?

it doesn't look like it. it looks like there was previously no newline at the end of the file.

bergmeister · 2018-02-10T10:35:56Z

Tests/Rules/AlignAssignmentStatement.tests.ps1

@@ -66,7 +66,7 @@ $x = @{ }
    }

    Context "When assignment statements are in DSC Configuration" {
-        It "Should find violations when assignment statements are not aligned" {
+        It "Should find violations when assignment statements are not aligned" -skip:($IsLinux -or $IsMacOS) {


why not simply !$isWindows?

bergmeister · 2018-02-10T10:36:48Z

Tests/Rules/UseIdenticalMandatoryParametersForDSC.tests.ps1

@@ -29,7 +29,7 @@ Describe "UseIdenticalMandatoryParametersForDSC" {
        }

        # todo add a test to check one violation per function
-        It "Should find a violations" {
+        It "Should find a violations" -pending {


Why pending?

the test is marked as TODO, and the comment seems like it is a test in potentio rather than something useful since it would only be checking a file that is known to be good.

Mark some tests as pending that weren't actually doing anything Skip DSC tests on Mac/Linux

Update to use Pester version 4.1.1 Change logic in appveyor to invoke pester only once

JamesWTruher · 2018-02-15T18:36:44Z

@bergmeister I think this is ready now - what do you think?

bergmeister · 2018-02-15T23:01:00Z

@JamesWTruher Overall most changes look OK (although I cannot verify some technical details why some stuff does not work on Windows but I think it is OK to trust to you on that).
My 2 thoughts are:

Should we adapt all assertion operator to the new pester v4 syntax with dashes? Although the old syntax still works, mixing old and new style is discouraged and will bite us at some point if we don't change it now. I don't mind if you do it later in another PR but we should be aware of this problem.
Should we add more details to the rule documentation due to the different platform specific behaviour that the tests show?

JamesWTruher · 2018-02-16T17:55:44Z

I'm actually not sure why there are differences between the platforms either, my changes were more mechanical (removing the broken check for linux, which didn't include mac, etc). I will continue to investigate that. As for the update to Pester 4 syntax, that's tracked in #890.
As for documentation, any rule which works differently between platforms should be understood and documented.

JamesWTruher self-assigned this Feb 10, 2018

raghushantha added the in progress label Feb 10, 2018

bergmeister reviewed Feb 10, 2018

View reviewed changes

JamesWTruher force-pushed the jameswtruher/build001 branch from 416c805 to 1dc2cf1 Compare February 12, 2018 18:47

JamesWTruher mentioned this pull request Feb 12, 2018

WIP: Roadmap for script analyzer #886

Open

6 tasks

Change logic for test execution to handle running on Mac

88d19fc

Mark some tests as pending that weren't actually doing anything Skip DSC tests on Mac/Linux

JamesWTruher force-pushed the jameswtruher/build001 branch from 1dc2cf1 to 07075e5 Compare February 12, 2018 19:30

Reduce verbosity of build script

ad17fe8

Update to use Pester version 4.1.1 Change logic in appveyor to invoke pester only once

JamesWTruher force-pushed the jameswtruher/build001 branch from 07075e5 to ad17fe8 Compare February 13, 2018 20:24

bergmeister added the Area - Build label Feb 14, 2018

JamesWTruher changed the title ~~WIP: Changes to allow tests to be run outside of CI~~ Changes to allow tests to be run outside of CI Feb 15, 2018

JamesWTruher removed the in progress label Feb 15, 2018

JamesWTruher mentioned this pull request Feb 16, 2018

update tests to use new Pester 4 syntax #890

Closed

JamesWTruher merged commit de28e4f into PowerShell:development Feb 16, 2018

JamesWTruher mentioned this pull request Feb 16, 2018

Added -ReportSummary switch #868

Closed

bergmeister mentioned this pull request Feb 16, 2018

Fix regressions introduced by PR 882 #891

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to allow tests to be run outside of CI #882

Changes to allow tests to be run outside of CI #882

JamesWTruher commented Feb 10, 2018 •

edited

Loading

bergmeister left a comment

bergmeister Feb 10, 2018 •

edited

Loading

JamesWTruher Feb 12, 2018 •

edited

Loading

bergmeister Feb 12, 2018

bergmeister Feb 10, 2018

JamesWTruher Feb 12, 2018

bergmeister Feb 10, 2018

JamesWTruher Feb 12, 2018 •

edited

Loading

bergmeister Feb 12, 2018

JamesWTruher Feb 13, 2018 •

edited

Loading

bergmeister Feb 10, 2018

JamesWTruher Feb 12, 2018

bergmeister Feb 12, 2018

JamesWTruher Feb 13, 2018

bergmeister Feb 10, 2018

JamesWTruher Feb 12, 2018

bergmeister Feb 10, 2018

JamesWTruher Feb 12, 2018

bergmeister Feb 10, 2018

JamesWTruher Feb 12, 2018

bergmeister Feb 10, 2018 •

edited

Loading

JamesWTruher Feb 12, 2018

JamesWTruher commented Feb 15, 2018 •

edited

Loading

bergmeister commented Feb 15, 2018

JamesWTruher commented Feb 16, 2018

Changes to allow tests to be run outside of CI #882

Changes to allow tests to be run outside of CI #882

Conversation

JamesWTruher commented Feb 10, 2018 • edited Loading

PR Summary

PR Checklist

bergmeister left a comment

Choose a reason for hiding this comment

bergmeister Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

JamesWTruher Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesWTruher Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesWTruher Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesWTruher commented Feb 15, 2018 • edited Loading

bergmeister commented Feb 15, 2018

JamesWTruher commented Feb 16, 2018

JamesWTruher commented Feb 10, 2018 •

edited

Loading

bergmeister Feb 10, 2018 •

edited

Loading

JamesWTruher Feb 12, 2018 •

edited

Loading

JamesWTruher Feb 12, 2018 •

edited

Loading

JamesWTruher Feb 13, 2018 •

edited

Loading

bergmeister Feb 10, 2018 •

edited

Loading

JamesWTruher commented Feb 15, 2018 •

edited

Loading