Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upChanges to allow tests to be run outside of CI #882
Conversation
|
Great work but maybe the upgrade to Pester v4 should be a separate PR? |
| @@ -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){ | |||
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 10, 2018
•
Collaborator
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.
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 12, 2018
•
Author
Member
yup, that's the plan, I'll work on that issue
the start is here: #886
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| It "will show the custom rules when given a glob" { | ||
| # needs fixing for Linux | ||
| $expectedNumRules = 4 | ||
| if (Test-PSEditionCoreCLRLinux) | ||
| if ( ! $IsWindows ) |
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 10, 2018
Collaborator
Very minor but here and in other changes the spacing inside the parenthesis is inconsistent.
This comment has been minimized.
This comment has been minimized.
| $initialTestScript = Get-Content $directory\TestScriptWithFixableWarnings.ps1 -Raw | ||
| BeforeAll { | ||
| $scriptName = "TestScriptWithFixableWarnings.ps1" | ||
| $testSource = join-path $PSScriptRoot ${scriptName} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 12, 2018
•
Author
Member
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
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 12, 2018
Collaborator
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?
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 13, 2018
•
Author
Member
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
| @@ -63,7 +63,7 @@ Param | |||
| $RequiredVersion | |||
| ) | |||
|
|
|||
| # #Requires -Module @{ModuleName = 'Pester'; ModuleVersion = '3.4.0'} | |||
| # #Requires -Module @{ModuleName = 'Pester'; ModuleVersion = '4.1.1'} | |||
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 10, 2018
Collaborator
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?
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 12, 2018
Author
Member
the test fixes are sort of entwined, by doing them all at once I won't need to change the tests again.
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 12, 2018
Collaborator
Ok, I see. Then we also need to update the documented version number of Pester in the test section here of the developer guide.
This comment has been minimized.
This comment has been minimized.
| @@ -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')) { | |||
This comment has been minimized.
This comment has been minimized.
bergmeister
Feb 10, 2018
Collaborator
$PSVersionTable.PSVersion.Major -lt 5 looks more elegant to me but this might be just a question of style and therefore opinionated.
This comment has been minimized.
This comment has been minimized.
| } |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 12, 2018
Author
Member
it doesn't look like it. it looks like there was previously no newline at the end of the file.
| @@ -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) { | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -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 { | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Feb 12, 2018
Author
Member
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.
416c805
to
1dc2cf1
Mark some tests as pending that weren't actually doing anything Skip DSC tests on Mac/Linux
1dc2cf1
to
07075e5
Update to use Pester version 4.1.1 Change logic in appveyor to invoke pester only once
07075e5
to
ad17fe8
|
@bergmeister I think this is ready now - what do you think? |
|
@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).
|
|
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. |
JamesWTruher commentedFeb 10, 2018
•
edited
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
xbetween the square brackets. Please mark anything not applicable to this PRNA.WIP:to the beginning of the title and remove the prefix when the PR is ready