Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd EscapeHandling parameter in ConvertTo-Json cmdlet #7775
Conversation
iSazonov
requested review from
dantraMSFT,
JamesWTruher and
PaulHigin
as
code owners
Sep 13, 2018
iSazonov
added
the
Documentation Needed
label
Sep 13, 2018
iSazonov
self-assigned this
Sep 13, 2018
iSazonov
requested a review
from
SteveL-MSFT
Sep 13, 2018
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
PlagueHO
commented
Sep 13, 2018
|
Great solution - thanks @iSazonov - really appreciate it. |
iSazonov
force-pushed the
iSazonov:convertto-json-escapehandling
branch
2 times, most recently
Sep 13, 2018
This comment has been minimized.
This comment has been minimized.
|
@TravisEz13 Should CI Travis be removed? |
This comment has been minimized.
This comment has been minimized.
|
@iSazonov Yes, it is now replaced with the |
SteveL-MSFT
requested changes
Sep 14, 2018
| } | ||
| $expected1 = "{$newline ""abc"": ""'def'""$newline}" | ||
| $expected2 = "{$newline ""abc"": ""\u0027def\u0027""$newline}" | ||
| @{ 'abc' = "'def'" } | ConvertTo-Json | Should -BeExactly $expected1 |
This comment has been minimized.
This comment has been minimized.
SteveL-MSFT
Sep 14, 2018
Member
Turn this into -TestCases? Benefit is if first assert fails, the whole thing doesn't fail.
iSazonov
added some commits
Sep 13, 2018
iSazonov
force-pushed the
iSazonov:convertto-json-escapehandling
branch
Sep 17, 2018
SteveL-MSFT
requested changes
Sep 17, 2018
| # wait for verbose message from ConvertTo-Json to ensure cmdlet is processing | ||
| Wait-UntilTrue { $ps.Streams.Verbose.Count -gt 0 } | ||
| # wait that ConvertTo-Json cmdlet is processing | ||
| Start-Sleep -Milliseconds 300 |
This comment has been minimized.
This comment has been minimized.
SteveL-MSFT
Sep 17, 2018
Member
Is there a reason for this change from using the verbose channel to know that the cmdlet has started to a sleep? On a particularly slow container/VM, it might take longer than 300ms.
This comment has been minimized.
This comment has been minimized.
iSazonov
Sep 18, 2018
•
Author
Collaborator
We removed the verbose message in #7487 and currently the test take 10 seconds until timeout.
We could consider adding debug message instead of the verbose message.
This comment has been minimized.
This comment has been minimized.
SteveL-MSFT
Sep 18, 2018
Member
@iSazonov ok, got it. Seems like the right thing to do would be to have some test hook so that we can make this predictable as anything with small timeouts are more likely to fail in CI especially when it's under load.
This comment has been minimized.
This comment has been minimized.
iSazonov
Sep 19, 2018
Author
Collaborator
I added verbose in BeginProcessing(). (Previously it was in ProcessRecord())
SteveL-MSFT
requested changes
Sep 19, 2018
| # wait a bit to ensure state has changed, not using synchronous Stop() to avoid blocking Pester | ||
| Start-Sleep -Milliseconds 100 | ||
| Wait-UntilTrue { $ps.Streams.Verbose.Count -gt 0 } -IntervalInMilliseconds 50 | ||
| $null = $ps.Stop() |
This comment has been minimized.
This comment has been minimized.
SteveL-MSFT
Sep 19, 2018
Member
Per comment on line 33, Stop() is synchronous and in the chance it blocks, Pester is blocked. Perhaps we can change the sleep on line 34 to Wait-UntilTrue where State changes?
This comment has been minimized.
This comment has been minimized.
iSazonov
Sep 19, 2018
Author
Collaborator
I think we could discuss this in #7819 and perhaps remove the tests at all.
This comment has been minimized.
This comment has been minimized.
SteveL-MSFT
Sep 19, 2018
Member
If you can revert changes to this test in this PR, I think we can move forward with this PR and resolve this test in the other PR.
This comment has been minimized.
This comment has been minimized.
iSazonov
force-pushed the
iSazonov:convertto-json-escapehandling
branch
to
7ef9f10
Sep 20, 2018
iSazonov
requested review from
BrucePay and
daxian-dbw
as
code owners
Sep 20, 2018
iSazonov
force-pushed the
iSazonov:convertto-json-escapehandling
branch
to
7ef9f10
Sep 20, 2018
This comment has been minimized.
This comment has been minimized.
|
Reopen to restart PowerShell-CI-Linux. |
iSazonov
closed this
Sep 20, 2018
iSazonov
reopened this
Sep 20, 2018
iSazonov
referenced this pull request
Sep 20, 2018
Closed
New EscapeHandling parameter in ConvertTo-Json cmdlet #2941
SteveL-MSFT
reviewed
Sep 21, 2018
| @@ -22,7 +26,7 @@ Describe 'ConvertTo-Json' -tags "CI" { | |||
| $null = $ps.AddScript({ | |||
| $obj = [PSCustomObject]@{P1 = ''; P2 = ''; P3 = ''; P4 = ''; P5 = ''; P6 = ''} | |||
| $obj.P1 = $obj.P2 = $obj.P3 = $obj.P4 = $obj.P5 = $obj.P6 = $obj | |||
| 1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10 -Verbose | |||
| 1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10 | |||
This comment has been minimized.
This comment has been minimized.
SteveL-MSFT
Sep 21, 2018
Member
Surprised this test passed if you remove -verbose as line 35 is waiting on verbose output?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
SteveL-MSFT
Sep 24, 2018
Member
You are removing -Verbose because another PR removed the verbose message (which you added back with the test hook in the other PR), right? If so, I suppose this is ok, but it seems this change doesn't have any impact to this test so wondering why have this change in this PR at all?
This comment has been minimized.
This comment has been minimized.
SteveL-MSFT
requested changes
Sep 24, 2018
| @@ -22,7 +26,7 @@ Describe 'ConvertTo-Json' -tags "CI" { | |||
| $null = $ps.AddScript({ | |||
| $obj = [PSCustomObject]@{P1 = ''; P2 = ''; P3 = ''; P4 = ''; P5 = ''; P6 = ''} | |||
| $obj.P1 = $obj.P2 = $obj.P3 = $obj.P4 = $obj.P5 = $obj.P6 = $obj | |||
| 1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10 -Verbose | |||
| 1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10 | |||
This comment has been minimized.
This comment has been minimized.
SteveL-MSFT
Sep 24, 2018
Member
You are removing -Verbose because another PR removed the verbose message (which you added back with the test hook in the other PR), right? If so, I suppose this is ok, but it seems this change doesn't have any impact to this test so wondering why have this change in this PR at all?
SteveL-MSFT
approved these changes
Sep 25, 2018
This comment has been minimized.
This comment has been minimized.
|
@daxian-dbw @dantraMSFT Could you please do (fast) review before we merge? |
TravisEz13
approved these changes
Oct 1, 2018
markekraus
approved these changes
Oct 2, 2018
|
LGTM |
iSazonov commentedSep 13, 2018
•
edited
PR Summary
Fix #7693.
Add EscapeHandling parameter in ConvertTo-Json cmdlet to allow escape the HTML (<, >, &, ', ") and control characters (e.g. newline).
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