Skip to content
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.

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

[Housekeeping] Fix Unit Test Failure: Catastrophic failure: System.ArgumentOutOfRangeException #2479

Merged
merged 4 commits into from
Jan 29, 2025

Conversation

TheCodeTraveler
Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler commented Jan 29, 2025

Description of Change

This PR fixes the ArgumentOutOfRangeException occurring in the Run All Unit Tests step of our Build Library (windows-latest) pipeline:

[xUnit.net 00:00:29.07] Catastrophic failure: System.ArgumentOutOfRangeException : Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index')

The exception was happening because both MockPopup and MockPopupHandler were both calling HandlerCompleteTCS.SetResult(); in OnClosed causing a race condition where our test would sometimes fail and sometimes pass.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard

Additional information

This PR updates the PopupTests to use Assert.ThrowsAnyAsync<OperationCanceledException>() which will pass when either an OperationCanceledException is thrown or an TaskCanceledException is thrown since TaskCanceledException derives from OperationCanceledException; Assert.ThrowsAnyAsync<T>() allows for derived types, whereas Assert.ThrowsAsync<T>() does not allow derived types.

This PR fixes an edge case where await popupDismissedTaskCompletionSource.Task.WaitAsync(token) does not throw a TaskCanceledException despite the token being canceled when HandlerCompleteTCS.Task had already completed; the fix is to manually add token.ThrowIfCancellationRequested(); in Popup.OnClosed().

This PR changes readonly IPopup poup -> readonly MockPopup popup because of the discrepancy between void IPopup.OnDismissedByTappingOutsideOfPopup() and the awaitable Task MockPopup.OnDismissedByTappingOutsideOfPopup(). I also added the missing await keyword when the tests call MockPopup.OnDismissedByTappingOutsideOfPopup().

This PR adds readonly MockPopupHandler popupHandler to PopupTests, and assigns this handler to readonly MockPopup popup in the constructor: popupHandler = CreateElementHandler<MockPopupHandler>(popup);

This PR also adds GitHubActionsTestLogger to provide us with more comprehensive logs when tests fail. This will help us solve test errors in the future.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

@TheCodeTraveler
Copy link
Collaborator Author

@dotnet-policy-service agree

@TheCodeTraveler TheCodeTraveler enabled auto-merge (squash) January 29, 2025 19:35
Copy link
Contributor

@ne0rrmatrix ne0rrmatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this solves a huge bug. Good job! It is nice to see this bug getting quashed. I had no idea what had been causing my builds to fail.

@TheCodeTraveler TheCodeTraveler merged commit d694f5f into main Jan 29, 2025
17 checks passed
@TheCodeTraveler TheCodeTraveler deleted the Fix-Popup-Test-Failure branch January 29, 2025 23:31
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants