Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix ConvertTo-Json test#8645

Closed
xtqqczze wants to merge 4 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
xtqqczze:fix-ConvertTo-Json.Testsxtqqczze/PowerShell-PowerShell:fix-ConvertTo-Json.TestsCopy head branch name to clipboard
Closed

Fix ConvertTo-Json test#8645
xtqqczze wants to merge 4 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
xtqqczze:fix-ConvertTo-Json.Testsxtqqczze/PowerShell-PowerShell:fix-ConvertTo-Json.TestsCopy head branch name to clipboard

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Jan 14, 2019

Copy link
Copy Markdown
Contributor

Fix ConvertTo-Json test

PR Summary

PR Context

8374a5c removed verbose output from ConvertTo-Json which broke a test.

PR Checklist

@iSazonov iSazonov added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label Jan 15, 2019
else
{
TypeInfo t = obj.GetType().GetTypeInfo();
WriteVerbose(StringUtil.Format(UtilityCommonStrings.ConvertToJsonProcessValueVerboseMessage, t.Name, depth));

Copy link
Copy Markdown
Contributor

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.

@xtqqczze xtqqczze Jan 15, 2019

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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
Copy Markdown
Member

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

else
{
TypeInfo t = obj.GetType().GetTypeInfo();
WriteVerbose(StringUtil.Format(UtilityCommonStrings.ConvertToJsonProcessValueVerboseMessage, t.Name, depth));

Copy link
Copy Markdown
Member

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

@stale

stale Bot commented Mar 4, 2019

Copy link
Copy Markdown

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.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale Bot added the Stale label Mar 4, 2019
@stale

stale Bot commented Mar 14, 2019

Copy link
Copy Markdown

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.
Thanks again for your contribution.
Community members are welcome to grab these works.

@stale stale Bot closed this Mar 14, 2019
@xtqqczze

xtqqczze commented Apr 24, 2019

Copy link
Copy Markdown
Contributor Author

Test marked as pending in #9458

@xtqqczze xtqqczze deleted the fix-ConvertTo-Json.Tests branch May 24, 2020 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Test Indicates that a PR should be marked as a test change in the Change Log Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.