test(sum): add more sum tests#1071
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.
Me, I'd probably use SourceIsEmpty to describe the scenario here. It's slightly clearer about what's really being tested.
| [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.
giard-alexandre
force-pushed
the
tests/sum
branch
from
6d4c9cf to
4f4532e
Compare
|
Alright sorry for the extended delay in getting this pushed out, long story but I moved countries so free time was at a premium. @JakenVeina Everything mentioned above should have been addressed, I'll let you resolve the conversations if it's done to your liking! |
2 participants
This should address the first 3 bullet points in #1031 (comment)