Fixed bug where IPickerExtension.GetItemsAsList() would always return an empty list#31879
Conversation
|
Would it be possible to add a unit test for this? Great catch! |
|
Will do.
|
|
I couldn't get the unit tests to execute locally. Will Github run them and report failures? |
@rmarinho will know. I'm not sure. But one can check an older PR like https://github.com/dotnet/maui/pull/31831/checks and see if the AZP contains logs for passed tests for the similar category. I can see here https://dev.azure.com/xamarin/public/_build/results?buildId=151456&view=ms.vss-test-web.build-test-results-tab: 
that one can select "Passed" tests: 
|
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 31879 in repo dotnet/maui |
The build did run the unit tests and they failed: https://dev.azure.com/xamarin/public/_build/results?buildId=151620&view=ms.vss-test-web.build-test-results-tab |
|
Looks like someone else will need to trigger the build. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Not sure if those failing UI tests have anything to do with my changes. |
Probably not :) so no worries |
|
@jfversluis Could you please do a review? |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in IPickerExtension.GetItemsAsList() where the method would always return an empty list due to a misunderstanding of the List<T>(int) constructor. The constructor sets the list's capacity but not its size, so the loop that tried to assign items by index never executed. The fix changes the implementation to use Add() to properly populate the list.
Key changes:
- Fixed the
GetItemsAsList()implementation to useAdd()instead of index assignment - Added comprehensive unit tests to verify both
GetItemsAsArray()andGetItemsAsList()work correctly - Added a new test category for Extensions tests
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Core/src/Core/Extensions/IPickerExtension.cs | Fixed the bug by using Add() instead of index assignment and removed unused System.Linq import |
| src/Core/tests/UnitTests/Extensions/IPickerExtensionTests.cs | Added new test file with comprehensive test coverage for both extension methods |
| src/Core/tests/UnitTests/TestCategory.cs | Added "Extensions" test category constant |
|
Nope, I'm confused with UI tests, ignore me. Carry on! Kerry on? 🤔 |
… an empty list (#31879) * Fixed bug where IPickerExtensions.GetItemsAsList() would always return an empty list. * Added unit tests for IPickerExtension. * Fixed unit tests.
… an empty list (#31879) * Fixed bug where IPickerExtensions.GetItemsAsList() would always return an empty list. * Added unit tests for IPickerExtension. * Fixed unit tests.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
The existing code assumes the constructor of
List<T>that takes anintwill create a list of that size. In reality, it creates a list of size 0 with a capacity of the provided number. The change fixes this by callingAdd()for each new element.Issues Fixed
Fixes #31878