Skip to content

Fix ConvertTo-Json test #8645

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.

Sign up for GitHub

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

Closed
xtqqczze wants to merge 4 commits into from
Closed

Fix ConvertTo-Json test #8645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b8453b9
Wait-UntilTrue should not timeout
xtqqczze Jan 14, 2019
232b48b
Revert "Remove one unneeded verbose output from 'ConvertTo-Json' (#74…
xtqqczze Jan 14, 2019
bcfe3d0
[Feature] Empty commit for CI
xtqqczze Jan 14, 2019
d773651
[Feature] Remove blank line to fix CodeFactor issue
xtqqczze Jan 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ private object ProcessValue(object obj, int depth)
else
{
TypeInfo t = obj.GetType().GetTypeInfo();
WriteVerbose(StringUtil.Format(UtilityCommonStrings.ConvertToJsonProcessValueVerboseMessage, t.Name, depth));
Copy link
Contributor

@PaulHigin PaulHigin Jan 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like the right fix to me. The verbose message was removed for a reason. If a test depended on the verbose message, then it (the test) should be removed or re-written.

Copy link
Contributor Author

@xtqqczze xtqqczze Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the test could be rewritten not to depend on the verbose output. This could be part of WIP #8593 . This PR fixes the test before further changes.

Copy link
Contributor

@PaulHigin PaulHigin Jan 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just temporarily disable the test until #8593 is complete. It seems wrong to revert a previously approved change without good cause.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Feb 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be best to use an InternalTestHook to get the cmdlet to write the VerboseMessage if the message is not useful outside of the test


if (t.IsPrimitive)
{
Expand Down
3 changes: 3 additions & 0 deletions src/Microsoft.PowerShell.Commands.Utility/resources/UtilityCommonStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,4 +174,7 @@
<data name="InvalidSDDL" xml:space="preserve">
<value>Cannot construct a security descriptor from the given SDDL due to the following error: {0}</value>
</data>
<data name="ConvertToJsonProcessValueVerboseMessage" xml:space="preserve">
<value>Processing object of type [{0}] at depth {1}</value>
</data>
</root>
2 changes: 1 addition & 1 deletion test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ Describe 'ConvertTo-Json' -tags "CI" {
})
$null = $ps.BeginInvoke()
# wait for verbose message from ConvertTo-Json to ensure cmdlet is processing
Wait-UntilTrue { $ps.Streams.Verbose.Count -gt 0 }
Wait-UntilTrue { $ps.Streams.Verbose.Count -gt 0 } | Should -BeTrue
$null = $ps.BeginStop($null, $null)
# wait a bit to ensure state has changed, not using synchronous Stop() to avoid blocking Pester
Start-Sleep -Milliseconds 100
Expand Down