GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Refactor ConvertTo-Json cmdlet tests by xtqqczze · Pull Request #8593 · PowerShell/PowerShell

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Jan 5, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • User-facing Documentation needed
      • Issue filed - Issue link:
  • Testing - New and feature
    • Not Applicable or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
xtqqczze added 6 commits January 4, 2019 23:14
@xtqqczze
Use ForEach operator instead of Foreach-Object
d747776
Avoid unecessary use of the pipeline.
@xtqqczze
Wait for running state rather than a verbose message
d496b55
ConvertTo-Json never writes to the verbose stream.
@xtqqczze
Wait for invocation to stop.
bbf3dcb
Use a timeout and interval to execute quicker.
@xtqqczze
[Feature] Refactor and reformat
cf700cc
@xtqqczze
Write to the verbose stream and wait for that instead
57c8f0d
@xtqqczze
[Feature] Add comment
a302c2c
@iSazonov
Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT) and Mark Kraus (markekraus) January 5, 2019 15:35
@iSazonov Ilya (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 Ilya (iSazonov) changed the title [Feature] Refactor ConvertTo-Json cmdlet tests Refactor ConvertTo-Json cmdlet tests Jan 5, 2019
@iSazonov

Ilya (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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov Ilya (iSazonov) assigned Aditya Patwardhan (adityapatwardhan) Jan 9, 2019
SteveL-MSFT
Steve Lee (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 Steve Lee (SteveL-MSFT) Jan 10, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@xtqqczze xtqqczze Jan 11, 2019

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jan 11, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@xtqqczze xtqqczze Apr 27, 2019

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Issue opened: #9489

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
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 Steve Lee (SteveL-MSFT) Jan 10, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@xtqqczze
Wait to ensure ConvertTo-Json has started processing
5d703be
SteveL-MSFT
Steve Lee (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 Steve Lee (SteveL-MSFT) Jan 10, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
xtqqczze added 3 commits January 11, 2019 03:47
@xtqqczze
Move ForEach out of script
c3ec12c
@xtqqczze
Use default timeout interval for Wait-UntilTrue and adjust sleep time
dbb33af
@xtqqczze
[Feature] Empty commit for CI
a3ce062
This was referenced Jan 15, 2019
Fix ConvertTo-Json test #8645
Closed
Refactor ConvertTo-Json to expose JsonObject.ConvertToJson as a public API #8682
Merged
@daxian-dbw
Dongbo Wang (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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@stale stale Bot added the Stale label Feb 20, 2019
@iSazonov

Ilya (iSazonov) commented Feb 21, 2019

Copy link
Copy Markdown
Collaborator

xtqqczze Could you please address comments?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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 Steve Lee (SteveL-MSFT) SteveL-MSFT requested changes
@markekraus Mark Kraus (markekraus) Awaiting requested review from markekraus
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@adityapatwardhan Aditya Patwardhan (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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@xtqqczze @iSazonov @SteveL-MSFT @adityapatwardhan
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.