Skip to content

Investigate UT failures happening on Linux/mac in CI build #198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
HowardWolosky opened this issue May 29, 2020 · 3 comments · Fixed by #199
Closed

Investigate UT failures happening on Linux/mac in CI build #198

HowardWolosky opened this issue May 29, 2020 · 3 comments · Fixed by #199
Labels
build Changes related to the build infrastructure for the project. in progress Work on this issue is already underway. Please don't work start new work on it.

Comments

@HowardWolosky
Copy link
Member

We have recently disabled UT's for Linux and Mac in the CI build because there have been unexpected failures during execution that don't appear to happen when run locally (at least for Linux). I'd like to get those re-enabled for full coverage, but they've been disabled in the short term to ensure that we can get real value out of the CI build since Windows is running cleanly right now.
@HowardWolosky HowardWolosky added up for grabs Anyone in the community is welcome to do this work help wanted Anyone in the community is welcome to do this work build Changes related to the build infrastructure for the project. labels May 29, 2020
@HowardWolosky HowardWolosky mentioned this issue May 29, 2020
Merged
@X-Guardian
Copy link
Contributor

The GitHubAnalytics, Obtaining issues for repository, When initially created, there are no issues test error is caused by the difference with PowerShell 5 vs 7 dealing with the empty array return value from Get-GitHubIssue.

For this code specifiying the URI of a newly created repo (without issues):

$issues = Get-GitHubIssue -Uri $Uri
@($issues).count

PowerShell 5 will return 0, but PowerShell 7 will return 1.

I suggest removing the array sub-expression operator around $issues in the test, and in the GitHubCore Invoke-GHRestMethodMultipleResult function, use the comma operator to create the return array.

@X-Guardian
Copy link
Contributor

If you want to be consistent and run the unit tests on PowerShell 7 for Windows, Linux and Mac, change your powershell tasks in run-unitTests.yaml to pwsh tasks.

@HowardWolosky HowardWolosky removed help wanted Anyone in the community is welcome to do this work up for grabs Anyone in the community is welcome to do this work labels May 29, 2020
@HowardWolosky
Copy link
Member Author

Thanks so much @X-Guardian! It hadn't occurred to me that the difference was due to PoSh 7 vs <7. I've been able to repro the UT failures on my Windows machine with PoSh 7 and I'm now making the fixes. That difference in behavior between array handling in 7 is quite interesting.

I don't want to switch the CI to only using pwsh tasks, because internally we have a system that's running PoSh 4.x that depends on this module (hence the min requirement in the module manifest). I like the (unintentional) outcome that the different platforms are thus testing the module against different versions of the runtime.

Again -- thanks for taking the time to investigate and respond!

@HowardWolosky HowardWolosky added the in progress Work on this issue is already underway. Please don't work start new work on it. label May 29, 2020
HowardWolosky added a commit that referenced this issue May 30, 2020
@HowardWolosky
Fix multi-result behavior across all versions of PowerShell (fixes CI…
bcd0a56 
… UT's on all platforms) (#199)

Tests were failing on Mac and Linux, but not Windows ([recent test run](https://dev.azure.com/ms/PowerShellForGitHub/_build/results?buildId=83887&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb)).  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:

https://github.com./microsoft/PowerShellForGitHub/blob/587e2042621091c79cc06be2aa9cc6ea836561f4/GitHubCore.ps1#L648
`...`
https://github.com./microsoft/PowerShellForGitHub/blob/587e2042621091c79cc06be2aa9cc6ea836561f4/GitHubCore.ps1#L670

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 `$finalResult` unless 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 `$null` entry, thus making `Count` 1, 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](https://stackoverflow.com/a/60330501) or you have to do wrap the result in an array on the caller side.

https://github.com./microsoft/PowerShellForGitHub/blob/587e2042621091c79cc06be2aa9cc6ea836561f4/GitHubCore.ps1#L684-L685

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build Changes related to the build infrastructure for the project. in progress Work on this issue is already underway. Please don't work start new work on it.
Projects
None yet
Milestone
No milestone
2 participants
@HowardWolosky @X-Guardian