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

Refactor ConvertTo-Json cmdlet tests#8593

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

Refactor ConvertTo-Json cmdlet tests#8593
xtqqczze wants to merge 10 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
xtqqczze:ConvertTo-Json.Testsxtqqczze/PowerShell-PowerShell:ConvertTo-Json.TestsCopy head branch name to clipboard

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Jan 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

ConvertTo-Json.Tests.ps1

PR Summary

  • Newtonsoft.Json.Linq.Jproperty should be converted to Json properly test
    • Refactor for speed and clarity.
  • StopProcessing should succeed test
    • Avoid 10 second sleep before timing out.
      • Existing test waits for ConvertTo-Json to write to verbose stream, which times out.
    • Avoid 100 millisecond sleep
      • Wait until Stopped state instead.
    • Refactor for speed and clarity.
  • Refactor and reformat:
    • add using namespace statements
    • use single quotes where possible
    • avoid pipeline where possible

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 5, 2019
@iSazonov iSazonov changed the title [Feature] Refactor ConvertTo-Json cmdlet tests Refactor ConvertTo-Json cmdlet tests Jan 5, 2019
@iSazonov

iSazonov commented Jan 5, 2019

Copy link
Copy Markdown
Collaborator

@xtqqczze Thanks for your contribution!
If you want to work on tests I want to ask you to work on new tests. Our test coverage is too small and new tests would bring us more benefits than simple test refactoring. Current coverage is ~60 but we want to reach >80%. There is some opened issues for new tests. Also you can look CodeCover status.

SteveL-MSFT
SteveL-MSFT requested changes Jan 10, 2019
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1 Outdated
$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

@SteveL-MSFT SteveL-MSFT Jan 10, 2019

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.

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.

@xtqqczze xtqqczze Jan 11, 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.

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.

@SteveL-MSFT SteveL-MSFT Jan 11, 2019

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.

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

@xtqqczze xtqqczze Apr 27, 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.

Issue opened: #9489

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

@SteveL-MSFT SteveL-MSFT Jan 10, 2019

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.

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

SteveL-MSFT
SteveL-MSFT reviewed Jan 10, 2019
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1 Outdated
Wait-UntilTrue { $ps.Streams.Verbose.Count -gt 0 } -TimeoutInMilliseconds 1000 -IntervalInMilliseconds 10

# Wait to ensure ConvertTo-Json has started processing.
Start-Sleep -Milliseconds 200

@SteveL-MSFT SteveL-MSFT Jan 10, 2019

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.

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.

This was referenced Jan 15, 2019
Closed
Merged
@daxian-dbw daxian-dbw self-requested a review January 21, 2019 02:11
@stale

stale Bot commented Feb 20, 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 Feb 20, 2019
@iSazonov

iSazonov commented Feb 21, 2019

Copy link
Copy Markdown
Collaborator

@xtqqczze Could you please address comments?

@stale stale Bot removed the Stale label Feb 21, 2019
@stale

stale Bot commented Mar 23, 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 23, 2019
@stale

stale Bot commented Apr 2, 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 Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT requested changes

@markekraus markekraus Awaiting requested review from markekraus

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

Assignees

@adityapatwardhan adityapatwardhan

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xtqqczze @iSazonov @SteveL-MSFT @adityapatwardhan
Morty Proxy This is a proxified and sanitized view of the page, visit original site.