dsccommunity / ComputerManagementDsc

DSC resources for for configuration of a Windows computer. These DSC resources allow you to perform computer management tasks, such as renaming the computer, joining a domain and scheduling tasks as well as configuring items such as virtual memory, event logs, time zones and power settings.
https://dsccommunity.org
MIT License
303 stars 83 forks source link

ScheduledTask - Update allowed values of ScheduleType to accept AtLogon #421

Closed jordanbreen28 closed 10 months ago

jordanbreen28 commented 10 months ago

Pull Request (PR) description

See https://github.com/dsccommunity/ComputerManagementDsc/issues/420

This Pull Request (PR) fixes the following issues

Fixes https://github.com/dsccommunity/ComputerManagementDsc/issues/420

Task list


This change is Reviewable

codecov[bot] commented 10 months ago

Codecov Report

Merging #421 (a7bf869) into main (9566942) will increase coverage by 0%. Report is 1 commits behind head on main. The diff coverage is 100%.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/dsccommunity/ComputerManagementDsc/pull/421/graphs/tree.svg?width=650&height=150&src=pr&token=n7Sx5K7YGT&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity)](https://app.codecov.io/gh/dsccommunity/ComputerManagementDsc/pull/421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) ```diff @@ Coverage Diff @@ ## main #421 +/- ## =================================== Coverage 90% 90% =================================== Files 18 18 Lines 1801 1801 =================================== + Hits 1628 1631 +3 + Misses 173 170 -3 ``` | [Files](https://app.codecov.io/gh/dsccommunity/ComputerManagementDsc/pull/421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) | Coverage Ξ” | | |---|---|---| | [...Resources/DSC\_ScheduledTask/DSC\_ScheduledTask.psm1](https://app.codecov.io/gh/dsccommunity/ComputerManagementDsc/pull/421?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity#diff-c291cmNlL0RTQ1Jlc291cmNlcy9EU0NfU2NoZWR1bGVkVGFzay9EU0NfU2NoZWR1bGVkVGFzay5wc20x) | `87% <100%> (+<1%)` | :arrow_up: |
jordanbreen28 commented 10 months ago

no additional test coverage is required, unsure why code coverage has failed.

johlju commented 10 months ago

It failed because it seems there a no current tests for the code path for which a line was modified in this change: https://app.codecov.io/gh/dsccommunity/ComputerManagementDsc/pull/421/blob/source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.psm1#L672 So the check that failed, it reports that there were not at least 90% coverage of all the changed lines.

Suggest adding a test the verifies that the output is exactly AtLogon (or the same as the schema) to make sure that there are no regressions.

jordanbreen28 commented 10 months ago

hey @johlju thanks for reviewing. I'll try to get something in, but I am very unfamiliar with DSC. Looks to me like there is already a check to ensure that the returned value is equal to AtLogon here https://github.com/dsccommunity/ComputerManagementDsc/blob/main/tests/Unit/DSC_ScheduledTask.Tests.ps1#L2206.

johlju commented 10 months ago

Yes, my bad. The code I referenced was in the Set-function. Get-function is fully covered.

But that line you referenced I think it should use BeExactly (or use regex matching) to make sure it does a case-sensitive tests of the string? The test should fail if Get-function would return the wrong casing. πŸ€”

jordanbreen28 commented 10 months ago

@johlju I've added a test now. it may very well be horrible so open to suggested improvements!

jordanbreen28 commented 10 months ago

Ah I give up with the code coverage.. Can't see what I'm missing here

johlju commented 10 months ago

The new tests you added does not call any of the functions Get-TargetResource, Test-TargetResource or Set-TargetResource, so the test doesn't actually test anything in the resource so the code coverage cannot change. πŸ™‚

If you would like to make the status check codecov/patch "green" then you need to add a test that calls Set-TargetResource with the correct parameters so it enters into the correct code path that should be tested, in this case at least the ScheduleType should be set to AtLogon and User to something like 'MockedUser. Then add a mock for New-ScheduledTaskTrigger and then assert that it was called with the expected parameters. Do something similar to this:

https://github.com/dsccommunity/ComputerManagementDsc/blob/a077da11152990e2acd85a615d0cc5d5d83d96e4/tests/Unit/DSC_ScheduledTask.Tests.ps1#L1642-L1667

But you only need an It-block that calls Set-TargetResource (change to correct mock parameters), change the mock to New-ScheduledTaskTrigger and assert that it was called with the correct parameters after you called Set-TargetResource by using something similar to:

Assert-MockCalled -CommandName New-ScheduledTaskTrigger -ParameterFilters {
    $AtLogon -eq $true -and $User -eq 'MockedUser'
} -Exactly -Times 1 -Scope It

Let me know if you get into any issues πŸ™‚

jordanbreen28 commented 10 months ago

The new tests you added does not call any of the functions Get-TargetResource, Test-TargetResource or Set-TargetResource, so the test doesn't actually test anything in the resource so the code coverage cannot change. πŸ™‚

If you would like to make the status check codecov/patch "green" then you need to add a test that calls Set-TargetResource with the correct parameters so it enters into the correct code path that should be tested, in this case at least the ScheduleType should be set to AtLogon and User to something like 'MockedUser. Then add a mock for New-ScheduledTaskTrigger and then assert that it was called with the expected parameters. Do something similar to this:

https://github.com/dsccommunity/ComputerManagementDsc/blob/a077da11152990e2acd85a615d0cc5d5d83d96e4/tests/Unit/DSC_ScheduledTask.Tests.ps1#L1642-L1667

But you only need an It-block that calls Set-TargetResource (change to correct mock parameters), change the mock to New-ScheduledTaskTrigger and assert that it was called with the correct parameters after you called Set-TargetResource by using something similar to:


Assert-MockCalled -CommandName New-ScheduledTaskTrigger -ParameterFilters {

    $AtLogon -eq $true -and $User -eq 'MockedUser'

} -Exactly -Times 1 -Scope It

Let me know if you get into any issues πŸ™‚

@johlju understood! That is much clearer now. I'll get the commit amended as soon as I can. Thanks for helping me push through with my lack of dsc/powershell expertise! πŸ˜€

jordanbreen28 commented 10 months ago

hey @johlju - I've hit a bit of a wall. Tried every combination possible that I can think of, even broke out the help of AI 😬 Cannot get a working unit test case with suitable mocks.. Appreciate your help for getting to this point.

johlju commented 10 months ago

I will have a look tomorrow.

jordanbreen28 commented 10 months ago

I will have a look tomorrow.

Thank you. I'll keep trying.

PlagueHO commented 10 months ago

Sorry I haven't had time to look at this. I won't have time till later this week unfortunately.

johlju commented 10 months ago

@jordanbreen28 this one wasn't straight forward - it took me a little googling to get this one right, but this seems to work:

Context "When scheduling a task to trigger at user logon" {
    BeforeAll {
        Mock -CommandName New-ScheduledTaskTrigger -MockWith {
            $cimInstance = New-CIMInstance -ClassName 'MSFT_TaskLogonTrigger' -Namespace 'root\Microsoft\Windows\TaskScheduler' -Property @{
                # Fill the CIM instance with the properties we expect to be used by the resource.
                UserId = $testParameters.User
                Delay  = ''
            } -ClientOnly

            <#
                Must add the TypeName property to the CIM instance for the array .PSObject.PSTypeNames
                to have the correct name for it to be recognized by the New-ScheduledTask command.
            #>
            $cimInstance | Add-Member -TypeName 'Microsoft.Management.Infrastructure.CimInstance#MSFT_TaskTrigger'

            return $cimInstance
        }

        Mock -CommandName New-ScheduledTask -MockWith {
            <#
                Mock an object with properties that are used by the resource
                for the newly created scheduled task.
            #>
            return [PSCustomObject] @{
                Triggers = @(
                    @{
                        StartBoundary = '2018-09-27T18:45:08+02:00'
                    }
                )
            }
        }

        Mock -CommandName Register-ScheduledTask
    }

    It "Should correctly configure the task with 'AtLogon' ScheduleType and the specified user" {
        $testParameters = $getTargetResourceParameters + @{
            ScheduleType      = 'AtLogon'
            User              = 'MockedUser'
            ActionExecutable  = 'C:\windows\system32\WindowsPowerShell\v1.0\powershell.exe'
            LogonType         = 'Password'
        }

        Set-TargetResource @testParameters

        Assert-MockCalled -CommandName New-ScheduledTaskTrigger -ParameterFilter {
            $AtLogon -eq $true -and $User -eq 'MockedUser'
        } -Exactly -Times 1 -Scope It

        Assert-MockCalled -CommandName New-ScheduledTask -Exactly -Times 1 -Scope It
    }
}
jordanbreen28 commented 10 months ago

@johlju Amazing! Lets try that.

jordanbreen28 commented 10 months ago

@johlju and we're green... thanks for all your support on this

johlju commented 10 months ago

@jordanbreen28 you're welcome. Thank you for contributing and raising the code coverage.

PlagueHO commented 10 months ago

Thank you @jordanbreen28 and @johlju !