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

Revise tests to avoid using Eumerable.Range #1044

Open
42 tasks
atifaziz opened this issue Nov 16, 2023 · 4 comments
Open
42 tasks

Revise tests to avoid using Eumerable.Range #1044

atifaziz opened this issue Nov 16, 2023 · 4 comments

Comments

@atifaziz
Copy link
Member

atifaziz commented Nov 16, 2023

Starting with .NET 8, Eumerable.Range returns an instance of a private RangeIterator that implements IList<T> (see also dotnet/runtime#88249). A lot of the unit tests potentially went with the assumption that Eumerable.Range returns a pure sequence. There are some operators in MoreLINQ that are optimised for sources that are collections and/or lists so tests using Eumerable.Range may be testing those optimised paths rather than the actual intended path iterating the source as an IEnumerable<>. Those tests need to be revised to avoid Enumerable.Range altogether.

Potential candidates that need revision:

@viceroypenguin
Copy link
Contributor

I'll create the branch, that way we can each work on it. Honestly, they should all be TestingSequence regardless, and then it doesn't matter.

@atifaziz
Copy link
Member Author

@viceroypenguin Awesome.

they should all be TestingSequence regardless

PR #475 is addressing that.

@viceroypenguin
Copy link
Contributor

Ah, I remember this one. I used it as a foundation for some of the changes I did in SuperLinq to handle this consistency. I'll pull in the commits here and offer some additional improvements for you to decide on. I'll push something shortly.

@viceroypenguin
Copy link
Contributor

viceroypenguin commented Nov 16, 2023

Created the first draft in e3c5554. This should be a good foundation to start editing the other tests to match.

After review, I didn't see that much in #475 that was usable here, though there may be some overlap you see that I don't.

For the first commit, I did the following:

  • Add .ToTestData() extension method
    • This simplifies the creation of different testing sequences
    • Actually uses TestingSequence for SourceKind.Sequence instead of a simple sequence
  • Update all AggregateRight tests to use TestingSequence, either directly or via .ToTestData().

One open problem that you can probably solve quicker than me: The various tests that use TestCaseSource are not folding up into child properties of their method test. The existing test case methods don't exhibit the same behavior, so I'm not sure if the difference is because they're void methods, or if there is another factor. I don't use nUnit myself, having switched to the much simpler xUnit, so I don't have the background to know this without further research.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants