Provide additional tests for the *-Job cmdlets#3110
Provide additional tests for the *-Job cmdlets#3110daxian-dbw merged 1 commit intoPowerShell:masterfrom jeffbi:Jobs
Conversation
iSazonov
left a comment
There was a problem hiding this comment.
Should we mark these tests by Slow tag?
There was a problem hiding this comment.
$j.gettype() can raise exception.
Maybe better:
$j | Should BeOfType System.Management.Automation.JobThere was a problem hiding this comment.
The same:
Get-Job -id $j.id | Should BeOfType System.Management.Automation.JobThere was a problem hiding this comment.
It is seems we can remove $jobs.gettype().fullname | should be "System.Object[]" because next two statement make all needed checks well.
And again:
$job | Should BeOfType System.Management.Automation.JobThere was a problem hiding this comment.
Why pipe? It is confused with | Should Be.
We could use:
Remove-Job -Job $j -ErrorAction SilentlyContinue
There was a problem hiding this comment.
Please replace -ea Stop with -ErrorAction Stop
There was a problem hiding this comment.
I don't like this idea - it can exhaust all CPU resources.
We could utilize something like get-content $TestDrive\jobs.txt -wait to get chunks in the job and send them to output.
There was a problem hiding this comment.
Not sure I understand this. What would be adding data to jobs.txt while get-content waits? Would we use yet another background job for this?
Would injecting a short start-sleep (say .25 sec) mitigate the CPU usage concern?
There was a problem hiding this comment.
Perhaps it is justified in these tests.
If no I would prefer synchronous and more deterministic tests.
0.25 is 250 ms so we could use file IO to be even faster.
- The test write a test string to a file.
- "start-job" job read (Get-Content -wait) the string and send to output
- The test call Receive-Job (in loop up to 5 sec) to get the test string
There was a problem hiding this comment.
I think @iSazonov means this:
BeforeAll {
New-Item $TestDrive\jobs.txt -type File
$j = Start-Job { get-content $TestDrive\jobs.txt -wait }
}
AfterAll {
Get-Job | Remove-Job -Force
}
It "....." {
Add-Content $TestDrive\job.txt -Value "Hello"
Receive-Job $j | should be "Hello"
Add-Content $TestDrive\job.txt -Value "World"
Receive-Job $j | should be "World"
}
It "...." {
Add-Content $TestDrive\job.txt -Value "Keep"
Receive-Job $j -Keep | should be "Keep"
Add-Content $TestDrive\job.txt -Value "Output"
(Receive-Job $j) -Join "," | should be "Keep,Output"
}
I think this is better than the loop approach because 1) it's simpler to read and understand; 2) in case that receive-job really is broken, the loop might leave the test in a bad place.
There was a problem hiding this comment.
@daxian-dbw Thanks! 👍
I believe we should put Receive-Job $j -Keep in waiting loop:
function WaitJobResult {
$result = Receive-Job $j -Keep
$startTime = [Datetime]::Now
while ($rerult -eq $null -or ([Datetime]::Now - $startTime) -lt 5)
{
Start-Sleep -Milliseconds 50
$result = Receive-Job $j -Keep
}
}
WaitJobResult | should be "Hello"
There was a problem hiding this comment.
There seems to be some problem with comments on GitHub 😖
There was a problem hiding this comment.
The while loop can be infinite 😕
There was a problem hiding this comment.
Will it? $n will only be set to 5 or 10, and unless Receive-Job never returns enough output to meet those numbers the loop should end pretty quickly.
There was a problem hiding this comment.
Here we test *-Job cmdlets so we cannot trust them.
We get results from Receive-Job. If Receive-Job will get the same results every call we shall get infinite loop.
There was a problem hiding this comment.
Quite right. Changed to avoid endless loop.
There was a problem hiding this comment.
Don't Remove-Job stop a job?
There was a problem hiding this comment.
lines 87 to 89 might not be necessary for this test as that behavior has already been exercised by the above test.
There was a problem hiding this comment.
This can be changed to $results = @(Receive-Job -Keep $job)
There was a problem hiding this comment.
It "Start-Job produces a job object" { $j.gettype().fullname | should be "System.Management.Automation.PSRemotingJob" } It "Start-Job can name the job" { $j = Start-Job -ScriptBlock {6 * 7} -Name "My Job" $j.Name | should be "My Job" } It "Get-Job retrieves a job object" { (Get-Job -id $j.id).gettype().fullname | should be "System.Management.Automation.PSRemotingJob" }
This may not strictly relevant, but for the 3 simple tests above, it seems too heavy to run start-job -scriptblock { 1 + 1 }, which essentially starts a process, before each test. Can we change the BeforeEach to
BeforeEach {
$j = start-job -scriptblock { 1 + 1 } -Name "My Job"
}
and group those 3 tests into one?
There was a problem hiding this comment.
We can certainly ad the -Name parameter in the BeforeEach block and merge the first two test together. Personally, I'd rather keep the Get-Job test separate.
There was a problem hiding this comment.
doesn't the AfterEach remove the job? Do you need to start a job here?
There was a problem hiding this comment.
why do you want to replace the original { start-sleep 15 } with this script block? None of the tests in this Context check the output of the job, so it's not necessary to write out anything. If you think 15 seconds is more than needed, maybe just { start-sleep 10 }?
There was a problem hiding this comment.
That was done when I expected the Receive-Job tests to be in this Context block. Reverted back to Start-Sleep.
There was a problem hiding this comment.
It seems we should remove the line.
There was a problem hiding this comment.
Darn. I thought I had. Removed now.
iSazonov
left a comment
There was a problem hiding this comment.
We now have mixed capitalization for cmdlet names, parameter names, "should be","should beoftype". It is difficult to read. We should fix this and use "Should Be", "Should BeOfType".
There was a problem hiding this comment.
It seems we should remove the test because we already have a test for Wait-Job and have tests for Receive-Jobs too. (So much that the test contains errors)
There was a problem hiding this comment.
This test is about receiving all the output. The other tests are about receiving partial output.
I'll remove this one if you prefer.
There was a problem hiding this comment.
Next test implicitly check the same but it can be modified in future so we should leave the test with corrections:
It "Receive-Job can retrieve job results" {
Wait-Job -Timeout 10 -id $j.id | Should Not BeNullOrEmpty
receive-job -id $j.id | Should be 2
}There was a problem hiding this comment.
Usually we explicitly use a string in quotes "Completed".
There was a problem hiding this comment.
It makes no sense to make 'Should Be' for every array member.
Maybe better:
checkContent $array | Should Be $true
function checkContent($array)
{
for ($i=1; $i -lt $array.Count; $i++)
{
if ( $array[$i] -ne ($array[$i-1] + 1) ) { return $false}
}
return $true
}There was a problem hiding this comment.
Both checkContent should be removed because this has already been checked in the previous test.
And it makes no sense to make 'Should Be' for every array member. We could use Compare-Object here.
There was a problem hiding this comment.
Now using Compare-Object to compare the beginning of the second Receive-Job with the results of the first, and a single check to ensure that the second Receive-Job didn't miss any data after the first.
There was a problem hiding this comment.
The same about string quotes .
There was a problem hiding this comment.
Please remove the debug code.
There was a problem hiding this comment.
if ($count -eq 10000)
It should be 1000 instead, otherwise the timeout would be 50 minutes 😄
There was a problem hiding this comment.
To be accurate, we should use the Perser style:
{ throw "qwerty" } | Should Not ThrowThere was a problem hiding this comment.
Comparing
PS:89> describe "abc" {
>> it "def" { throw 'Receive-Job behaves suspiciously: Cannot receive 5 results in 5 minutes.' }
>> it "hgi" { $true | Should be $true }
>> }
Describing abc
[-] def 25ms
Receive-Job behaves suspiciously: Cannot receive 5 results in 5 minutes.
at line: 2 in
[+] hgi 15ms
with
PS:90> describe "abc" {
>> it "def" { { throw 'Receive-Job behaves suspiciously: Cannot receive 5 results in 5 minutes.' } | Should Not Th
row }
>> it "hgi" { $true | Should be $true }
>> }
Describing abc
[-] def 32ms
Expected: the expression not to throw an exception. Message was {Receive-Job behaves suspiciously: Cannot receive 5 results in 5 minutes.}
from line:2 char:20
+ ... t "def" { { throw 'Receive-Job behaves suspiciously: Cannot receive 5 ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
at line: 2 in
2: it "def" { { throw 'Receive-Job behaves suspiciously: Cannot receive 5 results in 5 minutes.' } | Should Not Throw }
[+] hgi 17ms
The first one is more readable. And in this case we throw the exception on purpose to fail the test case, so "{ throw "qwerty" } | Should Not Throw" feels like doing extra work for the same purpose. Maybe we can just keep it as is?
There was a problem hiding this comment.
Yes, we should keep. (This exception should raise never. So a performance should not be taken into account.)
The idea was that all the results of the tests have to be caught by Should that Pester normally logs the results. But I check "Pester.xml" and see that the original throw is logged well.
Closed.
There was a problem hiding this comment.
System.Management.Automation.Job is string so it would be safer to use quotation marks:
$j | Should BeOfType "System.Management.Automation.Job"Below too.
There was a problem hiding this comment.
What about $result2[0..-1] ?
There was a problem hiding this comment.
This is to compare the whole of $result1 with the first n elements of $result2, where n is the length of $result1. Did the second Receive-Job return all of the data that the first one did?
Then it checks for any missing data by seeing if the element immediately following the first n in $result2 is the next number in sequence.
There was a problem hiding this comment.
Yes, Closed.
Maybe only use -SyncWindow 0.
There was a problem hiding this comment.
It seems we agreed to leave the test with improvments.
There was a problem hiding this comment.
Test added back, with improvements
There was a problem hiding this comment.
Since you don't do "| Should Be" in CheckContent now, the result from this function should be checked.
There was a problem hiding this comment.
Told myself to do it, missed it. Thanks for catching.
There was a problem hiding this comment.
With -SyncWindow 0, Compare-Object compares 2 lists in order. Example:
PS> $a = 1,2,3,4
PS> $b = 2,3,4,1
PS> $c = 1,2,3,4
PS> Compare-Object -ReferenceObject $a -DifferenceObject $b
PS> Compare-Object -ReferenceObject $a -DifferenceObject $b -SyncWindow 0
InputObject SideIndicator
----------- -------------
2 =>
1 <=
3 =>
2 <=
4 =>
3 <=
1 =>
4 <=
PS> Compare-Object -ReferenceObject $a -DifferenceObject $c -SyncWindow 0
PS>
You can add this parameter if you want to compare them in order.
|
@jeffbi Please resolve the merge conflit. |
There was a problem hiding this comment.
Please swap BeforeEach and AfterEach.
There was a problem hiding this comment.
The test don't use $j so we should move it in separate Context block without creating a job in BeforeEach.
There was a problem hiding this comment.
I suggest using the excellent function ShouldBeErrorId
(I have already prepared new PR #3161 to move existing tests to this pattern)
The new look of this test:
{ Get-Job $j -ErrorAction Stop } | ShouldBeErrorId "JobWithSpecifiedNameNotFound,Microsoft.PowerShell.Commands.GetJobCommand" Only insert:
Import-Module $PSScriptRoot\..\..\Common\Test.Helpers.psm1There was a problem hiding this comment.
Nice! Changed to use ShouldBeErrorId
More changes per code review Fix merge conflict
|
@jeffbi Thanks for the great work! We are still far from a full set of job tests. What are your plans - stop here and merge or add new tests? |
|
@jeffbi @iSazonov Thank you both for the great work and thoroughness! I will merge this one once Travis CI passes. If you are ready to add more new tests, feel free to open another PR. BTW, @iSazonov, do you mind clicking "Review Changes" on the right top corner of the "Files Changed" page and select "Approve"? Thanks! |
No description provided.