-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Replace Assert.NotNull with Assert.Empty in AsyncEnumerable SequenceTests #117821
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
base: main
Are you sure you want to change the base?
Conversation
…ests Co-authored-by: stephentoub <[email protected]>
@copilot, these should probably all be Assert.Single rather than Assert.Empty. |
…e SequenceTests Co-authored-by: stephentoub <[email protected]>
I've implemented your suggestion with some refinements. Changed the assertions to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces Assert.NotNull
calls with more specific assertions in the AsyncEnumerable SequenceTests, changing from null checks to element count verification. The changes improve test precision by verifying actual sequence behavior rather than just non-nullness.
- Replaced generic
Assert.NotNull
calls with specific count assertions (Assert.Equal
,Assert.Single
) - Added detailed comments explaining expected sequence behavior for different step values
- Refactored loop-based assertions into explicit test cases with clear expectations
@copilot, does the similar SequenceTests in System.Linq for Enumerable have the same issues with the corresponding asserts? If so, can you fix those, too? |
Co-authored-by: stephentoub <[email protected]>
Head branch was pushed to by a user without write access
Yes, the System.Linq SequenceTests had the exact same Assert.NotNull issues. I've fixed them using the same approach - replacing with Assert.Single for cases that return one element and Assert.Equal with Count() for cases that return two elements. All tests pass. Commit: 1496c27 |
This PR updates the test assertions in
SequenceTests.cs
forSystem.Linq.AsyncEnumerable
to replace allAssert.NotNull
calls withAssert.Empty
calls, reflecting updated test expectations.Changes Made
Assert.NotNull(AsyncEnumerable.Sequence(...))
toAssert.Empty(AsyncEnumerable.Sequence(...))
Assert.NotNull(AsyncEnumerable.Sequence(...))
toAssert.Empty(AsyncEnumerable.Sequence(...))
Assert.NotNull(AsyncEnumerable.Sequence(...))
toAssert.Empty(AsyncEnumerable.Sequence(...))
Context
The modified assertions are within the
EndOutOfRange_Throws
test method, specifically in theValidateUnsigned<T>()
andValidateSigned<T>()
helper methods. These changes represent a semantic shift from verifying that sequences are not null to verifying that they contain no elements.Test Impact
Assert.NotNull
assertions were verifying that theAsyncEnumerable.Sequence()
method returns a non-null sequence, while the newAssert.Empty
assertions verify that the sequences contain no elements.According to the current implementation of
AsyncEnumerable.Sequence()
, the method always returns at least one element (the start value) and never returns an empty sequence. The test cases being modified include:AsyncEnumerable.Sequence(123, 123, 0)
- returns[123]
AsyncEnumerable.Sequence(123, 124, 1)
- returns[123, 124]
AsyncEnumerable.Sequence(123, 122, -1)
- returns[123, 122]
This change may indicate that either:
AsyncEnumerable.Sequence()
is expected to changeFiles Modified
src/libraries/System.Linq.AsyncEnumerable/tests/SequenceTests.cs
This pull request was created as a result of the following prompt from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.