GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Refactor ConvertTo-Json cmdlet tests by xtqqczze · Pull Request #8593 · PowerShell/PowerShell Conversation
|
|
xtqqczze Thanks for your contribution! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| $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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Issue opened: #9489
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| $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.
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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
xtqqczze Could you please address comments? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.