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.

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

Test IncrementalLoadingCollection itemsPerPage ctor param #4023

Merged
merged 1 commit into from
Aug 30, 2021
Merged

Test IncrementalLoadingCollection itemsPerPage ctor param #4023

merged 1 commit into from
Aug 30, 2021

Conversation

Rosuavio
Copy link
Contributor

@Rosuavio Rosuavio commented May 12, 2021 •
edited
Loading

Tests different items per page for IncrementalLoadingCollection

PR Type

What kind of change does this PR introduce?

Tests different values for itemsPerPage in the IncrementalLoadingCollection ctor.

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented May 12, 2021

Thanks RosarioPulella for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Kyaa-dost May 12, 2021 16:44
@ghost ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label May 12, 2021
@michael-hawker
Copy link
Member

@RosarioPulella was this just to check tests, should we commit this or are we waiting on something else still?

@Rosuavio
Copy link
Contributor Author

The original intention was to explore IncrementalLoadingCollection.LoadMoreItemsAsync(unit count) and providing different values for its param. But that did not seem to work the way I thought it would so instead this PR just tests the itemPerPage from the IncrementalLoadingCollection ctor.

@Rosuavio Rosuavio marked this pull request as ready for review June 22, 2021 19:07
@Rosuavio Rosuavio changed the title Test and Fix IncrementalLoadingCollection PageSize Test IncrementalLoadingCollection itemsPerPage ctor param Jun 22, 2021
@michael-hawker michael-hawker added this to the 7.1 milestone Jun 24, 2021
@michael-hawker michael-hawker added testing 🏗️ and removed bug 🐛 An unexpected issue that highlights incorrect behavior labels Jun 24, 2021
michael-hawker
michael-hawker reviewed Jun 24, 2021
View reviewed changes
UnitTests/UnitTests.UWP/UI/Collection/Test_IncrementalLoadingCollection.cs

var requests = new List<Task>();

for (int pageNum = 1; pageNum <= Pages; pageNum++)
for (var pageNum = 1; pageNum <= pages; pageNum++)
{
requests.Add(collection.LoadMoreItemsAsync(0).AsTask()
.ContinueWith(t => Assert.IsTrue(t.IsCompletedSuccessfully)));
Copy link
Member

Choose a reason for hiding this comment

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

Should we check the interim results for pageSize here as well as we're passing that into the test now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure its that easy since these calls are happening asynchronously and we would need to test ageist collection which will be changing as these tasks complete.

@michael-hawker michael-hawker added the for-review 📖 To evaluate and validate the Issues or PR label Jul 15, 2021
@michael-hawker michael-hawker merged commit 5e4b0b8 into CommunityToolkit:main Aug 30, 2021
@Rosuavio Rosuavio deleted the ilc-pageSize branch August 30, 2021 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XAML-Knight XAML-Knight XAML-Knight approved these changes

@michael-hawker michael-hawker michael-hawker approved these changes

@azchohfi azchohfi Awaiting requested review from azchohfi

@Kyaa-dost Kyaa-dost Awaiting requested review from Kyaa-dost

Assignees

@XAML-Knight XAML-Knight

Labels
for-review 📖 To evaluate and validate the Issues or PR testing 🏗️
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
3 participants
@Rosuavio @michael-hawker @XAML-Knight