P2P-Nathan repair validation errors #19
Conversation
Added AppVeyor testing config, tests work and issues need to be repaired.
…ck.test.template.ps1 as it threw an exception outside of the test, -erroraction was properly set, but the execution still broke.
|
This PR will enable continuous integration testing, results are visible for the test with the inline links or at https://ci.appveyor.com/project/P2P-Nathan/scom-community-catalog/build/1.0.3/tests |
Pack.tests.template.ps1
Outdated
| @@ -47,7 +46,7 @@ foreach ($packFile in $packFiles) { | |||
| Context 'details.json' { | |||
|
|
|||
| It "details.json is valid json" { | |||
| { $contents | ConvertFrom-Json } | Should Not Throw | |||
| {$script:json = $contents | ConvertFrom-Json } | Should Not Throw | |||
There was a problem hiding this comment.
This makes the tests order dependant (other tests will fail if this one isn't run first). The original call probably needs a try/catch wrapping it.
Pack.tests.template.ps1
Outdated
| @@ -33,18 +32,21 @@ foreach ($packFile in $packFiles) { | |||
|
|
|||
| It "Has a details.json" { | |||
| $packFile.FullName | Should Exist | |||
| $packFile.Name | Should BeExactly "details.json" | |||
There was a problem hiding this comment.
In AppVeyor and VSCode only the test name is surfaced - can you either update it to include this restriction or (preferably) create another test? Good unit testing practise is to only test a single thing with each test.
There was a problem hiding this comment.
To increase the readability of the output I'll change the test name to "Has a details.json (case-sensitive)", this should help a user that gets the error even through they have a details.JSON in the folder.
Pack.tests.template.ps1
Outdated
| } | ||
|
|
||
| It "Has a Readme.md" { | ||
| $readme = $packFile.FullName -replace $packFileName, 'README.md' | ||
| $readme | Should Exist | ||
| # Get-ChildItem is used as Get-Item returns the same case it is passed. | ||
| (Get-ChildItem -Path $packFile.DirectoryName -Filter "readme.md").Name | Should BeExactly "ReadMe.md" |
There was a problem hiding this comment.
In AppVeyor and VSCode only the test name is surfaced - can you either update it to include this restriction or (preferably) create another test? Good unit testing practise is to only test a single thing with each test.
(also, feel free to update my incorrectly cased file name above!)
There was a problem hiding this comment.
To increase the readability of the output I'll change the test name to "Has a ReadMe.md (case-sensitive)", this should help a user that gets the error even through they have a README.md in the folder.
|
Might be worth adding this to the top line of the repo readme.md as well: as this will manifest : |
|
"This makes the tests order dependant (other tests will fail if this one
isn't run first). The original call probably needs a try/catch wrapping it."
Does pester run multi-threaded when performing tested?
…On Oct 27, 2017 2:51 AM, "Matthew Long" ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Looks good overall, but some tweaks to the test template are probably
necessary to align with best practice.
------------------------------
In Pack.tests.template.ps1
<#19 (comment)>
:
> @@ -47,7 +46,7 @@ foreach ($packFile in $packFiles) {
Context 'details.json' {
It "details.json is valid json" {
- { $contents | ConvertFrom-Json } | Should Not Throw
+ {$script:json = $contents | ConvertFrom-Json } | Should Not Throw
This makes the tests order dependant (other tests will fail if this one
isn't run first). The original call probably needs a try/catch wrapping it.
------------------------------
In Pack.tests.template.ps1
<#19 (comment)>
:
> @@ -33,18 +32,21 @@ foreach ($packFile in $packFiles) {
It "Has a details.json" {
$packFile.FullName | Should Exist
+ $packFile.Name | Should BeExactly "details.json"
In VSCode only the test name is surfaced - can you either update it to
include this restriction or (preferably) create another test? Good unit
testing practise is to only test a single thing with each test.
------------------------------
In Pack.tests.template.ps1
<#19 (comment)>
:
> }
It "Has a Readme.md" {
$readme = $packFile.FullName -replace $packFileName, 'README.md'
$readme | Should Exist
+ # Get-ChildItem is used as Get-Item returns the same case it is passed.
+ (Get-ChildItem -Path $packFile.DirectoryName -Filter "readme.md").Name | Should BeExactly "ReadMe.md"
In VSCode only the test name is surfaced - can you either update it to
include this restriction or (preferably) create another test? Good unit
testing practise is to only test a single thing with each test.
(also, feel free to update my incorrectly cased file name above!)
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#19 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ARCe2l7H7C5_apfGrQjkuVlgS5xz1X6aks5swYuHgaJpZM4QIJgJ>
.
|
|
Not currently, but it may do in the future. The other danger is simply the tests being re-ordered. |
|
Good to know, I'll get that updated, as well as update the tests for the
details.json and ReadMe.md files.
Thank you for the feedback
…On Fri, Oct 27, 2017 at 8:12 AM, Matthew Long ***@***.***> wrote:
Not currently, but it may do in the future. The other danger is simply the
tests being re-ordered.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#19 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ARCe2rCV9UXcdGSm6bu9g2Nshr6JKIJsks5swda_gaJpZM4QIJgJ>
.
|
|
No problem, just locally tested this and it seemed to do the trick # Even with ErrorAction Silent, this could still throw terminating exceptions
$json = $null
try { $json = $contents | ConvertFrom-Json -ErrorAction SilentlyContinue } catch {} |
Included feedback from PR into changes
|
The noted changes have been added, and the updated tests have been passed. Waiting for a community moderator to approve, then we can pull it in. |
Corrected errors that were preventing the validation success.