Fix multi-result behavior across all versions of PowerShell (fixes CI UT's on all platforms)#199
Merged
HowardWolosky merged 5 commits intomicrosoft:masterfrom May 30, 2020
Conversation
…lved Tests are failing remotely on Linux but not locally. Disabling for now until this can be investigated and resolved, so that we can still depend at least on the Windows CI build in the interim. Recent run with Linux UT failures: https://dev.azure.com/ms/PowerShellForGitHub/_build/results?buildId=83887&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb
HowardWolosky
added
the
build
Contributor
Author
|
Need to see if the issue is limited to just Linux, or if mac UT's are failing similarly. So, we'll run once with just Linux disabled. If mac has similar failures, then we'll just disable UT's in both for the time being and track figuring out what's going on via #198. |
Contributor
Author
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
HowardWolosky
changed the title
Contributor
Author
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Tests were failing on Mac and Linux, but not Windows (recent test run). That's because Windows CI was running against PoSh 5.x while Linux and Mac were running on PoSh 7.x.
There's a slight difference in behavior for how those two treat arrays.
The real root cause for this was the behavior of
Invoke-GHRestMethodMultipleResult. When creating$finalResult, it was always blindly adding the result to the existing array:PowerShellForGitHub/GitHubCore.ps1
Line 648 in 587e204
...PowerShellForGitHub/GitHubCore.ps1
Line 670 in 587e204
Oddly enough, this created a difference in behavior between PowerShell versions when making the result an array on the caller side. Now I ensure that I don't add anything to
$finalResultunless there's actually a value. With that change, we can now be sure that when we grab the result as an array, it'll be appropriately empty or populated (and not populated with a single$nullentry, thus makingCount1, erroneously).I removed the attempt to force the results to be an array, because this is pointless. PowerShell will always unwrap an array of 0 or 1 in a return result. If you want to ensure that a result is always an array, you have to wrap the result in an object or you have to do wrap the result in an array on the caller side.
PowerShellForGitHub/GitHubCore.ps1
Lines 684 to 685 in 587e204
I also normalized some naming in all of the tests, so that when we're getting back a singular result (by querying for a specific item) that we use a singular variable name, and a plural variable name otherwise.
With this change, we should now be passing CI on all OS platforms and across PowerShell 4+.
Resolves #198