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".
There was a problem hiding this comment.
I couldn't discern the patten used in this code base so I went very basic. I can improve the name(s). I'm not sure what you would want for "UnitUnderTest" in the pattern.
| _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.
There was a problem hiding this comment.
What I was actually trying to demonstrate was what series of calls would lead to the error. This test is really just a step indicating it wasn't the problem. Filter() itself is tested more thoroughly in all the Filter* classes.
There was a problem hiding this comment.
Are you trying to say you want this test removed?
| // 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.
|
I updated test names and removed the extraneous comment per your feedback. Let me know if those names need further tweaking. I also added another test that moves the |
2 participants
See #1119