Fix Exception in list static filter index assumptions#1120
Conversation
| public void Dispose() => _cleanup.Dispose(); | ||
|
|
||
| [Fact] | ||
| public void Add() |
There was a problem hiding this comment.
Test names should be a little more descriptive. Ideally, the pattern is "UnitUnderTest_Scenario_ExpectedOutcome", but that's certainly not followed everywhere, so at minimum, we just want to see some description of either "Scenario" or "ExpectedOutcome" in the name. Just saying "Add", "Refresh", etc. doesn't tell me anything about what we're intending to test here. Could be as simple as "KeyIsRemoved".
| _cleanup.Add( | ||
| _source.Connect() | ||
| .RemoveKey() | ||
| .Filter(x => x.Age < average) |
There was a problem hiding this comment.
In the test fixture for "RemoveKey", the only thing that should be getting tested is .RemoveKey(), What you're REALLY trying to test for here is behavior in .Filter(), specifically, .Filter()'s ability to support missing indexes. That belongs in the appropriate "Filter" fixture.
| // A Replace might have a negative CurrentIndex from a Refresh in RemoveKeyEnumerator | ||
| if (currentIndex < 0) | ||
| { | ||
| // this code mimics the previous logic |
There was a problem hiding this comment.
This comment doesn't belong here. Useful for the PR, sure, but just pulling up and reading this comment, as-is begs the question "What previous logic?". The comment above clarifying that CurrentIndex can be negative does plenty to tell us what's going on here: we don't have the index, so we've got to look it up.
| var itemState = upstreamItemsStates[change.Item.CurrentIndex]; | ||
| upstreamItemsStates[change.Item.CurrentIndex] = ( | ||
| var currentIndex = change.Item.CurrentIndex; | ||
| // A Replace might have a negative CurrentIndex from a Refresh in RemoveKeyEnumerator |
There was a problem hiding this comment.
Probably not even the only scenario. Boy do I hate working inside the old "list" code.
2 participants
See #1119