GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Improve test coverage for CDXML cmdlet infrastructure by JamesWTruher · Pull Request #4537 · PowerShell/PowerShell

Conversation

James Truher (JamesWTruher) commented Aug 9, 2017

Copy link

Copy Markdown

Collaborator

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%

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

James Truher (JamesWTruher) added 3 commits

August 9, 2017 13:46


          Simple CDXML tests for Get and Remove

f624d07

Includes MOF files to create and delete class and instances which are used by the tests


          Additional tests - All CRUD operations are now covered

6f09fe6

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


          Add tests for exceptions and support whatif

5279b03

James Truher (JamesWTruher) assigned Dongbo Wang (daxian-dbw)

Aug 9, 2017

James Truher (JamesWTruher) requested review from Aditya Patwardhan (adityapatwardhan) and Dan Travison (dantraMSFT)

August 9, 2017 20:59

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label

Aug 9, 2017

Aditya Patwardhan (adityapatwardhan) approved these changes

Aug 11, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/powershell/engine/Cdxml/Cdxml.Tests.ps1 Outdated

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

Aditya Patwardhan (adityapatwardhan) Aug 11, 2017

Copy link

Copy Markdown

Member

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 %

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Aug 12, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

James Truher (JamesWTruher) Aug 14, 2017

Copy link

Copy Markdown

Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/powershell/engine/Cdxml/Cdxml.Tests.ps1 Outdated

		field2 = 0
		}
		New-CimTest @instanceArgs
		$result = Get-CimInstance -namespace root/default -class PSCore_Test1 \| ?{$_.id -eq "telephone"}

Aditya Patwardhan (adityapatwardhan) Aug 11, 2017

Copy link

Copy Markdown

Member

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 ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) Aug 12, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

James Truher (JamesWTruher) Aug 14, 2017

Copy link

Copy Markdown

Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/powershell/engine/Cdxml/Cdxml.Tests.ps1

		$result.field2 \| should be 33
		}
		}

Aditya Patwardhan (adityapatwardhan) Aug 11, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/powershell/engine/Cdxml/assets/CimTest/CdxmlTest.psd1 Outdated

		HelpInfoUri = "https://go.microsoft.com/fwlink/?linkid=390832"
		}

Aditya Patwardhan (adityapatwardhan) Aug 11, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

James Truher (JamesWTruher) Aug 14, 2017

Copy link

Copy Markdown

Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/powershell/engine/Cdxml/assets/CimTest/CdxmlTest.psd1 Outdated

		AliasesToExport = @()
		CmdletsToExport = @()
		FunctionsToExport = @( 'Get-CimTest', 'Remove-CimTest', 'New-CimTest', 'Set-CimTest' )
		HelpInfoUri = "https://go.microsoft.com/fwlink/?linkid=390832"

Aditya Patwardhan (adityapatwardhan) Aug 11, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

James Truher (JamesWTruher) Aug 14, 2017

Copy link

Copy Markdown

Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) reviewed

Aug 14, 2017

View reviewed changes

Comment thread

test/powershell/engine/Cdxml/Cdxml.Tests.ps1

		Describe "Cdxml cmdlets are supported" -Tag CI,RequireAdminOnWindows {
		BeforeAll {
		$skipNotWindows = ! $IsWindows
		if ( $skipNotWindows ) {

Dan Travison (dantraMSFT) Aug 14, 2017

Copy link

Copy Markdown

Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

James Truher (JamesWTruher) Aug 14, 2017

Copy link

Copy Markdown

Collaborator Author

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) reviewed

Aug 14, 2017

View reviewed changes

Comment thread

test/powershell/engine/Cdxml/Cdxml.Tests.ps1 Outdated

		}
		$result = MofComp.exe $deleteMof
		if ( $LASTEXITCODE -ne 0 ) {
		$script:ItSkipOrPending = @{ Pending = $true }

Dan Travison (dantraMSFT) Aug 14, 2017

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

James Truher (JamesWTruher) Aug 14, 2017

Copy link

Copy Markdown

Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) reviewed

Aug 14, 2017

View reviewed changes

Comment thread

test/powershell/engine/Cdxml/Cdxml.Tests.ps1

		$job = Get-CimTest -id 3 \| Remove-CimTest -asjob
		$result = $null
		$i = 0
		# wait up to 10 seconds, then the test will fail

Dan Travison (dantraMSFT) Aug 14, 2017

Copy link

Copy Markdown

Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

James Truher (JamesWTruher) Aug 14, 2017

Copy link

Copy Markdown

Collaborator Author

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-

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Add hardening to test code to handle missing or misbehaving MOFCOMP

0af83e2

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

James Truher (JamesWTruher) commented Aug 15, 2017

Copy link

Copy Markdown

Collaborator Author

Dan Travison (@dantraMSFT), I believe this is ok now

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) commented Aug 16, 2017

Copy link

Copy Markdown

Member

Dan Travison (@dantraMSFT) Can you please take another look at this PR?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dan Travison (dantraMSFT) approved these changes

Aug 16, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) merged commit 0d1e191 into PowerShell:master

Aug 16, 2017

James Truher (JamesWTruher) deleted the jameswtruher/cdxmltest001 branch

May 11, 2022 16:35

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Improve test coverage for CDXML cmdlet infrastructure (PowerShell#4537)

17b44ce

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%

Reviewers

Dongbo Wang (daxian-dbw)

daxian-dbw left review comments

Aditya Patwardhan (adityapatwardhan)

adityapatwardhan approved these changes

+1 more reviewer

Dan Travison (dantraMSFT)

dantraMSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

James Truher (JamesWTruher) commented Aug 9, 2017

Uh oh!

Aditya Patwardhan (adityapatwardhan) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

James Truher (JamesWTruher) commented Aug 15, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Aug 16, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Dongbo Wang (daxian-dbw) Aug 12, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.