Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Added test function for Class based resources and adjusted test logic… #75

Merged
merged 3 commits into from
Oct 25, 2016
Merged

Conversation

bgelens
Copy link
Contributor

@bgelens bgelens commented Oct 24, 2016

… appropriately

This will test the psm1 file containing Class based resources for parse errors but skips the schema check.


This change is Reviewable

@msftclas
Copy link

Hi @bgelens, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@bgelens
Copy link
Contributor Author

bgelens commented Oct 24, 2016

Just ran against DSC Class resource. See results: https://ci.appveyor.com/project/bgelens/dockerdsc/build/0.0.0.5

@kwirkykat kwirkykat added the needs review The pull request needs a code review. label Oct 24, 2016
@kwirkykat
Copy link
Member

I like it 😄 Just a few comments, but nothing major


Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions.


Meta.Tests.ps1, line 205 at r1 (raw file):

-fileName

Since FileName is a parameter, it should be capitalized.

-FileName

Meta.Tests.ps1, line 251 at r1 (raw file):

-fileName

Since FileName is a parameter, it should be capitalized.

-FileName

TestHelper.psm1, line 525 at r1 (raw file):

param(
[Parameter(ValueFromPipeline=$True,Mandatory=$True)]
[string]$fileName
)

The parameter name here needs to be capitalized.
To match the DSC Resource Kit style guidelines (I know there are a lot of them sorry), the param block should look like this:

param
(
    [Parameter(ValueFromPipeline = $true, Mandatory = $true)]
    [String] $FileName
)

TestHelper.psm1, line 528 at r1 (raw file):

$fileName

Since this is a parameter, FileName should be capitalized here as well.

$FileName

TestHelper.psm1, line 530 at r1 (raw file):

$result = foreach

If you just return true from inside the foreach loop, you don't need to catch the result
foreach (...


TestHelper.psm1, line 534 at r1 (raw file):

$true

return $true


TestHelper.psm1, line 539 at r1 (raw file):

if ($result)

Quoted 7 lines of code…

{
    $true
}
else
{
    $false
}

if you just return true in the foreach loop above this can all be replaced with:
return $false


Comments from Reviewable

@kwirkykat kwirkykat added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Oct 24, 2016
@bgelens
Copy link
Contributor Author

bgelens commented Oct 25, 2016

Fixed the issues. I changed the parameter name to Path as it makes more sense and adjusted the functions code to be more condensed.

@kwirkykat kwirkykat added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Oct 25, 2016
@kwirkykat
Copy link
Member

@bgelens Almost there! Just forgot to change param name in the help comment


Reviewed 1 of 2 files at r2.
Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.


TestHelper.psm1, line 525 at r1 (raw file):
The param block still doesn't quite match:

(ValueFromPipeline=$True, Mandatory=$True)

(ValueFromPipeline = $true, Mandatory = $true)

[string]

[String]

Sorry to be nitpicky about it, but since you have another typo to fix might as well do this at the same time.


TestHelper.psm1, line 513 at r2 (raw file):

fileName

Forgot to change the parameter name here


Comments from Reviewable

@bgelens
Copy link
Contributor Author

bgelens commented Oct 25, 2016

Hope this is it :-)

@kwirkykat
Copy link
Member

:lgtm:


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@kwirkykat kwirkykat merged commit a242740 into PowerShell:dev Oct 25, 2016
@kwirkykat kwirkykat removed the needs review The pull request needs a code review. label Oct 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants