(#66) Adding Tests to ChocoCCM #79
Conversation
|
A few PSSA rules I need to follow in the tests before merging, it seems. Edit: Sorted, though I couldn't make things pretty and follow the rules. A true conundrum! |
Adds test covering public functions within ChocoCCM. Admittedly, currently only ~66% code coverage. This does, however, hit pretty much every _working_ command - most of the missing percentage are argument completers (which I am to improve), `if not session throw` blocks (which I aim to remove), and those commands that aren't working. Exceptions to be improved include: - Set-CCMDeploymentStep, which isn't implemented fully and should probably just be removed for now. - Remove-CCMStaleDeployment, which is currently broken - Import-PDQDeployPackage, because I couldn't find an example of an importable file.
These were not terribly useful now we had the new tests in place. Also removes the non-existent function `Get-CCMDeploymentStepResult` from the PSD1, after improving the "has all the functions available" test (which showed this disparity).
Removes the -All parameter from Get-CCMDeployment, which means this might work. Adjusts the test to be a little better.
Unless we intended to provide regex-matching functionality here (which isn't obvious from the help), this fixes the ability to return multiple computers with a single name (or even random regex query).
| It "Sets the Session Variable" -Skip { | ||
| # TODO: Fix this test. | ||
| InModuleScope ChocoCCM { $script:Session } | Should -Not -BeNullOrEmpty | ||
| } |
There was a problem hiding this comment.
what remains to be done with this one?
| # Creating an already existing group name | ||
| Context "Adding a new group with an already existing name" -Skip { | ||
| # This is apparently fine to do. Hmph. | ||
| # Given the way it's written (e.g. using $Current to update the object?), | ||
| # I can't tell if this is good or bad. It seems to be modelled on Add-CCMGroupMember... sort of. |
There was a problem hiding this comment.
Yeah, I'm with you on that. It is a bit funky, especially the way group lookups and handling is done with this module. It's probably not going to change here though, but this kind of test could be useful later on.
| It "Sets the Session Variable" -Skip { | ||
| # TODO: Fix this test. | ||
| InModuleScope ChocoCCM { $script:Session } | Should -Not -BeNullOrEmpty | ||
| } |
| } | ||
| } | ||
|
|
||
| Context "Disabling a non-existent deployment" -Skip { |
There was a problem hiding this comment.
Guessing this is skipped because we don't throw an error currently? We should leave a comment explaining that here.
| # Why is ID not expanding .items? | ||
| # Potential bug around items vs not items, given the same call in all cases. | ||
| It "Returns the VLC Software Details" -Skip { |
There was a problem hiding this comment.
I think this might perhaps need some more elaboration on what's expected here? I can't make sense of this comment 😅
There was a problem hiding this comment.
Ha! Sorry about that.
Based on my recollection, this slightly frustrated comment was around the different objects being output based on the parameterset - Software and Package expand Result, while ID and all expand Result.Items. My comment was clearly not entirely useful, but I was annoyed at this potential antipattern as I always feel like this mixed handling can result in confusion.
I think it's slightly mitigated by the API returning less detailed objects with the All (default) parameter set, though I may be wrong / misremembering something else.
There was a problem hiding this comment.
Oh, it's one of those.... Maybe just clarify in this comment for now that it's referring to the parameter sets of the command returning differently-shaped output so we know for the future :3
| $script:Protocol = "http" | ||
| } | ||
| Mock Invoke-RestMethod -ModuleName ChocoCCM | ||
| Mock Get-CCMDeployment -ModuleName ChocoCCM { @{Id = $script:IDUnderTest } } |
There was a problem hiding this comment.
I'm not seeing where this var is being set from... should it be being set in this script?
Description Of Changes
This PR adds a few tests to the module.
Motivation and Context
I wanted to refactor something, but without tests to show that my changes didn't actually change the resulting operations (beyond a few fixes), I was unlikely to be able to convince folk to look at it.
Ideally, we will add some integration tests that can work against a running instance, but this is (hopefully) a great start for unit testing that can be run quickly at dev and build time!
Most missed testing is now in
Set-CCMDeploymentStep(which is non-functional but lengthy, and has a bug open around fixing it - #68),Import-PDQDeployPackage(which I didn't have data to test against), theif not $script:session throwblocks, and argument completers.There are a number of currently skipped tests that could do with a chat.
Testing
No changes to operational code were made, except for those that fixed bugs in functions. These should now work!
Operating Systems Testing
Change Types Made
...technically I suppose the changes to fix functions are breaking changes, but in a good way? I'd argue there are no truly breaking changes in here. There is a change to make a name match more exact that may break usage, but does not break what we say should happen. Happy to chat about that.
[ ] Breaking change (fix or feature that could cause existing functionality to change).[ ] Documentation changes.Change Checklist
[ ] Requires a change to the documentation.[ ] Documentation has been updated.Related Issue
Fixes #66