Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 144 additions & 0 deletions src/DynamicData.Tests/Cache/RemoveKeyFixture.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
#region

using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
using System.Reactive.Disposables;

using DynamicData.Binding;
using DynamicData.Tests.Domain;

using FluentAssertions;

using Xunit;

#endregion

namespace DynamicData.Tests.Cache;

public class RemoveKeyFixture : IDisposable
{
private readonly RandomPersonGenerator _generator = new();

[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "CA2213:Disposable fields should be disposed", Justification = "Handled with CompositeDisposable")]
private readonly ISourceCache<Person, string> _source;

private readonly CompositeDisposable _cleanup = new();

public RemoveKeyFixture()
{
_source = new SourceCache<Person, string>(p => p.Key);
_cleanup.Add(_source);
}

public void Dispose() => _cleanup.Dispose();

[Fact]
public void CacheRemoveKey_Add_KeyIsRemoved()
{
ReadOnlyObservableCollection<Person> collection;
_cleanup.Add(
_source.Connect()
.RemoveKey()
.Bind(out collection)
.Subscribe()
);
var people = _generator.Take(100).ToArray();
_source.AddOrUpdate(people);

Assert.Equivalent(people, collection);
}

[Fact]
public void CacheRemoveKey_Filter_ItemsFilterKeyIsRemoved()
{
var people = _generator.Take(100).ToArray();
var average = people.Average(x => x.Age);

ReadOnlyObservableCollection<Person> collection;
_cleanup.Add(
_source.Connect()
.RemoveKey()
.Filter(x => x.Age < average)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you trying to say you want this test removed?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'm saying move it over to the Filter fixture. .Filter() is the operator here that has defective behavior. If other operators are capable of generating changesets with missing list indexes, .Filter() needs to be able to process them. At least, that's how you've written the fix. With that being a behavior that .Filter() needs, there should ve a test in the Filter fixture that feeds missing indexes into the operator, and verifies that it works.

.Bind(out collection)
.Subscribe()
);
_source.AddOrUpdate(people);

Assert.Equivalent(people.Where(x => x.Age < average), collection);
}

[Fact]
public void CacheRemoveKey_AutoRefreshUpdateITems_CollectionUpdated()
{
ReadOnlyObservableCollection<Person> collection;
_cleanup.Add(
_source.Connect()
.AutoRefresh(x => x.Age)
.RemoveKey()
.Bind(out collection)
.Subscribe()
);
var people = _generator.Take(100).ToArray();
_source.AddOrUpdate(people);

Assert.Equivalent(people, collection);

foreach (var person in people)
{
person.Age = person.Age + 1;
}
Assert.Equivalent(people, collection);
}

[Fact]
public void Cache_AutoRefreshRemoveKeyFilterUpdate_CollectionUpdated()
{
var people = _generator.Take(100).ToArray();
var average = people.Average(x => x.Age);
ReadOnlyObservableCollection<Person> collection;
_cleanup.Add(
_source.Connect()
.AutoRefresh(x => x.Age)
.RemoveKey()
.Filter(x => x.Age < average)
.Bind(out collection)
.Subscribe()
);
_source.AddOrUpdate(people);

Assert.Equivalent(people.Where(x => x.Age < average), collection);

foreach (var person in people)
{
person.Age = person.Age + 1;
}
Assert.Equivalent(people.Where(x => x.Age < average), collection);
}

[Fact]
public void Cache_AutoRefreshFilterRemoveKeyUpdate_CollectionUpdated()
{
var people = _generator.Take(100).ToArray();
var average = people.Average(x => x.Age);
ReadOnlyObservableCollection<Person> collection;
_cleanup.Add(
_source.Connect()
.AutoRefresh(x => x.Age)
.Filter(x => x.Age < average)
.RemoveKey()
.Bind(out collection)
.Subscribe()
);
_source.AddOrUpdate(people);

Assert.Equivalent(people.Where(x => x.Age < average), collection);

foreach (var person in people)
{
person.Age = person.Age + 1;
}
Assert.Equivalent(people.Where(x => x.Age < average), collection);
}
}
17 changes: 13 additions & 4 deletions src/DynamicData/List/Internal/Filter.Static.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ public static IObservable<IChangeSet<T>> Create(

var downstreamItems = new ChangeAwareList<T>();
var itemsBuffer = new List<T>();

var downstream = source.Select(upstreamChanges =>
{
foreach (var change in upstreamChanges)
Expand Down Expand Up @@ -212,13 +211,23 @@ public static IObservable<IChangeSet<T>> Create(
{
var isIncluded = predicate.Invoke(change.Item.Current);

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not even the only scenario. Boy do I hate working inside the old "list" code.

if (currentIndex < 0)
{
var previous = upstreamItemsStates.Select(x => x.item)
.IndexOfOptional(change.Item.Current)
.ValueOrThrow(() => new InvalidOperationException($"Cannot find index of {typeof(T).Name} -> {change.Item.Current}. Expected to be in the list"));
currentIndex = previous.Index;
}
var itemState = upstreamItemsStates[currentIndex];

upstreamItemsStates[currentIndex] = (
item: change.Item.Current,
isIncluded: isIncluded);

var downstreamIndex = (isIncluded || itemState.isIncluded)
? change.Item.CurrentIndex - CountExcludedItemsBefore(change.Item.CurrentIndex)
? currentIndex - CountExcludedItemsBefore(currentIndex)
: -1;

switch (itemState.isIncluded, isIncluded)
Expand Down
Loading