-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Improve test coverage for CDXML cmdlet infrastructure #4537
Improve test coverage for CDXML cmdlet infrastructure #4537
Conversation
Includes MOF files to create and delete class and instances which are used by the tests
updated create mof file to support associations as well as complex objects updated delete mof file to remove all classes added an enum to cdxml (not yet used) updated psd1 to export new cmdlets for set and new
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.
LGTM with minor comments
Context "Remove-CimTest cmdlet" { | ||
BeforeEach { | ||
Get-CimTest | Remove-CimTest | ||
1..4 | %{ New-CimInstance -namespace root/default -class PSCore_Test1 -property @{ |
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.
Please use ForEach-Object instead of %
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.
This needs to be addressed.
As stated in #3791, our scripts should not have PSScriptAnalyzer warnings and errors.
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.
fixed
field2 = 0 | ||
} | ||
New-CimTest @instanceArgs | ||
$result = Get-CimInstance -namespace root/default -class PSCore_Test1 | ?{$_.id -eq "telephone"} |
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.
Use Where-Object instead of ?
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.
Same here.
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.
fixed
$result.field2 | should be 33 | ||
} | ||
} | ||
|
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.
Extra line.
HelpInfoUri = "https://go.microsoft.com/fwlink/?linkid=390832" | ||
} | ||
|
||
|
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.
Remove extra lines.
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.
fixed
AliasesToExport = @() | ||
CmdletsToExport = @() | ||
FunctionsToExport = @( 'Get-CimTest', 'Remove-CimTest', 'New-CimTest', 'Set-CimTest' ) | ||
HelpInfoUri = "https://go.microsoft.com/fwlink/?linkid=390832" |
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.
Do we need this? This probably points to something else.
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.
probably not - removed
Describe "Cdxml cmdlets are supported" -Tag CI,RequireAdminOnWindows { | ||
BeforeAll { | ||
$skipNotWindows = ! $IsWindows | ||
if ( $skipNotWindows ) { |
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.
Why not use a direct $IsWindows test versus a variable?
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.
I thought it was clearer here -skip
(I've seen other tests do exactly the opposite of what was wanted)
} | ||
$result = MofComp.exe $deleteMof | ||
if ( $LASTEXITCODE -ne 0 ) { | ||
$script:ItSkipOrPending = @{ Pending = $true } |
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.
Why isn't this an outright failure? It appears to be masking a bug.
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.
I thought about this, and while it might be a bug in the mof file or a problem with mofcomp, marking a test as pending doesn't get us off the hook - all pending tests must be resolved before we ship (and I do track our pending tests). However, I didn't want to invalidate the build because I couldn't even get to the actual test. I thought it was better to mark the test as pending rather than fail.
$job = Get-CimTest -id 3 | Remove-CimTest -asjob | ||
$result = $null | ||
$i = 0 | ||
# wait up to 10 seconds, then the test will fail |
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.
Why isn't Wait-Job -Timeout sufficient?
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.
ahem, yes, that's much better
-fixed-
If there is a problem with mofcomp execution, the tests will fail rather than being marked as pending (feedback from DanTra) use cmdlet names rather than aliases for foreach-object and where-object Script analyzer report is clean Also remove unneeded empty lines and helpinfouri from psd1 file
@dantraMSFT, I believe this is ok now |
@dantraMSFT Can you please take another look at this PR? |
address issue #4159
Create custom cim classes (via MOF) and cdxml cmdlets against the new cim class
Create tests for CRUD operations for CDXML cmdlets
Coverage for Microsoft.PowerShell.Cmdletization namespace is nearly 50%