Fix wrong output for New-Service in variable assignment and -OutVariable
#10444
Conversation
|
Disclaimer: Novice in c# and really don't know the impact of the change, the code change was by referring similar usage in the same file. |
|
CI failure seems to be something different. |
src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs
Outdated
Show resolved
Hide resolved
|
@kvprasoon you might need to look at how the test is constructed. If the test is expecting the object to be disposed, it might explain why the tests are erroring out. |
|
@kvprasoon Of the three errors, I think the last failure may be expected based on how your test is written. When you create a new service with a For the first two errors, they indicate that the display name is null when it is not. I wonder if there is a timing issue (i.e. I wonder if the service display name property value does not show up immediately after creating the service). You should try creating some services outside of Pester, show their properties when the new service objects come back, then invoke Get-Service to get the services, and see if the properties are set the way you expect as well. Plus, experiment with timing of those a bit. |
|
@KirkMunro @vexx32 , My earlier comment was for the previous commit, for latest one (not current), the output doesn't contain |
|
|
||
| if ($displayname) { | ||
| $service.displayname | Should -Be $displayname | ||
| } |
There was a problem hiding this comment.
Rather than make this check conditional such that it only runs some of the time, how about just making it check for the appropriate value and having it run all the time?
$service.displayname | Should -Be $(if ($displayname) {$displayname} else {$name})There was a problem hiding this comment.
Like the idea, but not really a fan of that particular syntax.
Maybe:
$testvalue = if ($DisplayName) { $displayname } else { $name }
$service.Displayname | should -BeExactly $testvalueThere was a problem hiding this comment.
There is absolutely nothing wrong with using a subexpression here.
There was a problem hiding this comment.
Didn't say it was wrong, just overly long and complicated for a single line. 🙂
There was a problem hiding this comment.
I couldn't disagree more, and the suggestion that the proposed line was "overly long and complicated" is misinformation.
With a condition as simple as this one, it is far better to keep the code focused on the tests being executed by having the condition inline than have more code to look over and more variables to think about.
As an added bonus, once we get a null-coalescing operator, inline code such as what was proposed becomes much, much simpler.
There was a problem hiding this comment.
Rather than make this check conditional such that it only runs some of the time, how about just making it check for the appropriate value and having it run all the time?
$service.displayname | Should -Be $(if ($displayname) {$displayname} else {$name})
This will implicitly add the check, if DisplayName is null, then ServiceName becomes DisplayName
New-Service in variable assignment and -OutVariable
|
🎉 Handy links: |
New-Service cmdlet's output doesn't have all properties when assigned to a variable or using -OutVariable.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.