OrleansContrib / OrleansTestKit

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

Unable to create multiple grains even though the test project does #115

Closed ElanHasson closed 3 years ago

ElanHasson commented 3 years ago
System.Exception : A grain has already been created in this silo. Only 1 grain per test silo should ever be created. Add grain probes for supporting grains.

Here is my code:

using System;
using System.Reflection;
using System.Threading.Tasks;
using FakeItEasy;
using AutoFixture;
using FluentAssertions;
using GenericPlatform.Interfaces;
using GenericPlatform.Silo.Grains;
using Microsoft.Extensions.Logging;
using NUnit.Framework;
using Orleans;
using Orleans.Runtime;
using Orleans.TestKit;
using Orleans.TestKit.Services;

namespace GenericPlatform.Grains.Tests
{
    public class EntityDefinitionGrainTests:TestKitBase
    {
        private EntityDefinition state;

        [OneTimeSetUp]
        public void OneTimeSetup()
        {
            this.state = new EntityDefinition();
            SetupState<EntityDefinition, EntityDefinitionGrain>(this.state);

        }
        private void SetupState<TState,TGrain>(TState state)
        {
            var mockState = A.Fake<IPersistentState<TState>>();
            A.CallTo(() => mockState.State).Returns(state);

            var mockMapper = A.Fake<IAttributeToFactoryMapper<PersistentStateAttribute>>();
            A.CallTo(() => mockMapper.GetFactory(A<ParameterInfo>._, A<PersistentStateAttribute>._))
                .Returns(context => mockState);

            Silo.AddService(mockMapper);
            var logger = A.Fake<ILogger<TGrain>>();
            Silo.ServiceProvider.AddService(logger);
        }

        [Test]
        public async Task VersionShouldBe_Zero_On_Initialization()
        {
            this.state = new EntityDefinition();
            var grain = await Silo.CreateGrainAsync<EntityDefinitionGrain>("0");

            var result = await grain.GetDefinition();
            result.Version.Should().Be(0);
            result.Properties.Should().BeEmpty();
        }

        [Test]
        public async Task VersionShouldBe_1_On_AfterOneOperation()
        {
            this.state = new EntityDefinition();

             var grain = await Silo.CreateGrainAsync<EntityDefinitionGrain>("0");
            await grain.AddProperty("Name", typeof(string).FullName!);
            var result = await grain.GetDefinition();
            result.Version.Should().Be(1);
            result.Properties.Should().ContainKey("Name");
        }
    }
}
seniorquico commented 3 years ago

@ElanHasson I'm not as familiar with NUnit, but did some quick reading here:

https://docs.nunit.org/articles/nunit/writing-tests/attributes/onetimesetup.html

Based on the docs, I think the lifecycle of your fixture is SingleInstance (based on the presence of the non-static [OneTimeSetUp] method). I think the problem here stems from the out-of-the-box difference between NUnit and xUnit. The test suite/demos in this repo use xUnit, which only operates in a mode equivalent to NUnit's InstancePerTestCase.

To simplify the implementation of the TestKit and to better match some of the runtime semantics of Orleans, a single TestKitSilo instance only works to produce a single grain activation. It is expected that you create a new TestKitSilo, run a unit of work, and then throw it away. I think there are many ways to accomplish this with NUnit and many may come down to personal preference. I'd be happy to brainstorm some code examples if that's helpful, though.

I think it's worth explicitly calling this out in the README.

ElanHasson commented 3 years ago

Thanks Kyle, I had similar thoughts.

It's been a while since I was working on that repo on question, so memory is a bit fuzzy.

I Agree with your assessment. What about a Reset() be method on the base class? Why can't I clear the "container"?

seniorquico commented 3 years ago

You can clear the container with the following. 😄

new TestKitSilo()

In all seriousness... Clearing the container would require re-creating and re-registering the various services/mocks. The [OneTimeSetUp] approach still wouldn't work. With [OneTimeSetUp], you'd instead need to loop through the various services/mocks and call their own Reset methods (which is different and specific to each mocking framework). Unfortunately, that flies opposite to the one-shot nature of the TestKitSilo, and it would likely require significant changes for little to no benefit.

Are you able to refactor your test fixture setup to use [SetUp] instead (also note the change in class inheritance)?

public class EntityDefinitionGrainTests
{
    private EntityDefinition state;
    private TestKitSilo silo;

    [OneTimeSetUp]
    public void OneTimeSetup()
    {
        this.state = new EntityDefinition();
    }

    [SetUp]
    public void Setup()
    {
        this.silo = new TestKitSilo();
        this.SetupState<EntityDefinition, EntityDefinitionGrain>(this.state);
    }

    private void SetupState<TState,TGrain>(TState state)
    {
        var mockState = A.Fake<IPersistentState<TState>>();
        A.CallTo(() => mockState.State).Returns(state);

        var mockMapper = A.Fake<IAttributeToFactoryMapper<PersistentStateAttribute>>();
        A.CallTo(() => mockMapper.GetFactory(A<ParameterInfo>._, A<PersistentStateAttribute>._))
            .Returns(context => mockState);

        this.silo.AddService(mockMapper);
        var logger = A.Fake<ILogger<TGrain>>();
        this.silo.ServiceProvider.AddService(logger);
    }

    [Test]
    public async Task VersionShouldBe_Zero_On_Initialization()
    {
        this.state = new EntityDefinition();
        var grain = await this.silo.CreateGrainAsync<EntityDefinitionGrain>("0");

        var result = await grain.GetDefinition();
        result.Version.Should().Be(0);
        result.Properties.Should().BeEmpty();
    }

    [Test]
    public async Task VersionShouldBe_1_On_AfterOneOperation()
    {
        this.state = new EntityDefinition();
        var grain = await this.silo.CreateGrainAsync<EntityDefinitionGrain>("0");

        await grain.AddProperty("Name", typeof(string).FullName!);

        var result = await grain.GetDefinition();
        result.Version.Should().Be(1);
        result.Properties.Should().ContainKey("Name");
    }
}

The above might be more complicated than you need. It all depends on your expectation to reuse the state instance field. If that could also be recreated, then you could move everything into the [SetUp] method and drop the [OneTimeSetUp] method.

In my opinion, the base class is so trivial it's not adding any value to the library. We don't use it in our projects anymore. There were cases with xUnit where we wanted to reuse a single TestKitSilo across multiple test cases (effectively the exact opposite case of this NUnit issue). We ended up finding that simply refactoring the setup code was the easiest approach.

I think you have two options to setup the fixture with NUnit:

  1. You can setup the TestKitSilo in a [OneTimeSetUp] method if you want to share a single grain activation across multiple test cases. To do this, simply move the CreateGrainAsync call from your test cases to your [OneTimeSetUp] method, and save the return value as an instance field.
  2. You can setup the TestKitSilo in a [SetUp] method if you want unique, separate grain activations per test case.

A special case of #2 would be the above code sample that combines [OneTimeSetUp] and [SetUp]. You setup some services in a [OneTimeSetUp] method and reuse them across multiple test cases, but you still create the TestKitSilo in the [SetUp] method for unique, separate grain activations per test case.

ElanHasson commented 3 years ago

We just switched as a team from NUnit to xUnit for a large project I'm working on.

I might as well work more in it :)

We can close this-- the info you provided should help others find this conversation and if they don't have the luxury of switching, can revive this dialog 😄

Thanks for your help!