Skip to content

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jan 5, 2019

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
Copy link
Collaborator

iSazonov commented Jan 5, 2019

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

$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
Copy link
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.

Copy link
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.

Copy link
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.

Copy link
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

$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
Copy link
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

Wait-UntilTrue { $ps.Streams.Verbose.Count -gt 0 } -TimeoutInMilliseconds 1000 -IntervalInMilliseconds 10

# Wait to ensure ConvertTo-Json has started processing.
Start-Sleep -Milliseconds 200
Copy link
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.

@stale
Copy link

stale bot commented Feb 20, 2019

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
Copy link
Collaborator

@xtqqczze Could you please address comments?

@stale stale bot removed the Stale label Feb 21, 2019
@stale
Copy link

stale bot commented Mar 23, 2019

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
Copy link

stale bot commented Apr 2, 2019

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

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.

4 participants