Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concept/Prototype for testing utilities #870

Merged
merged 1 commit into from
Jul 21, 2024

Conversation

JakenVeina
Copy link
Collaborator

@JakenVeina JakenVeina commented Feb 27, 2024

This is basically the concept, in my mind, of what our testing utilities should look like...

To summarize...

public static class ObservableExtensions
{
    public static IDisposable RecordCacheItems<TObject, TKey>(
        this    IObservable<IChangeSet<TObject, TKey>>      source,
        out     CacheItemRecordingObserver<TObject, TKey>   observer,
                IScheduler?                                 scheduler = null);

    public static IDisposable RecordListItems<TObject, TKey>(
        this    IObservable<IChangeSet<TObject, TKey>>      source,
        out     ListItemRecordingObserver<TObject, TKey>    observer,
                IScheduler?                                 scheduler = null);

    public static IDisposable RecordValues<T>(
        this    IObservable<T>              source,
        out     ValueRecordingObserver<T>   observer,
                IScheduler?                 scheduler = null);

    public static IObservable<IChangeSet<T>> ValidateChangeSets<T>(this IObservable<IChangeSet<T>> source);

    public static IObservable<IChangeSet<TObject, TKey>> ValidateChangeSets<TObject, TKey>(
        this    IObservable<IChangeSet<TObject, TKey>>  source,
                Func<TObject, TKey>                     keySelector);

    public static IObservable<T> ValidateSynchronization<T>(this IObservable<T> source);
}

public abstract class RecordingObserverBase<T>
    : IObserver<T>
{
    public Exception? Error { get; }
    public bool HasCompleted { get; }
    public bool HasFinalized { get; }
    public IReadOnlyList<Recorded<Notification<T>>> Notifications { get; }
}

public sealed class ValueRecordingObserver<T>
    : RecordingObserverBase<T>
{
    public IReadOnlyList<T> RecordedValues { get; }
}

public sealed class CacheItemRecordingObserver<TObject, TKey>
        : RecordingObserverBase<IChangeSet<TObject, TKey>>
    where TObject : notnull
    where TKey : notnull
{
    public IReadOnlyList<IChangeSet<TObject, TKey>> RecordedChangeSets { get; }
    public IReadOnlyDictionary<TKey, TObject> RecordedItemsByKey { get; }
    public IReadOnlyList<TObject> RecordedItemsSorted { get; }
}

public sealed class ListItemRecordingObserver<T>
        : RecordingObserverBase<IChangeSet<T>>
    where T : notnull
{
    public IReadOnlyList<IChangeSet<T>> RecordedChangeSets { get; }
    public IReadOnlyList<T> RecordedItems { get; }
}

In the case of DD today, the RecordingObserver classes are largely redundant, compared to the Aggregator classes (except for ValueRecordingObserver) so they could perhaps be removed, and kept as a concept for DD2. Otherwise, the intent here would be that we'd add more RecordingObserver classes and ValidateChangeSets() methods, as needed, for the additional types of changesets, such as IGroupedChangeSet<TObject, TKey>.

I think the advantage of the RecordingObserver implementation, over the Aggregator implementation (aside from consistency) is having them implemented without leveraging Observable.Create() or Observer.Create(). Using those, we potentially risk faults in operators under test being hidden by the safeguards that are baked into Observable.Create() and Observer.Create(). @dwcullop argues that since DD uses these exclusively, there's no point in testing for behaviors that they prevent, and he's turned around my opinion to agree. However, I still think it's worth having these implementations be safeguard-free, given that the code to do so remains very simple, and that there could be bad-behavior scenarios we're just not thinking of.

The ValidateChangeSets() methods are definitely a good fit for existing DD, since they can be added to existing tests, without having to touch how the Aggregator classes are implemented, or consumed.

I plan to take a pass over this code again, depending on the feedback I get, to try and de-duplicate it. I'd like, for example, the logic for "apply a change to an IDictionary<> or an IList<T>" to be shared code somewhere in DD internals, that we can reuse.

@JakenVeina JakenVeina force-pushed the feature/comprehensive-testing-utilities branch from 4410187 to 6d4fb86 Compare July 21, 2024 22:45
@JakenVeina JakenVeina marked this pull request as ready for review July 21, 2024 22:58
@JakenVeina JakenVeina merged commit 8fd1124 into main Jul 21, 2024
1 check passed
@JakenVeina JakenVeina deleted the feature/comprehensive-testing-utilities branch July 21, 2024 22:59
Copy link

github-actions bot commented Aug 5, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants