OrleansContrib / OrleansTestKit

Unit Test Toolkit for Microsoft Orleans
http://dotnet.github.io/orleans
MIT License
76 stars 42 forks source link

Implementation of ResumeAsync for TestStreamSubscriptionHandle type #114

Closed HuwLittle closed 2 years ago

HuwLittle commented 2 years ago

Allows to test persistent streams

HuwLittle commented 2 years ago

Ideally the Silo.DeactivateAsync(grain); would have a hook into the detach, but couldn't find an way to do this that was simple and clean

HuwLittle commented 2 years ago

@seniorquico - could you look at this one too please?

seniorquico commented 2 years ago

The ReactivateAsync method does not fit a supported workflow. This is because Orleans does not reuse C# object instances after a grain has been deactivated. If a new activation is created, even on the silo where the grain was previously activated, you'll get a new C# object instance. From a high-level, unit testing perspective, the difference between testing a grain instance that's never been activated before and a grain instance that's been activated many times before comes down to whether or not the grain rehydrates from external state. Within the TestKit, reusing C# object instances could lead to even more significant runtime differences with Orleans and introduce subtle bugs. The TestKit is designed to be a harness for mock-based testing of a single grain activation. Attempting its best to match Orleans, this translates to a single call to OnActivateAsync and a single call to OnDeactivateAsync on a given C# object instance.

If I understand correctly, the call to action here centers around this stubbed out method:

public override Task<StreamSubscriptionHandle<T>> ResumeAsync(IAsyncObserver<T> observer, StreamSequenceToken token = null) =>
    throw new NotImplementedException();

I know in our projects, we have code like the following:

var streamProvider = GetStreamProvider(PROVIDER_NAME);
var stream = streamProvider.GetStream<EventType>(GetPrimaryKey(), "EventNamespace");
var streamSubscriptionHandles = await stream.GetAllSubscriptionHandles();
foreach (var streamSubscriptionHandle in streamSubscriptionHandles)
{
    await streamSubscriptionHandle.ResumeAsync(OnEventTypeAsync);
}

Just like Orleans' persistent state and reminders, we need a way to predefine a subscription handle from the TestKitSilo to enable calling ResumeAsync (or even immediately calling UnsubscribeAsync). These are great scenarios to support testing, but we should find a solution that doesn't change our core assumptions about the C# object instances.

I'd also like to point out the Orleans TestCluster. This is the correct approach to pursue if your testing requirements grow beyond the scope of a single grain activation.

HuwLittle commented 2 years ago

Yep agree, I will remove that resume async. The core subscription handle would work if the Handler was part of an IPersistentState, but I need to think of the scenario where a user calls GetAllSubscriptionHandles().

Thanks for looking at this and for the suggestions.

HuwLittle commented 2 years ago

Hi @seniorquico - I've updated the code to remove the ResumeAsync hook as well as adding a test case for the situation where a grain will call stream.GetAllSubscriptionHandles() instead of storing the handle in an IPersistentState. Let me know what you think.

Thank you for your help and time.

seniorquico commented 2 years ago

Awesome, @HuwLittle. I'm traveling now, but I'll carve out some time next week for this. This will be a great addition to the test kit.

HuwLittle commented 2 years ago

Hi @seniorquico - have you managed to take a look at this by any chance?

HuwLittle commented 2 years ago

Hi @seniorquico - just another poke on this. Do you know roughly when we could expect you to look through this?

HuwLittle commented 2 years ago

Hey @seniorquico - would it be possible for you to look at this for me? I also don't mind helping out in reviewing PRs to this repository if you'd like some assistance?

SamEmber commented 2 years ago

@seniorquico Any chance someone could look at this and get it merged? I would also like this functionality.

seniorquico commented 2 years ago

Released to NuGet as 3.1.8.