Don't crash on CIM classes with no superclass #1046
Conversation
|
Bergmeister's PR #985 addresses same issue, I am agnostic about which one to take. Would be nice to fix though... |
|
My PR is still WIP because I also wanted to remove the empty catch block, which then caused test failures on Linux, therefore, I think it is OK to take your PR first with only the minimal changes for fixing the NullReferenceException. But I think we should add a test case though for the bug that this PR fixes before merging the PR despite the fix being trivial. |
|
The error in the AppVeyor WMF4 build is actually due to the recent breaking change with TLS 1.2 here: dotnet/announcements#77 |
|
OK, added a test case. |
|
@JamesWTruher @bergmeister can we merge this now? |
|
Sorry, I will review this the next days (I want to test that that the added test would actually fail) but it looks OK otherwise. |
There was a problem hiding this comment.
@edyoung There are 2 problems with the added test:
- The test does not fail when using a version of PSSA without this fix. This is due to PSSA throwing a non-terminating error. A solution that to assert against this is the following pattern:
$exceptionThrown = $false
try {
$violations = Invoke-ScriptAnalyzer -Path $noParentClassFilepath -IncludeRule $ruleName -ErrorAction Stop
$violations.Count | Should -Be 0
}
catch {
$exceptionThrown = $true
}
$exceptionThrown | Should -Be $false- The test uses checked in files for testing, which is not good as it makes the test code less readable and will add to the (already existing) clutter of the code base. Please use Pester's built in functionality for file operation isolation that also handles the cleanup of it: https://github.com/pester/Pester/wiki/TestDrive
|
Added -ErrorAction Stop - the additional try/catch/should check isn't needed as far as I can see - Pester fails the test because a terminating error occurs. Changed the test data to be inline, but TBH I don't really think it's an improvement. |
|
@JamesWTruher wondered if [path.io]::combine("testdrive:", ...) would do the right thing on linux. I tried it, and I believe it does, but changed to Join-Path anyway. Also, turns out this rule basically doesn';t work on linux anyway because DscClassCache.Initialize() throws and the rest of the rule doesn't do anything. So I marked the test to be skipped on linux and mac like the others in this file. |
|
@JamesWTruher if you're ok with this please go ahead & merge (FYI the main reason I care about this is that PowerShell Gallery script scanning process hits this exception fairly frequently and I want to cut the noise). |
|
@edyoung Since you have addressed the comment by @JamesWTruher and me about adding a test, I think it is OK to merge it in since the implementation has already been approved. Should there be any more tweaks to be made to the test, we can add it after this PR in order to speedup things for you. |
* Don't crash on CIM classes with no superclass * ReTrigger CI by using better linq variable name * add test case * incorporate PR feedback * tweak test for linux * use join-path
PR Summary
Fix #982 by checking for null on superclass property.
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets. Please mark anything not applicable to this PRNA.WIP:to the beginning of the title and remove the prefix when the PR is ready