Skip to content

Fixing progress for Get-ComputerInfo#9236

Merged
adityapatwardhan merged 2 commits intoPowerShell:masterfrom
powercode:ComputerInfo
Apr 4, 2019
Merged

Fixing progress for Get-ComputerInfo#9236
adityapatwardhan merged 2 commits intoPowerShell:masterfrom
powercode:ComputerInfo

Conversation

@powercode
Copy link
Copy Markdown
Collaborator

@powercode powercode commented Mar 27, 2019 •
edited
Loading

PR Summary

The function that handled progress for Get-ComputerInfo always set the RecordType to Completed.

PR Context

Progress for the cmdlet seemed to disappear.

PR Checklist

@PoshChan-Staging

This comment has been minimized.

Sign in to view
@powercode powercode force-pushed the ComputerInfo branch 2 times, most recently from 858b021 to c033494 Compare March 27, 2019 14:03
SteveL-MSFT
SteveL-MSFT approved these changes Mar 27, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

Minor style nit

@SteveL-MSFT
Copy link
Copy Markdown
Member

SteveL-MSFT commented Mar 27, 2019

@powercode I believe you're the first person to get to experience @PoshChan's new capability. I'm curious of your thoughts if it was helpful and if you have any suggestions to improve it.

@powercode
Copy link
Copy Markdown
Collaborator Author

powercode commented Mar 28, 2019 •
edited
Loading

@SteveL-MSFT I loved the easy access to the test failures! Didn't think much about what should be improved as I just saw it as a huge improvement!

I was very pleasantly surprised!

@adityapatwardhan
Copy link
Copy Markdown
Member

adityapatwardhan commented Mar 29, 2019 •
edited
Loading

@powercode I was wondering whether a test like the following should be added:

PowerShell/test/powershell/engine/Help/HelpSystem.Tests.ps1

Lines 99 to 110 in 665cd26

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

@powercode
Copy link
Copy Markdown
Collaborator Author

powercode commented Apr 1, 2019

@adityapatwardhan You're welcome to push it!

@adityapatwardhan
Copy link
Copy Markdown
Member

adityapatwardhan commented Apr 1, 2019

@powercode Found a bug while adding test. The value of status was updated before the null check, hence we never set the recordtype as completed. Fixed the bug and added the test. Please have a look.

@adityapatwardhan adityapatwardhan merged commit 4ec36a0 into PowerShell:master Apr 4, 2019
@adityapatwardhan adityapatwardhan added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 4, 2019
@adityapatwardhan adityapatwardhan added this to the 6.3.0-preview.1 milestone Apr 4, 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 approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

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

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.0.0-preview.1

Development

Successfully merging this pull request may close these issues.

4 participants

@powercode @PoshChan-Staging @SteveL-MSFT @adityapatwardhan