Add ThreadJob module package and tests #7169
Conversation
PaulHigin
requested review from
adityapatwardhan,
anmenaga,
daxian-dbw and
TravisEz13
as code owners
PaulHigin
requested review from
SteveL-MSFT
and removed request for
daxian-dbw,
TravisEz13,
anmenaga and
adityapatwardhan
| $results | Should Be "Hello There 60" | ||
| } | ||
|
|
||
| It 'ThreadJob with terminating error' { |
There was a problem hiding this comment.
What about a test for non-terminating error?
... and, in general, tests for verifying that objects in all PS streams (Verbose, Debug, Error, ...) are returned correctly from ThreadJobs.
There was a problem hiding this comment.
This is not intended to be an exhaustive list of job tests, but I have added data stream tests.
| { | ||
| # Start four thread jobs with ThrottleLimit set to two | ||
| Get-Job | where PSJobTypeName -eq "ThreadJob" | Remove-Job -Force | ||
| Start-ThreadJob -ScriptBlock { Start-Sleep -Seconds 60 } -ThrottleLimit 2 |
There was a problem hiding this comment.
This is not related to review; but for my education:
The ThrottleLimit syntax looks interesting; couple of questions:
- What is the scope of ThrottleLimit ? e.g. "all threadjobs in current script" ?
- How do subsequent Start-ThreadJob's in the script know under what ThrottleLimit they operate?
There was a problem hiding this comment.
Throttle limit is per PowerShell session so it limits all concurrent threadjobs created in that session. The queue is static (singleton). I currently don't have a way to get the current throttle limit value ... you just set it. But something like that can be added.
See source code for more details:
https://github.com/PaulHigin/PSThreadJob/blob/master/ThreadJob/ThreadJob/PSThreadJob.cs#L18
| } | ||
| } | ||
|
|
||
| Describe 'Job2 Tests' -Tags 'CI' { |
There was a problem hiding this comment.
I would rename the Describe to "Job2 class API tests" or something like this.
For an unfamiliar person it is not clear what 'Job2' is.
|
|
||
| $job = Start-ThreadJob -ScriptBlock { "Hello" } | ||
| $results = $job | Receive-Job -Wait | ||
| $results | Should be "Hello" |
There was a problem hiding this comment.
Can we use Pester 4 syntax?
$results | Should -Be "Hello"There was a problem hiding this comment.
The build only pulls in Pester 3.3.9 right now which does not support this syntax. We should wait to update tests until the default pester version supports the new syntax.
|
|
||
| It 'ThreadJob with terminating error' { | ||
|
|
||
| $job = Start-ThreadJob -ScriptBlock { throw "MyError!" } |
There was a problem hiding this comment.
Can we also add a test for a non-terminating error where some objects are returned and 1 non-terminating error is returned?
There was a problem hiding this comment.
This is outside the scope of this change. Please create an Issue for adding more threadjob tests.
|
|
||
| It 'Verifies terminating job error' { | ||
|
|
||
| $job = Start-ThreadJob -ScriptBlock { throw "My Job Error!" } | Wait-Job |
There was a problem hiding this comment.
There was a problem hiding this comment.
It is a slightly different test since it involves Receive-Job, so I'd like to keep it.
| $job3 | Remove-Job -Force | ||
|
|
||
| (Get-Runspace).Count | Should Be ($rsStartCount + 2) | ||
|
|
There was a problem hiding this comment.
Thanks, but will keep for readability.
| Describe 'Job2 Tests' -Tags 'CI' { | ||
|
|
||
| AfterEach { | ||
|
|
There was a problem hiding this comment.
Thanks, but will keep for readability.
| } | ||
|
|
||
| AfterEach { | ||
|
|
There was a problem hiding this comment.
Thanks, but will keep for readability.
|
@adityapatwardhan can you update your review? |
|
|
||
| It 'ThreadJob with ScriptBlock and Initialization script' { | ||
|
|
||
| $job = Start-ThreadJob -ScriptBlock { "Goodbye" } -InitializationScript { "Hello" } |
There was a problem hiding this comment.
Can this actually take a script (i.e. a path to a script) or just a scriptblock. If only takes a scriptblock, then the parameter should probably be named "-InitializationScriptBlock".
There was a problem hiding this comment.
Good point. It just takes a scriptblock. But I used the same parameter name as what Start-Job uses, which also just takes a scriptblock. So for consistency we should probably keep the parameter name as is (or change both).
There was a problem hiding this comment.
I was just thinking of consistency between -ScriptBlock and -InitialScriptBlock. But following what existing commands do is probably fine.
|
@TravisEz13 I just found a bug in this version of ThreadJob. Please hold off merging until I have a fix ready. Thanks. |
TravisEz13
changed the title
|
@PaulHigin to prevent maintainers from merging, rename your PR with WIP in front of the title. remove WIP to unblock. This will fail the Work In Progress (WIP), CI test immediately. |
PaulHigin
changed the title
|
I have fixed the bug, updated tests, and updated the package with the new version. Please take a look. |
|
@adityapatwardhan Can you please review changes? |
|
|
||
| # Get-Job -Filter is not supported | ||
| $result = Get-Job -Filter @{Id = ($job.Id)} 3>$null | ||
| $result | Should -Be $null |
There was a problem hiding this comment.
This should be Should -BeNullOrEmpty
assets/files.wxs
Outdated
| @@ -1826,7 +1826,7 @@ | |||
| </Directory> | |||
| </Directory> | |||
| <Directory Id="dir37EBACBC570E49108415C3BA316EB66D" Name="ThreadJob"> | |||
| <Directory Id="dirCB93DAC4CEB64A8583295B9AC4A79883" Name="1.1.0"> | |||
| <Directory Id="dirCB93DAC4CEB64A8583295B9AC4A79883" Name="1.1.1"> | |||
There was a problem hiding this comment.
Not an issue for this PR. We should consider not putting the modules in a version folder. If we have to update the modules, this will make patching very difficult.
PR Summary
This adds the ThreadJob module package from the PSGallery, along with Pester tests
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests