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

Improve test coverage for CDXML cmdlet infrastructure #4537

Merged
merged 4 commits into from
Aug 16, 2017

Conversation

JamesWTruher
Copy link
Member

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%

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
Copy link
Member

@adityapatwardhan adityapatwardhan 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 minor comments

Context "Remove-CimTest cmdlet" {
BeforeEach {
Get-CimTest | Remove-CimTest
1..4 | %{ New-CimInstance -namespace root/default -class PSCore_Test1 -property @{
Copy link
Member

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 %

Copy link
Member

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.

Copy link
Member Author

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"}
Copy link
Member

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Member Author

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
}
}

Copy link
Member

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"
}


Copy link
Member

Choose a reason for hiding this comment

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

Remove extra lines.

Copy link
Member Author

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"
Copy link
Member

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.

Copy link
Member Author

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 ) {
Copy link
Contributor

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?

Copy link
Member Author

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 }
Copy link
Contributor

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.

Copy link
Member Author

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
Copy link
Contributor

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?

Copy link
Member Author

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
@JamesWTruher
Copy link
Member Author

@dantraMSFT, I believe this is ok now

@daxian-dbw
Copy link
Member

@dantraMSFT Can you please take another look at this PR?

@daxian-dbw daxian-dbw merged commit 0d1e191 into PowerShell:master Aug 16, 2017
@JamesWTruher JamesWTruher deleted the jameswtruher/cdxmltest001 branch May 11, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants