Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit tests are now running using Pester 5 #41

Merged
merged 24 commits into from
Jun 25, 2021

Conversation

johlju
Copy link
Member

@johlju johlju commented May 28, 2020

Pull Request (PR) description

  • Unit tests are now running using Pester 5 (issue Convert tests to Pester 5 #40).
  • Excludes the PowerShell module script file DscResource.Common.psm1 located
    in folder source from the HQRM testing.
  • Now code coverage is uploaded to codecov.io.

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • Documentation added/updated in README.md.
  • Comment-based help added/updated for all new/changed functions.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Community Testing Guidelines.
  • Integration tests added/updated (where possible). See
    DSC Community Testing Guidelines.
  • New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

@johlju johlju changed the title Fix/pester5 Unit tests are now running using Pester 5 May 28, 2020
@johlju johlju added the on hold The issue or pull request has been put on hold by a maintainer. label May 28, 2020
@johlju
Copy link
Member Author

johlju commented May 28, 2020

This PR is waiting for pipeline fixes to support Pester 5.

@PlagueHO
Copy link
Member

We'll need to release the new sampler version after that.

@johlju
Copy link
Member Author

johlju commented May 29, 2020

Yep, and then wait for the HQRM tests to be resolved (so they can be run in both Pester 4 and Pester 5).

@PlagueHO PlagueHO closed this May 29, 2020
@PlagueHO PlagueHO reopened this May 29, 2020
@PlagueHO PlagueHO closed this May 29, 2020
@PlagueHO PlagueHO reopened this May 29, 2020
@johlju
Copy link
Member Author

johlju commented May 29, 2020

Closing and reopening

@johlju johlju closed this May 29, 2020
@johlju johlju reopened this May 29, 2020
@johlju
Copy link
Member Author

johlju commented May 29, 2020

I will fix the bugs in the unit tests. After that the HQRM tests are dependent on the PR dsccommunity/DscResource.Test#76.

@johlju

This comment has been minimized.

@johlju
Copy link
Member Author

johlju commented May 30, 2020

The failing test have been refactored. This PR is now waiting for the HQRM tests to work in Pester 5.

Base automatically changed from master to main January 18, 2021 13:18
@johlju
Copy link
Member Author

johlju commented Jun 18, 2021

There seems to be a bug in the HQRM tests that need to be sorted before this PR can be merged.

@johlju johlju added needs review The pull request needs a code review. and removed on hold The issue or pull request has been put on hold by a maintainer. labels Jun 20, 2021
@johlju johlju requested a review from PlagueHO June 20, 2021 15:24
@johlju
Copy link
Member Author

johlju commented Jun 20, 2021

@PlagueHO do you have time to review this? No hurry at all. It is a bit large. But there have been no changes to code, just the tests have been modified to work with Pester 5.

@codecov
Copy link

codecov bot commented Jun 20, 2021

Codecov Report

Merging #41 (dafb626) into main (28d4666) will increase coverage by 91.30%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           main      #41       +/-   ##
=========================================
+ Coverage      0   91.30%   +91.30%     
=========================================
  Files         0       26       +26     
  Lines         0      460      +460     
=========================================
+ Hits          0      420      +420     
- Misses        0       40       +40     
Impacted Files Coverage Δ
source/Public/Assert-Module.ps1 100.00% <0.00%> (ø)
source/Public/Get-LocalizedData.ps1 85.41% <0.00%> (ø)
source/Public/Test-IsNanoServer.ps1 100.00% <0.00%> (ø)
source/Public/New-InvalidArgumentException.ps1 100.00% <0.00%> (ø)
source/Public/Get-TemporaryFolder.ps1 100.00% <0.00%> (ø)
source/Private/Test-DscObjectHasProperty.ps1 100.00% <0.00%> (ø)
source/suffix.ps1 100.00% <0.00%> (ø)
source/Public/Set-PSModulePath.ps1 100.00% <0.00%> (ø)
source/Public/Compare-DscParameterState.ps1 80.36% <0.00%> (ø)
source/Public/New-InvalidOperationException.ps1 100.00% <0.00%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28d4666...dafb626. Read the comment docs.

@johlju
Copy link
Member Author

johlju commented Jun 20, 2021

Now the correct code coverage is reported, so the missing code coverage have to be resolved in another PR (unless I messed up and accidently removed a test).

Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor tweaks but looking good - I love the new test init structure (so much improved over the old one 😁).

Reviewed 25 of 28 files at r1, 2 of 3 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @johlju)


cocdecov.yml, line 3 at r5 (raw file):

codecov:
  require_ci_to_pass: no
  # master should be the baseline for reporting

Should be main here


tests/QA/module.tests.ps1, line 24 at r5 (raw file):

Describe 'General module control' -Tags 'FunctionalQuality' {
    It 'imports without errors' {

Message should be 'Should import without errors'


tests/QA/module.tests.ps1, line 63 at r5 (raw file):

            if ($ErrorActionPreference -ne 'Stop')
            {
                Write-Warning -Message "ScriptAnalyzer not found!"

Single quotes here.


tests/QA/module.tests.ps1, line 67 at r5 (raw file):

            else
            {
                throw "ScriptAnalyzer not found!"

Single quotes here.


tests/QA/module.tests.ps1, line 73 at r5 (raw file):

    It "<Name> has a unit test" -TestCases $testCases {
        Get-ChildItem "tests\" -Recurse -Include "$Name.Tests.ps1" | Should -Not -BeNullOrEmpty

Single quotes here and also needs parameter name


tests/QA/module.tests.ps1, line 77 at r5 (raw file):

    It "Script Analyzer for <Name>" -TestCases $testCases -Skip:(-not $scriptAnalyzerRules) {
        $functionFile = Get-ChildItem -path $sourcePath -Recurse -Include "$Name.ps1"

Path parameter name needs caps


tests/QA/module.tests.ps1, line 125 at r5 (raw file):

        $astSearchDelegate = { $args[0] -is [System.Management.Automation.Language.FunctionDefinitionAst] }
        $parsedFunction = $AbstractSyntaxTree.FindAll( $astSearchDelegate, $true ) |
            Where-Object -FilterScript { $_.Name -eq $Name }
{
    $_.Name -eq $Name
}

tests/QA/module.tests.ps1, line 143 at r5 (raw file):

        $astSearchDelegate = { $args[0] -is [System.Management.Automation.Language.FunctionDefinitionAst] }
        $parsedFunction = $AbstractSyntaxTree.FindAll( $astSearchDelegate, $true ) |
            Where-Object -FilterScript { $_.Name -eq $Name }

Can we use:

{
    $_.Name -eq $Name
}

tests/QA/module.tests.ps1, line 147 at r5 (raw file):

        $functionHelp = $parsedFunction.GetHelpContent()

        $parameters = $parsedFunction.Body.ParamBlock.Parameters.Name.VariablePath.ForEach{ $_.ToString() }

Nitpick: ForEach({})


tests/Unit/Public/Compare-DscParameterState.Tests.ps1, line 991 at r5 (raw file):

                It 'Should return $false for PSCredential InDesiredState' {
                    $script:result.where({$_.Property -eq 'PSCredential'}).InDesiredState | Should -BeFalse

Where should be capital 'W' - also can we use:

$script:result.Where({
    $_.Property -eq 'PSCredential'
}).InDesiredState | Should -BeFalse

and throughout.

@johlju johlju requested a review from PlagueHO June 23, 2021 15:25
Copy link
Member Author

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 24 of 28 files at r1, 1 of 3 files at r3, 1 of 1 files at r5, 3 of 3 files at r6.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @PlagueHO)


cocdecov.yml, line 3 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should be main here

Done.


tests/QA/module.tests.ps1, line 24 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Message should be 'Should import without errors'

Done.


tests/QA/module.tests.ps1, line 63 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Single quotes here.

Done.


tests/QA/module.tests.ps1, line 67 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Single quotes here.

Done.


tests/QA/module.tests.ps1, line 73 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Single quotes here and also needs parameter name

Done.


tests/QA/module.tests.ps1, line 77 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Path parameter name needs caps

Done.


tests/QA/module.tests.ps1, line 125 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
{
    $_.Name -eq $Name
}

Done.


tests/QA/module.tests.ps1, line 143 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we use:

{
    $_.Name -eq $Name
}

Done.


tests/QA/module.tests.ps1, line 147 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Nitpick: ForEach({})

Done.


tests/Unit/Public/Compare-DscParameterState.Tests.ps1, line 991 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Where should be capital 'W' - also can we use:

$script:result.Where({
    $_.Property -eq 'PSCredential'
}).InDesiredState | Should -BeFalse

and throughout.

Done. And fixed everywhere... A LOT of them ;)

Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r6.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @johlju)

@PlagueHO PlagueHO merged commit 01ead63 into dsccommunity:main Jun 25, 2021
@johlju johlju removed the needs review The pull request needs a code review. label Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert tests to Pester 5
2 participants