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

More specialization tests #799

Merged

Conversation

Philippe-Cholet
Copy link
Member

@Philippe-Cholet Philippe-Cholet commented Nov 12, 2023

  • To do the next point, test_specializations need clonable iterators which helped find iterators that should be clonable but were not yet. 3 very small commits.
  • I do not want to add specialization tests one by one in future pull requests so this is mostly done! 1 big but straightforward commit.
  • For two tests I previously wrote, I now discard inputs (in the quickcheck-way) instead of truncating them. 1 small commit.

Now about mostly done... Not all of our iterators are tested here:

  • group_by
  • chunks
  • tee
  • rciter
  • test_specializations need clonable iterators but &mut iterator is not clonable, Therefore, we can't clone PeekingTakeWhile and TakeWhileRef. We need to find a way to test them too.

Finally a bug... multi_cartesian_product is not tested here because specializations of last and count FAIL sometimes, and I don't want this pull request to be blocked by it! It requires further investigation and will have a dedicated pull request.
EDIT: Apparently, the iterator is not fused even when all child iterators are, and this was the intended behavior, that's confusing.

Later EDITs: While making a bunch of specialization benchmarks, I noticed that I forgot some and some here:

  • repeat_n
  • TupleBuffer (buffer for a finished .tuples())
  • MapForGrouping used for GroupingMapBy but it's strictly internal to the library, a method of GroupingMapBy should be tested to test the specialization of future MapForGrouping::fold.
  • unfold, iterate: infinite iterators so e.g. test last would not finish.

Copy link
Member

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

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

This looks good to me. You can go ahead and merge this as-is, or you can add a test for multi_cartesian_product and mark it as #[ignore] (so we don't forget about it, but it also doesn't fail CI).

`private::{DuplicatesBy, Meta}` both derive `Clone` but it's not enough for `DuplicatesBy` and `Duplicates` to derive it as well, because `ById` and `ByFn` do not yet.
I think users are do not want to clone it as it would clone a `HashMap` and it would probably be slow.
However, I need that to add specialization tests.
I do not want to add them one by one in future pull requests so this is done!
For tests to not be slow, we sometimes need small enough inputs but I should not have truncated inputs previously.
Thanks to jswrenn, I now know that the right quickcheck-way is to discard some inputs (it then asks for other inputs).
@Philippe-Cholet Philippe-Cholet force-pushed the new-specialization-tests branch from 234bb53 to 62e11b0 Compare November 13, 2023 13:10
@Philippe-Cholet Philippe-Cholet added this pull request to the merge queue Nov 13, 2023
Merged via the queue into rust-itertools:master with commit e44dd53 Nov 13, 2023
8 checks passed
@Philippe-Cholet Philippe-Cholet deleted the new-specialization-tests branch November 13, 2023 13:15
@jswrenn jswrenn mentioned this pull request Nov 14, 2023
@jswrenn jswrenn added this to the next milestone Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants