ScheduledTask: Add 'StopExisting' to valid values for MultipleInstances parameter #335

Rob-S · 2020-05-13T01:09:44Z

Pull Request (PR) description

Add 'StopExisting' to valid values for ScheduledTask MultipleInstances parameter - fixes Issue #333

This Pull Request (PR) fixes the following issues

Fixes Issue #333

Task list

[√] Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
[√] Resource documentation added/updated in README.md in resource folder.
[√] Resource parameter descriptions added/updated in schema.mof
and comment-based help.
[N/A] Comment-based help added/updated.
[N/A] Localization strings added/updated in all localization files as appropriate.
[N/A] Examples appropriately added/updated.
[√] Unit tests added/updated. See DSC Resource Testing Guidelines.
[N/A] Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
[√] New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is

…s parameter - fixes Issue dsccommunity#333

Rob-S · 2020-05-13T05:54:40Z

The HQRM tests are failing with the error below for all the examples for every resource, saying Use Import-DSCResource to import the resource. However, the examples do import the resources using the module name: Import-DscResource -ModuleName ComputerManagementDsc. Has something gone awry with the tests?

    Context ScheduledTask\1-ScheduledTask_CreateScheduledTaskOnce_Config.ps1
      [-] Should compile MOFs for example correctly 86ms
        Expected no exception to be thrown, but an exception "At D:\a\1\s\source\Examples\Resources\ScheduledTask\1-ScheduledTask_CreateScheduledTaskOnce_Config.ps1:35 char:9
        +         ScheduledTask ScheduledTaskOnceAdd
        +         ~~~~~~~~~~~~~
        Undefined DSC resource 'ScheduledTask'. Use Import-DSCResource to import the resource." was thrown from D:\a\1\s\source\Examples\Resources\ScheduledTask\1-ScheduledTask_CreateScheduledTaskOnce_Config.ps1:35 char:9
            +         ScheduledTask ScheduledTaskOnceAdd
            +         ~~~~~~~~~~~~~.
        141:                     } | Should -Not -Throw
        at <ScriptBlock>, D:\a\1\s\output\RequiredModules\DscResource.Test\0.13.0\Tests\QA\ExampleFiles.common.Tests.ps1: line 27

PlagueHO · 2020-05-13T07:30:57Z

Hi @Rob-S - this is due to a change to recent ModuleBuilder change. I've got a PR #334 open that will resolve it. Once that is trough the build should work properly again.

PlagueHO · 2020-05-14T04:38:58Z

You should be able to rebase and resolve conflicts and tests should pass.

Rob-S · 2020-05-20T19:38:08Z

I had resolved the conflict in the CHANGELOG.md file. Does anything else need done on this?

PlagueHO · 2020-05-20T23:42:45Z

Hi @Rob-S - sorry, this is on me. I'll review this weekend (this week is a bit crazy with MS Build and so none of my evenings are free).

PlagueHO

Really sorry about the delay in getting this reviewed @Rob-S.

Is it possible to add or update the integration tests to cover this so that we can confirm it will work OK in practice against different OS?

Reviewed 3 of 5 files at r1, 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rob-S)

CHANGELOG.md, line 13 at r3 (raw file):

    by changing `CopyDirectories` to `CopyPaths` - Fixes [Issue #332](https://github.com/dsccommunity/ComputerManagementDsc/issues/332).

- StartedTask

Can you change this to ScheduledTask?

source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.psm1, line 554 at r3 (raw file):

        }

        if ($MultipleInstances -ne 'StopExisting')

Can you add a comment to this code just explaining why it is required? I can see it is covered in the Known Issues, but someone working on this might not see it.

Rob-S · 2020-06-10T16:15:11Z

I did try adding a unit test (I took a look at the integration tests, but I'm not really familiar with how those work yet). In the pipeline run, it looks like it's not even trying to run the tests, saying "Cannot bind argument to parameter 'Path' because it is null." although I do see a "Project Path" parameter is set.

PlagueHO · 2020-06-13T23:57:14Z

Hi @Rob-S - the build failure is caused by a recent release of Module Builder. I've fixed the issue in this PR #334 - if you rebase onto master again the build should pass - then we can take a look at the integration tests.

PlagueHO · 2020-06-14T00:52:11Z

Hi @Rob-S - Sorry, correction, the issue you're seeing is because of the Pester v5.x release. I'm going to pin this module to remain on Pester 4.10.1.

PlagueHO · 2020-06-14T01:28:16Z

Once this PR #337 is through you can rebase and the tests will hopefully pass.

…pleInstances-Parameter-Should-Allow-For-'StopExisting'

PlagueHO · 2020-06-15T06:15:01Z

@Rob-S - can you try rebasing your branch onto master again to see if that fixes the build?

PlagueHO · 2020-06-17T06:28:09Z

Hi @Rob-S - looks like the tests are failing. I think this may be because:https://dev.azure.com/dsccommunity/ComputerManagementDsc/_build/results?buildId=2305&view=logs&j=158455ef-2db5-56fd-6022-83ded799e050&t=b737ddad-5217-50db-260a-b6e0dd548ea1&l=933

Rob-S · 2020-06-17T14:37:59Z

@PlagueHO Yes, I had fixed the tests around the same time you entered that comment.

… Set-TargetResource

PlagueHO

I think what we need is a unit test that tests when MultipleInstances is set to StopExisting. I could be wrong, but I didn't see one. Is it easy to add that?

Reviewed 1 of 1 files at r5, 1 of 2 files at r7, 1 of 1 files at r9.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Rob-S)

CHANGELOG.md, line 14 at r9 (raw file):

  - Pin `Pester` module to 4.10.1 because Pester 5.0 is missing code
    coverage - Fixes [Issue #336](https://github.com/dsccommunity/ComputerManagementDsc/issues/336).

Can you remove this blank line?

source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.psm1, line 1832 at r9 (raw file):

    #>
        $MultipleInstances = [System.String] $settings.MultipleInstances
        if ([String]::IsNullOrEmpty($MultipleInstances))

Can you change [String] to [System.String]?

tests/Unit/DSC_ScheduledTask.Tests.ps1, line 115 at r9 (raw file):

                }

                $scheduledTask = $NULL

Can you change to $null (lowercase)

Rob-S · 2020-06-26T02:54:51Z

I think what we need is a unit test that tests when MultipleInstances is set to StopExisting. I could be wrong, but I didn't see one. Is it easy to add that?

No. It's easy to add it for the other values, but not for StopExisting, since the Enum does not support that. See the comments for my commits when I attempted to do that. The issue is that you have to go to the SettingsSet level and if you try to mock that function it fails miserably elsewhere -- the "real" SettingsSet functions must be used.

PlagueHO

Thanks @Rob-S - let's get this through and I'll see if I can figure out away to abstract the function to increase testability on it. As long as it is not breaking any existing functionality (the existing tests confirm that), then we should be OK. I'll raise another issue to increase coverage on that condition.

Reviewed 3 of 3 files at r10.
Reviewable status: complete! all files reviewed, all discussions resolved

PlagueHO

Reviewable status: complete! all files reviewed, all discussions resolved

Rob-S added 2 commits May 12, 2020 20:04

Add 'StopExisting' to valid values for ScheduledTask MultipleInstance…

12941a9

…s parameter - fixes Issue dsccommunity#333

convert .mof file

5c5f518

Split line

17ef187

Resolve conflict in CHANGELOG.md

79ced86

PlagueHO added the needs review The pull request needs a code review. label May 20, 2020

PlagueHO requested changes Jun 8, 2020

View reviewed changes

Issue-dsccommunity#333 Commit requested changes

830db90

Merge branch 'master' into Issue-dsccommunity#333-ScheduledTask-Multi…

2972d8c

…pleInstances-Parameter-Should-Allow-For-'StopExisting'

Rob-S added 3 commits June 17, 2020 00:21

Issue-dsccommunity#333 Workaround for Get-ScheduledTask bug

a3f5a28

Issue-dsccommunity#333 Add new lines

abb57a1

Issue-dsccommunity#333 Change to use IsNullOrEmpty

20f0b89

Issue-dsccommunity#333 Add default values for settings in unit tests

2ba9fff

Rob-S added 5 commits June 17, 2020 09:51

Issue-dsccommunity#333 Moved Get-TargetResource in same "It" block as…

995b3f4

… Set-TargetResource

Issue-dsccommunity#333 Mock Register-ScheduledTask in unit tests

b130b70

Issue-dsccommunity#333 Assert-MockCalled in unit tests

f0b1d7b

Issue-dsccommunity#333 Add mock function New-ScheduledTaskSettingsSet

49f0a10

Issue-dsccommunity#333 Giving up, removing tests

ab45487

PlagueHO requested changes Jun 24, 2020

View reviewed changes

Issue-dsccommunity#333 with requested changes

2deef34

PlagueHO approved these changes Jun 26, 2020

View reviewed changes

PlagueHO merged commit 47d9409 into dsccommunity:master Jun 26, 2020

PlagueHO mentioned this pull request Jun 26, 2020

ScheduledTask: Add Test to cover 'StopExisting' value in MultipleInstances Parameter #339

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ScheduledTask: Add 'StopExisting' to valid values for MultipleInstances parameter #335

ScheduledTask: Add 'StopExisting' to valid values for MultipleInstances parameter #335

Rob-S commented May 13, 2020 •

edited by johlju

Loading

Rob-S commented May 13, 2020

PlagueHO commented May 13, 2020

PlagueHO commented May 14, 2020

Rob-S commented May 20, 2020 •

edited

Loading

PlagueHO commented May 20, 2020

PlagueHO left a comment

Rob-S commented Jun 10, 2020

PlagueHO commented Jun 13, 2020

PlagueHO commented Jun 14, 2020

PlagueHO commented Jun 14, 2020

PlagueHO commented Jun 15, 2020

PlagueHO commented Jun 17, 2020

Rob-S commented Jun 17, 2020 •

edited

Loading

PlagueHO left a comment

Rob-S commented Jun 26, 2020 •

edited

Loading

PlagueHO left a comment

PlagueHO left a comment

ScheduledTask: Add 'StopExisting' to valid values for MultipleInstances parameter #335

ScheduledTask: Add 'StopExisting' to valid values for MultipleInstances parameter #335

Conversation

Rob-S commented May 13, 2020 • edited by johlju Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

Rob-S commented May 13, 2020

PlagueHO commented May 13, 2020

PlagueHO commented May 14, 2020

Rob-S commented May 20, 2020 • edited Loading

PlagueHO commented May 20, 2020

PlagueHO left a comment

Choose a reason for hiding this comment

Rob-S commented Jun 10, 2020

PlagueHO commented Jun 13, 2020

PlagueHO commented Jun 14, 2020

PlagueHO commented Jun 14, 2020

PlagueHO commented Jun 15, 2020

PlagueHO commented Jun 17, 2020

Rob-S commented Jun 17, 2020 • edited Loading

PlagueHO left a comment

Choose a reason for hiding this comment

Rob-S commented Jun 26, 2020 • edited Loading

PlagueHO left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

Rob-S commented May 13, 2020 •

edited by johlju

Loading

Rob-S commented May 20, 2020 •

edited

Loading

Rob-S commented Jun 17, 2020 •

edited

Loading

Rob-S commented Jun 26, 2020 •

edited

Loading