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

test: add Missing unit tests for Grouping.Any #57550

Merged
merged 1 commit into from
Aug 18, 2021
Merged

test: add Missing unit tests for Grouping.Any #57550

merged 1 commit into from
Aug 18, 2021

Conversation

SEWeiTung
Copy link
Contributor

@SEWeiTung SEWeiTung commented Aug 17, 2021

Seeing there're some missing unit cases for the extension of Enumerable.GroupBy().Any(), here's the fix for it.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 17, 2021
@ghost
Copy link

ghost commented Aug 17, 2021

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

Issue Details

For 'IIListProvider''s logic: If you want to check whether it has values or not, ">0" is enough. Otherwise, according to the original logic, if the source is IIListProvider and the 'GetCount()' <0, it will go to 'e.MoveNext();'. This is a bit too duplicated.

Author: MaledongGit
Assignees: -
Labels:

area-System.Linq, community-contribution

Milestone: -

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Hi @MaledongGit, and thank you for your contribution!

This will actually break the implementation.

The IListProvider.GetCount method will return -1 if it cannot cheaply calculate the count, in which case we need to fall back to the default enumerator-based approach. This change will result in any IListProvider with O(n) count calculation being erroneously reported as being empty.

I'm seeing a few tests fail in CI, but none actually originate from System.Linq.Tests. This suggests a potential gap in our test coverage. @MaledongGit would you be interested in changing this into a PR that adds a test like that?

@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Aug 17, 2021
@eiriktsarpalis eiriktsarpalis self-assigned this Aug 17, 2021
@SEWeiTung
Copy link
Contributor Author

@eiriktsarpalis:

Thanks for your tips, and what you said is just what I feel strange about.... XD

I'm seeing a few tests fail in CI, but none actually originate from System.Linq.Tests. This suggests a potential gap in our test coverage

Add into the 'tests' under System.LINQ folder ONLY? Is there anything class or if I should mock a class inherted from IIListProvider?

@SEWeiTung
Copy link
Contributor Author

@eiriktsarpalis: I'd be happy if I could help :)

@eiriktsarpalis
Copy link
Member

This is the file containing all Enumerable.Any tests: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Linq/tests/AnyTests.cs. Adding a test would require finding an IListProvider<T> implementation that returns -1 if onlyIfCheap is set to true. One such example is the outer enumerator used by GroupBy:

onlyIfCheap ? -1 : Lookup<TKey, TElement>.Create(_source, _keySelector, _elementSelector, _comparer).Count;

@stephentoub stephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 17, 2021
@SEWeiTung SEWeiTung changed the title remove useless check of 'Enumerable's AnyAll' check test: add Missing unit tests for Grouping.Any Aug 17, 2021
@SEWeiTung
Copy link
Contributor Author

SEWeiTung commented Aug 17, 2021

Sorry to misunderstand it, and I've changed my PR to add the missing unit tests for Grouping().Any().

@eiriktsarpalis eiriktsarpalis removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 18, 2021
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Thank you @MaledongGit!

@SEWeiTung
Copy link
Contributor Author

Thank you @MaledongGit!

It's my fault, I should be responsible for it ;)

@eiriktsarpalis eiriktsarpalis merged commit 63f6461 into dotnet:main Aug 18, 2021
@SEWeiTung SEWeiTung deleted the improveAnyAll branch August 19, 2021 00:41
@ghost ghost locked as resolved and limited conversation to collaborators Sep 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Linq community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants