Skip to content
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.

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

Fixing issue with help progress #8788

Merged
merged 6 commits into from
Mar 11, 2019

Conversation

powercode
Copy link
Collaborator

@powercode powercode commented Jan 29, 2019

The Get-Help cmdlet never calls WriteProgress with a RecordType set to Completed, so the progress is left being rendered even after the Get-Help cmdlet has completed.

The PR is adding a write progress call with recordtype set to Completed.

It also cleans up lots of chains to this.Context.HelpSystem by extracting a variable.

PR Summary

PR Context

PR Checklist

@adityapatwardhan
Copy link
Member

@powercode Love the change! Could you add some tests? Something like the following.

Describe "Testing Get-Help Progress" {
 It "Last ProgressRecord should be Completed" {
  $j = Start-Job { Get-Help DoesNotExist }
  $j | Wait-Job
  $j.ChildJobs[0].Progress[-1].RecordType | Should -Be ([System.Management.Automation.ProgressRecordType]::Completed)
}}

Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

LGTM with one style comment.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 7, 2019
@iSazonov
Copy link
Collaborator

iSazonov commented Feb 8, 2019

Reopen to restart all CIs (no response).

@iSazonov iSazonov closed this Feb 8, 2019
@iSazonov iSazonov reopened this Feb 8, 2019
It "Last ProgressRecord should be Completed" {
$j = Start-Job { Get-Help DoesNotExist }
$j | Wait-Job
$j.ChildJobs[0].Progress[-1].RecordType | Should -Be ([System.Management.Automation.ProgressRecordType]::Completed)
Copy link
Member

Choose a reason for hiding this comment

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

Please add a Remove-Job $j

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be in try-finally.

test/powershell/engine/Help/HelpSystem.Tests.ps1 Outdated Show resolved Hide resolved
@adityapatwardhan
Copy link
Member

@powercode Pushed an empty commit to execute Feature tests. The new tests are categorized as Feature.

@adityapatwardhan adityapatwardhan merged commit 0ebbdc1 into PowerShell:master Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants