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

Add ThreadJob module package and tests #7169

Merged
merged 5 commits into from Jul 6, 2018

Conversation

@PaulHigin
Copy link
Contributor

commented Jun 25, 2018

PR Summary

This adds the ThreadJob module package from the PSGallery, along with Pester tests

PR Checklist

@SteveL-MSFT
Copy link
Member

left a comment

LGTM

$results | Should Be "Hello There 60"
}

It 'ThreadJob with terminating error' {

This comment has been minimized.

Copy link
@anmenaga

anmenaga Jun 25, 2018

Contributor

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.

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Jun 25, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@anmenaga

anmenaga Jun 25, 2018

Contributor

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?

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Jun 25, 2018

Author Contributor

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' {

This comment has been minimized.

Copy link
@anmenaga

anmenaga Jun 25, 2018

Contributor

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.

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Jun 25, 2018

Author Contributor

Ok.


$job = Start-ThreadJob -ScriptBlock { "Hello" }
$results = $job | Receive-Job -Wait
$results | Should be "Hello"

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Jun 25, 2018

Member

Can we use Pester 4 syntax?

$results | Should -Be "Hello"

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Jun 26, 2018

Author Contributor

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!" }

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Jun 25, 2018

Member

Can we also add a test for a non-terminating error where some objects are returned and 1 non-terminating error is returned?

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Jun 25, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Jun 26, 2018

Author Contributor

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)

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Jun 25, 2018

Member

extra empty line

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Jun 26, 2018

Author Contributor

Thanks, but will keep for readability.

Describe 'Job2 Tests' -Tags 'CI' {

AfterEach {

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Jun 25, 2018

Member

extra empty line

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Jun 26, 2018

Author Contributor

Thanks, but will keep for readability.

}

AfterEach {

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Jun 25, 2018

Member

extra empty line

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Jun 26, 2018

Author Contributor

Thanks, but will keep for readability.

@TravisEz13

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

@adityapatwardhan can you update your review?


It 'ThreadJob with ScriptBlock and Initialization script' {

$job = Start-ThreadJob -ScriptBlock { "Goodbye" } -InitializationScript { "Hello" }

This comment has been minimized.

Copy link
@BrucePay

BrucePay Jun 26, 2018

Collaborator

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".

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Jun 26, 2018

Author Contributor

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).

This comment has been minimized.

Copy link
@BrucePay

BrucePay Jun 27, 2018

Collaborator

I was just thinking of consistency between -ScriptBlock and -InitialScriptBlock. But following what existing commands do is probably fine.

@PaulHigin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

@TravisEz13 I just found a bug in this version of ThreadJob. Please hold off merging until I have a fix ready. Thanks.

@TravisEz13 TravisEz13 changed the title Add ThreadJob module package and tests WIP Add ThreadJob module package and tests Jun 27, 2018

@TravisEz13

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

@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 PaulHigin changed the title WIP Add ThreadJob module package and tests Add ThreadJob module package and tests Jun 29, 2018

@PaulHigin PaulHigin force-pushed the PaulHigin:add-threadjob-module branch to 452f4fa Jun 29, 2018

@PaulHigin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

I have fixed the bug, updated tests, and updated the package with the new version. Please take a look.

@PaulHigin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

@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

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Jul 2, 2018

Member

This should be Should -BeNullOrEmpty

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Jul 2, 2018

Author Contributor

Fixed

@@ -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">

This comment has been minimized.

Copy link
@TravisEz13

TravisEz13 Jul 6, 2018

Member

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.

This comment has been minimized.

Copy link
@TravisEz13

@TravisEz13 TravisEz13 merged commit a57fafe into PowerShell:master Jul 6, 2018

5 checks passed

CodeFactor No issues found.
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@PaulHigin PaulHigin deleted the PaulHigin:add-threadjob-module branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.