-
Notifications
You must be signed in to change notification settings - Fork 409
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
Improve and Validate TestingSequence
assertions
#936
Conversation
Codecov Report
@@ Coverage Diff @@
## master #936 +/- ##
==========================================
+ Coverage 92.39% 92.44% +0.04%
==========================================
Files 112 113 +1
Lines 3434 3453 +19
Branches 1021 1025 +4
==========================================
+ Hits 3173 3192 +19
+ Misses 199 196 -3
- Partials 62 65 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Up to you - feels like separate PRs to me, given the nature of the work, but if you feel they're the same, then this one could be pulled directly without #901. PR description here updated as if single PR. |
TestingSequence
assertions
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.
This is really a great set of improvements and having gone the extra distance to test the TestingSequence<T>
implementation is just absolute bonus! Thanks!
I have made several suggestions to simplify things. Have a look and then I think we're good to merge soon.
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'll address the code changes when I get a chance.
Co-authored-by: Atif Aziz <code@raboof.com>
Co-authored-by: Atif Aziz <code@raboof.com>
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.
Thanks for all the improvements, follow-up and tests added with this PR! Good to merge! 🚀
Woot! 🎉 |
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. Updated to allow relying on this specification viaOptions.AllowRepeatedDispoals
..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). Updated to allow relying on this specification viaOptions.AllowRepeatedMoveNexts
.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.AllowMultipleEnumerations
to an exact expectation of how many times the sequence should be enumerated.TestingSequence
validates what it claims it validates; giving us better confidence that invalid MoreLinq and MoreLinq.Test code is written correctly.