-
Notifications
You must be signed in to change notification settings - Fork 420
Improve TestingSequence
assertions
#901
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
Conversation
TestingSequence
assertions
Codecov Report
@@ Coverage Diff @@
## master #901 +/- ##
=======================================
Coverage 92.41% 92.41%
=======================================
Files 112 112
Lines 3439 3439
Branches 1021 1021
=======================================
Hits 3178 3178
Misses 199 199
Partials 62 62 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
A future PR will make additional improvements, similar to viceroypenguin/SuperLinq#143.
|
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.
I like the extra checks. However, even though the specification allows the following:
- Per the specification, it is not a failure to call
.Dispose()
multiple times. The.Dispose()
method is expected to be idempotent, such that it is callable multiple times without throwing an exception. As such, we should not be afraid to take advantage of such behavior when it makes code easier.- Per the specification, it is not a failure to call
.MoveNext()
after receiving afalse
response. The enumerator is simply expected to continue to returnfalse
for each following call. As such, we should not be afraid to take advantage of such behavior when it makes code easier (see SimplifyZipLongest
implementation #905 for examples).
I would make it an option rather than a default of TestingSequence
for 3 reasons:
- We've almost never needed to rely on this so far.
- It should be very obvious in tests if someone is relying on such allowed behaviour and it should be challenged because doing extra work should be avoided if it can be helped.
- A hard-written implementation could be buggy on edges because not everyone reads the documentation/specifications and so if one can avoid inducing such bugs then it saves everyone time and trouble.
This has been superseded by PR #936. |
This PR updates the assertions made in
TestingSequence
:.Dispose()
multiple times. The.Dispose()
method is expected to be idempotent, such that it is callable multiple times without throwing an exception. As such, we should not be afraid to take advantage of such behavior when it makes code easier..MoveNext()
after receiving afalse
response. The enumerator is simply expected to continue to returnfalse
for each following call. As such, we should not be afraid to take advantage of such behavior when it makes code easier (see SimplifyZipLongest
implementation #905 for examples).IEnumerator
s do not complain when calling.MoveNext()
after disposal, it does indicate an error in our code to expect that.MoveNext()
is a valid behavior after we have disposed the iterator. As such, we should fail directly.IEnumerator
s return a default or the last value when calling.Current
after.MoveNext()
returnsfalse
, the spec does not make any promises on the usefulness of.Current
in this situation. More importantly, we should be relying on.MoveNext()
return value and not attempting to reference.Current
in these cases. As such, we should fail directly.IEnumerator
s do not complain when calling.Current
after disposal, it does indicate an error in our code to expect that.Current
is a valid behavior after we have disposed the iterator. As such, we should fail directly.