★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/7775Nouvelle recherche | Portail wanayoo
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 EscapeHandling parameter in ConvertTo-Json cmdlet #7775

Merged
merged 5 commits into from Oct 2, 2018

Conversation

@iSazonov
Copy link
Collaborator

commented Sep 13, 2018 •

PR Summary

Fix #7693.

Add EscapeHandling parameter in ConvertTo-Json cmdlet to allow escape the HTML (<, >, &, ', ") and control characters (e.g. newline).

PR Checklist

@iSazonov iSazonov self-assigned this Sep 13, 2018

@iSazonov iSazonov requested a review from SteveL-MSFT Sep 13, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 13, 2018

@PlagueHO

This comment has been minimized.

Copy link

commented Sep 13, 2018

Great solution - thanks @iSazonov - really appreciate it. 🏆

@iSazonov iSazonov force-pushed the iSazonov:convertto-json-escapehandling branch 2 times, most recently Sep 13, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 13, 2018

@TravisEz13 Should CI Travis be removed?

@TravisEz13

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@iSazonov Yes, it is now replaced with the PowerShell-CI-linux and PowerShell-CI-macos

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1 Outdated
}
$expected1 = "{$newline ""abc"": ""'def'""$newline}"
$expected2 = "{$newline ""abc"": ""\u0027def\u0027""$newline}"
@{ 'abc' = "'def'" } | ConvertTo-Json | Should -BeExactly $expected1

This comment has been minimized.

Copy link
@SteveL-MSFT

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 iSazonov force-pushed the iSazonov:convertto-json-escapehandling branch Sep 17, 2018

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1 Outdated
# 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.

Copy link
@SteveL-MSFT

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.

Copy link
@iSazonov

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.

Copy link
@SteveL-MSFT

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.

Copy link
@iSazonov

iSazonov Sep 19, 2018

Author Collaborator

I added verbose in BeginProcessing(). (Previously it was in ProcessRecord())

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1 Outdated
# 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.

Copy link
@SteveL-MSFT

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.

Copy link
@iSazonov

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.

Copy link
@SteveL-MSFT

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.

Copy link
@iSazonov

iSazonov Sep 20, 2018

Author Collaborator

Reverted in the PR and moved to #7819.

@iSazonov iSazonov force-pushed the iSazonov:convertto-json-escapehandling branch to 7ef9f10 Sep 20, 2018

@iSazonov iSazonov requested review from BrucePay and daxian-dbw as code owners Sep 20, 2018

@iSazonov iSazonov force-pushed the iSazonov:convertto-json-escapehandling branch to 7ef9f10 Sep 20, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 20, 2018

Reopen to restart PowerShell-CI-Linux.

@iSazonov iSazonov closed this Sep 20, 2018

@iSazonov iSazonov reopened this Sep 20, 2018

@iSazonov iSazonov referenced this pull request Sep 20, 2018

Closed

New EscapeHandling parameter in ConvertTo-Json cmdlet #2941

2 of 8 tasks complete
@@ -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.

Copy link
@SteveL-MSFT

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.

Copy link
@iSazonov

iSazonov Sep 23, 2018

Author Collaborator

The waiting take 10 seconds until timeout.

This comment has been minimized.

Copy link
@SteveL-MSFT

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.

Copy link
@iSazonov

iSazonov Sep 25, 2018

Author Collaborator

Reverted the change.

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

Copy link
@SteveL-MSFT

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?

@iSazonov

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 26, 2018

@daxian-dbw @dantraMSFT Could you please do (fast) review before we merge?

@markekraus
Copy link
Collaborator

left a comment

LGTM

@iSazonov iSazonov merged commit d862d40 into PowerShell:master Oct 2, 2018

8 checks passed

CodeFactor 3 issues fixed. 1 issue found.
Details
PowerShell-CI-linux #PR-7775-20180925.01 succeeded
Details
PowerShell-CI-macos #PR-7775-20180925.01 succeeded
Details
PowerShell-CI-spelling #PR-7775-20180925.01 succeeded
Details
PowerShell-CI-windows #PR-7775-20180925.01 succeeded
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details

@iSazonov iSazonov deleted the iSazonov:convertto-json-escapehandling branch Oct 2, 2018

adityapatwardhan pushed a commit to adityapatwardhan/PowerShell that referenced this pull request Apr 9, 2019

Add EscapeHandling parameter in ConvertTo-Json cmdlet (PowerShell#7775)
Add EscapeHandling parameter in ConvertTo-Json cmdlet to allow escape the HTML (<, >, &, ', ") and control characters (e.g. newline).
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.