test: better Average tests - #1168
Conversation
| using var source = new TestSourceCache<Person, string>(person => person.Name); | ||
|
|
||
| using var subscription = source.Connect() | ||
| .Avg(person => person.Age, emptyValue: -1) |
There was a problem hiding this comment.
Okay, maybe you've got more context on this, given that you've probably looked at the source more than I have, after this PR, but.... how the hell does this make sense. The emptyValue parameter is explicitly documented as being "The resulting average value when there is no data." Why would we not expect the operator to emit this value in this test?
Is it actually a defect that the operator doesn't emit here? If so, I'd say implement the test as we WANT it to be, and mark it with Skip = "Existing defect: ..." and a short description. That's what I've been doing for other operators, as I've reworked them.
There was a problem hiding this comment.
As long as you're making new files, go ahead and set them up to match the proper organization scheme. I.E. AvgFixture instead of AverageFixture to match the actual operator name.
No, there isn't any documentation for what the "proper organization scheme" is, as it should just be obvious from looking at the codebase. Except the codebase is utterly inconsistent, at the moment, cause there's so much of it to get around to reviewing and reorganizing.
I suppose since #1031 is the effort to slowly reorganize and modernize the codebase, I should add some notes there.
| } | ||
|
|
||
| [Fact] | ||
| public void NullableValuesAreCountedAsZero() |
There was a problem hiding this comment.
Is this really a desirable behavior? I assume it's something that's explicitly coded into the current operator, yes? It strikes me as a bad idea to make null coalescing an opt-out behavior, as opposed to opt-in. Even moreso, CAN you opt out of it?
Might be worth a breaking change in the API to go ahead and make, while you're rewrite the operator anyway.
Thoughts?
| } | ||
|
|
||
| [Fact] | ||
| public void InvalidateWhenResubscribesAndReevaluatesValues() |
There was a problem hiding this comment.
If there's actually some behavior of .Avg() being tested here, that isn't tested elsewhere, the test name should clarify that. And ideally, it should be exercised without the use of another unrelated operator.
Otherwise, this test doesn't belong here. It seems to me that what's being tested here is actually the behavior of .InvalidateWhen(), not any behavior in .Avg(). All InvalidateWhen() is doing internally is a .Switch() over the upstream. so it's just unsubscribing and re-subscribing to it. There should already be separate tests for those things (at least, for un-subscribing, subscribing is basically already covered by every test).
| } | ||
|
|
||
| [Fact] | ||
| public void AggregateChangeSetOverload_PreservesAverageBehavior() |
There was a problem hiding this comment.
The "proper" organization for different operator overloads with the name is to give each one its own fixture. Which you're already doing with .ForCache and .ForList. Just make some more sub-fixtures off of that, say, .ForCache.ForChangeSet and .ForCache.ForAggregateChangeSet. See Cache.ToObservableChangeSetFixture for example.
If you don't think it's worth duplicating all the testing behavior, since one of these two operators is just an alias for the other, I'd agree. You can either make a base fixture class that they both inherit, say, .ForCache.Base, or just put the test functionality on the "inner" of the two, and have superficial testing of the alias's behavior on the other, with notes on the first fixture saying that the other fixture contains all the real behavioral tests.
| } | ||
|
|
||
| [Fact] | ||
| public void AlreadyCompletedSource_InitialAverageAndCompletionPropagate() |
There was a problem hiding this comment.
What I've done on other fixtures is combine this with the regular "SourceCompletes" test, and parameterize it with the StreamCompletionStrategy enum. Have a look at Cache.FilterFixture.Base.SourceFails_ErrorPropagates and Cache.FilterFixture.Status.SourceCompletes_CompletionPropagates.
If the if statements to handle two different behaviors ends up too tedious and you want to keep them as separate tests, at least use the same terminology of Immediate and Asynchronous termination. I.E. SourceCompletesImmediately and SourceCompletesAsynchronously.
There was a problem hiding this comment.
Apply applicable comments from .ForCache to this file as well.
There was a problem hiding this comment.
You actually want to keep this file (with renaming, mentioned below), but leave it empty. Without an empty file sharing the name prefix of the other two, you don't get any file nesting within IDEs. It's a change I made a few weeks ago to have files be automatically nested, based on name.
2 participants
Fleshing out Avg operator tests as the next step in #1031