test(sum): add more sum tests#1071
test(sum): add more sum tests#1071giard-alexandre wants to merge 1 commit intoreactivemarbles:mainfrom
Conversation
JakenVeina
left a comment
JakenVeina
left a comment
There was a problem hiding this comment.
Looks solid, overall. A few nitpicks, and a few suggested new tests/test cases.
| } | ||
|
|
||
| [Fact] | ||
| public void NoItemsAdded_NoSumEmitted() |
There was a problem hiding this comment.
This reflects what the current behavior is? If so, it's one of the things that should be considered for a change. The most compelling argument, I think, is that Enumerable.Sum() emits 0 on an empty source, rather than, say, throwing an exception, so that's probably what consumers are going to expect, intuitively.
Not asking for a change, if this is the current behavior, that's what the test should reflect. Just musing about something to consider in the next phase.
| } | ||
|
|
||
| [Fact] | ||
| public void NoItemsAdded_NoSumEmitted() |
There was a problem hiding this comment.
Me, I'd probably use SourceIsEmpty to describe the scenario here. It's slightly clearer about what's really being tested.
| } | ||
|
|
||
| [Fact] | ||
| public void AllItemsRemoved_SumReturnsToZero() |
There was a problem hiding this comment.
Yeah, this definitely confirms that we should be emitting 0 upon subscription to an empty source.
| [InlineData(0, 50)] | ||
| [InlineData(1, 40)] | ||
| [InlineData(2, 30)] | ||
| public void ItemIsRemoved_SumDecreases(int removalIndex, int expectedSum) |
There was a problem hiding this comment.
Probably SumReflectsRemoval works better, and matches what you've used elsewhere. This actually highlights that you should probably add test cases for negative values, for completeness' sake.
|
|
||
| [Fact] | ||
| public void AddedItemsContributeToSumDouble() | ||
| public class ListSource |
There was a problem hiding this comment.
Definitely split these into separate files. To match other operators where we've got multiple overloads, I'd do SumFixture.ForCache.cs and SumFixture.ForList.cs. Check out (if I recall correctly) ToObservableChangeSetFixture.
| } | ||
|
|
||
| [Fact] | ||
| public void SourceCompletesAfterEmitting_CompletionPropagates() |
There was a problem hiding this comment.
Add equivalent tests for SourceCompletesImmediately and SourceFailsImmediately.
Quite often, these scenarios need a little bit of custom logic, to ensure correct behavior, especially for operators that emit an initial value, or that should always emit a value when the source emits one.
In the case of Sum(), a source can exist that emits an initial changeset, and then a completion, because no new item changes are allowed, and these would both occur immediately upon subscription. We'd want to make sure that .Sum() also emits an initial sum value, for those items, and then a completion, both immediately upon subscription.
| } | ||
|
|
||
| [Fact] | ||
| public void ItemsAreAdded_SumIsCorrect_ForInt() |
There was a problem hiding this comment.
Since we're targeting specific numeric types, we should probably add some test cases for .MaxValue and .MinValue to guarantee no overflows occurring where there shouldn't be. Maybe as new test cases to these existing tests, maybe as dedicated test methods.
2 participants
This should address the first 3 bullet points in #1031 (comment)