-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Refactor ConvertTo-Json cmdlet tests #8593
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
Conversation
Avoid unecessary use of the pipeline.
ConvertTo-Json never writes to the verbose stream.
Use a timeout and interval to execute quicker.
|
@xtqqczze Thanks for your contribution! |
| $obj = [pscustomobject]@{P1 = ''; P2 = ''; P3 = ''; P4 = ''; P5 = ''; P6 = ''} | ||
| $obj.P1 = $obj.P2 = $obj.P3 = $obj.P4 = $obj.P5 = $obj.P6 = $obj | ||
| (1..100).ForEach{ | ||
| Write-Verbose -Message 'Ready' -Verbose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a race condition here where this message would cause line 43 to think that ConvertTo-Json has started processing, but the BeginStop online 46 could be called after this verbose message but before ConvertTo-Json actually started thus invalidating this test. I believe ConvertTo-Json -Verbose will output some verbose message thus fulfilling the wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConvertTo-Json never writes to the verbose stream, and doesn't write to the success stream until it is done. For example:
(1..5) | ConvertTo-Json -Verbose | ForEach-Object { $_; Pause }
Note there is no verbose output written and that the Pause function is only called once.
In the original code Wait-UntilTrue times out because of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xtqqczze ok, I thought I had added a verbose message previously, but perhaps it got removed. My preference would be to insert a verbose message at start of convertto-json as that guarantees the processing has started and makes this test valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue opened: #9489
| $output = 1 | ConvertTo-Json -AsArray | ||
| $output | Should -BeLike "``[*1*]" | ||
| It 'The result string is packed in an array symbols when AsArray parameter is used.' { | ||
| $output = ConvertTo-Json -InputObject 1 -AsArray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason to change all of the usage of pipeline to -InputObject. Seems like the correct thing is to have some use pipeline and some use -InputObject
| Wait-UntilTrue { $ps.Streams.Verbose.Count -gt 0 } -TimeoutInMilliseconds 1000 -IntervalInMilliseconds 10 | ||
|
|
||
| # Wait to ensure ConvertTo-Json has started processing. | ||
| Start-Sleep -Milliseconds 200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a sleep isn't sufficient since in CI, the container or VM could be slow so again the pipeline is stopped before ConvertTo-Json is actually started. The original code used verbose output that comes out of ConvertTo-Json to detect it has started.
|
This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. |
|
@xtqqczze Could you please address comments? |
|
This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. |
|
This PR has been automatically closed because it is stale. If you wish to continue working on the PR, please first update the PR, then reopen it. |
ConvertTo-Json.Tests.ps1
PR Summary
Newtonsoft.Json.Linq.Jproperty should be converted to Json properlytestStopProcessing should succeedtestPR 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