Fix casting single element array to generic collection #3170
Conversation
| for ($i = 0; $i -lt $Count; $i++) | ||
| { | ||
| $result[$i] | Should Be $Elements[$i] | ||
| } |
There was a problem hiding this comment.
Can we use Compare-Object and exclude Count at all?
There was a problem hiding this comment.
Yes, the Count key is not necessary, and I updated the tests to avoid it.
Instead of using a loop or Compare-Object, I think $result -join ";" | Should Be ($Elements -join ";") is better here. In case the results are different, I can see the whole set of results and expected values in the log.
|
|
||
| @{ Command = {$result = [System.Collections.ObjectModel.Collection[int]]@(1)}; CollectionType = 'Collection`1'; ElementType = "Int32"; Count = 1; Elements = @(1) } | ||
| @{ Command = {$result = [System.Collections.ObjectModel.Collection[int]]@(1,2)}; CollectionType = 'Collection`1'; ElementType = "Int32"; Count = 2; Elements = @(1,2) } | ||
| @{ Command = {$result = [System.Collections.ObjectModel.Collection[int]]"4"}; CollectionType = 'Collection`1'; ElementType = "Int32"; Count = 1; Elements = @(4) } |
There was a problem hiding this comment.
Perhaps it makes sense to add tests for complex type items (array, hash, psobject) because code use LanguagePrimitives.ConvertTo?
There was a problem hiding this comment.
I added one more set of tests for [List[FileInfo]] cast from string or array of strings. If you think more tests is needed in this area, feel free to open an issue.
There was a problem hiding this comment.
I suppose that's enough.
Closed.
| $result -join ";" | Should Be ($Elements -join ";") | ||
| } | ||
|
|
||
| It "<Command>" -TestCases $testCases2 { |
There was a problem hiding this comment.
Why we split testCases1 and testCases2? It seems the test code is the same.
There was a problem hiding this comment.
Good point :) I thought the FileInfo test needs to be @($result.Name) -join ";", but $result -join ";" should work too.
|
LGTM for tests. @daxian-dbw Please clarify for me: Is the PR related with |
|
@daxian-dbw Thanks for clarify! |
lzybkr
left a comment
lzybkr
left a comment
There was a problem hiding this comment.
I have one comment on the code - it would be good to ensure you know what the regression was from V4 and that we have tests for it - so we ensure this fix (which looks right to me) doesn't regress anything else.
Other than that, just the 2 minor comments on the tests.
| $testCases1 = @( | ||
| @{ Command = {$result = [Collections.Generic.List[int]]@(1)}; CollectionType = 'List`1'; ElementType = "Int32"; Elements = @(1) } | ||
| @{ Command = {$result = [Collections.Generic.List[int]]@(1,2)}; CollectionType = 'List`1'; ElementType = "Int32"; Elements = @(1,2) } | ||
| @{ Command = {$result = [Collections.Generic.List[int]]"4"}; CollectionType = 'List`1'; ElementType = "Int32"; Elements = @(4) } |
There was a problem hiding this comment.
Also test > 1 string converting to List[int]?
| param($Command, $CollectionType, $ElementType, $Elements) | ||
|
|
||
| $result = $null | ||
| . $Command |
There was a problem hiding this comment.
It would be less surprising to me if Command was just a script block and here you wrote:
$result = . $CommandThere was a problem hiding this comment.
The resulted collection will be unraveled if I use $result = . {[Collections.Generic.List[int]]@(1)}.
In order to prevent unraveling, a comma needs to be prefixed like "{,[Collections.Generic.List[int]]@(1)}".
But then the test case title becomes ",[Collections.Generic.List[int]]@(1)" which I thought was more confusing than "$result = [Collections.Generic.List[int]]@(1)" and that's why I choose the current form.
Do you want me to change back to {, [Collections.Generic.List[int]]@(1)}?
There was a problem hiding this comment.
I see - neither is great - I'm fine either way then.
Because you tried what I suggested first - maybe it's worth a comment to save someone else the trouble trying to change it.
There was a problem hiding this comment.
Got it. Will add a comment.
5 participants
Fix #2208