Skip to content

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

Closed
wants to merge 4 commits into from
Closed

Improve TestingSequence assertions #901

wants to merge 4 commits into from

Conversation

viceroypenguin
Copy link
Contributor

@viceroypenguin viceroypenguin commented Nov 25, 2022

This PR updates the assertions made in TestingSequence:

  • 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 a false response. The enumerator is simply expected to continue to return false for each following call. As such, we should not be afraid to take advantage of such behavior when it makes code easier (see Simplify ZipLongest implementation #905 for examples).
  • While most IEnumerators 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.
  • While most IEnumerators return a default or the last value when calling .Current after .MoveNext() returns false, 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.
  • While most IEnumerators 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.

@viceroypenguin viceroypenguin changed the title Improve TestingSequence assertions Improve TestingSequence assertions Nov 25, 2022
@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #901 (74faa1f) into master (c081197) will not change coverage.
The diff coverage is n/a.

@@           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

@viceroypenguin
Copy link
Contributor Author

A future PR will make additional improvements, similar to viceroypenguin/SuperLinq#143.

  • Expressing exactly how many enumerations are allowed
  • Unit testing TestingSequence to prove it tests behavior correctly.

Copy link
Member

@atifaziz atifaziz left a 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 a false response. The enumerator is simply expected to continue to return false for each following call. As such, we should not be afraid to take advantage of such behavior when it makes code easier (see Simplify ZipLongest implementation #905 for examples).

I would make it an option rather than a default of TestingSequence for 3 reasons:

  1. We've almost never needed to rely on this so far.
  2. 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.
  3. 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.

@atifaziz
Copy link
Member

This has been superseded by PR #936.

@atifaziz atifaziz closed this Jan 20, 2023
@viceroypenguin viceroypenguin deleted the testsequence-fixes branch January 20, 2023 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants