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

Extension Doesn't Work with AutoMapper 6.2.0 In Integration Tests #36

Closed
adamrodger opened this Issue Nov 15, 2017 · 21 comments

Comments

Projects
None yet
6 participants
@adamrodger

adamrodger commented Nov 15, 2017

When integration testing with ASP.Net Core, it is very common to use TestServer to start your service as an in-memory host so that you can perform service-level integration tests via a real HttpClient.

However, since AutoMapper 6.2.0 was released this no longer works. Your Startup class is effectively called multiple times within the same process, so the AddAutoMapper() call happens multiple times, so the Mapper.Initialize() call also happens multiple times. This results in an exception in the second test:

System.InvalidOperationException : Mapper already initialized. You must call Initialize once per application domain/process.
   at AutoMapper.Mapper.set_Configuration(IConfigurationProvider value)
   at AutoMapper.Mapper.Initialize(Action`1 config)
   at AutoMapper.ServiceCollectionExtensions.AddAutoMapperClasses(IServiceCollection services, Action`1 additionalInitAction, IEnumerable`1 assemblies)
   at MyService.Startup.ConfigureServices(IServiceCollection services)

How can we 'clear out' the AutoMapper statics so that integration testing can be supported in a single process?

@lbargaoanu

This comment has been minimized.

Show comment
Hide comment
@lbargaoanu
Member

lbargaoanu commented Nov 15, 2017

@jbogard

This comment has been minimized.

Show comment
Hide comment
@jbogard

jbogard Nov 15, 2017

Member

I think we'll need a Mapper.Reset again because the built-in DI container for MS doesn't support configuring child containers (that's how I usually accomplish this).

Member

jbogard commented Nov 15, 2017

I think we'll need a Mapper.Reset again because the built-in DI container for MS doesn't support configuring child containers (that's how I usually accomplish this).

@lbargaoanu

This comment has been minimized.

Show comment
Hide comment
@lbargaoanu

lbargaoanu Nov 15, 2017

Member

I think a cleaner solution would be to make AddAutoMapperClasses receive a MapperConfiguration. Maybe use the static one by default.

Member

lbargaoanu commented Nov 15, 2017

I think a cleaner solution would be to make AddAutoMapperClasses receive a MapperConfiguration. Maybe use the static one by default.

@adamrodger

This comment has been minimized.

Show comment
Hide comment
@adamrodger

adamrodger Nov 15, 2017

From looking at the code, I agree with @lbargaoanu. I've made a local version of the extension which stops calling Mapper.Initialize and now all my integration tests pass. This is:

public static IServiceCollection AddAutoMapper(this IServiceCollection services,
                                               IEnumerable<Assembly> assemblies)
{
    Type baseType = typeof(Profile);
    var profiles = assemblies.SelectMany(a => a.ExportedTypes).Where(baseType.IsAssignableFrom);

    // create a new config with all those profiles registered
    var config = new MapperConfiguration(cfg =>
    {
        foreach (Type profile in profiles)
        {
            cfg.AddProfile(profile);
        }
    });

    services.TryAddSingleton<IMapper>(sp => new Mapper(config, sp.GetService));

    return services;
}

That does make the assumption that AutoMapper is thread-safe so you can safely share that one Mapper instance in all request threads, but since there's a static Mapper.Map I'm guessing that's safe.

adamrodger commented Nov 15, 2017

From looking at the code, I agree with @lbargaoanu. I've made a local version of the extension which stops calling Mapper.Initialize and now all my integration tests pass. This is:

public static IServiceCollection AddAutoMapper(this IServiceCollection services,
                                               IEnumerable<Assembly> assemblies)
{
    Type baseType = typeof(Profile);
    var profiles = assemblies.SelectMany(a => a.ExportedTypes).Where(baseType.IsAssignableFrom);

    // create a new config with all those profiles registered
    var config = new MapperConfiguration(cfg =>
    {
        foreach (Type profile in profiles)
        {
            cfg.AddProfile(profile);
        }
    });

    services.TryAddSingleton<IMapper>(sp => new Mapper(config, sp.GetService));

    return services;
}

That does make the assumption that AutoMapper is thread-safe so you can safely share that one Mapper instance in all request threads, but since there's a static Mapper.Map I'm guessing that's safe.

@jbogard

This comment has been minimized.

Show comment
Hide comment
@jbogard

jbogard Nov 15, 2017

Member

Adding a MapperConfiguration parameter would work, I think the problem is we do a ton of stuff in the constructor there. It'd probably need to be an IMapperConfigurationExpression instance instead?

That would help as well with people wanting to call services.AddAutoMapper multiple times.

Member

jbogard commented Nov 15, 2017

Adding a MapperConfiguration parameter would work, I think the problem is we do a ton of stuff in the constructor there. It'd probably need to be an IMapperConfigurationExpression instance instead?

That would help as well with people wanting to call services.AddAutoMapper multiple times.

@lbargaoanu

This comment has been minimized.

Show comment
Hide comment
@lbargaoanu

lbargaoanu Nov 15, 2017

Member

Whatever works, I guess, but if you pass something, it gets used, otherwise, the same old. I think :)

Member

lbargaoanu commented Nov 15, 2017

Whatever works, I guess, but if you pass something, it gets used, otherwise, the same old. I think :)

@lbargaoanu

This comment has been minimized.

Show comment
Hide comment
@lbargaoanu

lbargaoanu Nov 15, 2017

Member

@adamrodger Yes, it's safe.

Member

lbargaoanu commented Nov 15, 2017

@adamrodger Yes, it's safe.

@jbogard

This comment has been minimized.

Show comment
Hide comment
@jbogard

jbogard Nov 15, 2017

Member

Ah, that won't work either because I still have to create MapperConfiguration and register it with the container. And that thing's immutable.

There was another PR that added a bit more work by doing an external mapper builder but it was quite complicated.

There's already so many overloads that I think I'd have to build an options object. And I don't feel like breaking so much right now :)

The easiest thing is probably Mapper.Reset and call it a day.

Member

jbogard commented Nov 15, 2017

Ah, that won't work either because I still have to create MapperConfiguration and register it with the container. And that thing's immutable.

There was another PR that added a bit more work by doing an external mapper builder but it was quite complicated.

There's already so many overloads that I think I'd have to build an options object. And I don't feel like breaking so much right now :)

The easiest thing is probably Mapper.Reset and call it a day.

@adamrodger

This comment has been minimized.

Show comment
Hide comment
@adamrodger

adamrodger Nov 15, 2017

I dunno, I think my proof-of-concept works fine. It means you can't call AddAutoMapper() twice within the same Startup class, but I don't really see much of a use case for that. You can only register IMapper once regardless of the transient/scoped/singleton scope so it doesn't make sense to call it twice anyway.

That's a better solution than Mapper.Reset because you don't need to remember to call it in tear down code. You'll just get a fresh IMapper each time you start the service up in an integration test rather than being restricted to doing it once per process like you are at the moment.

adamrodger commented Nov 15, 2017

I dunno, I think my proof-of-concept works fine. It means you can't call AddAutoMapper() twice within the same Startup class, but I don't really see much of a use case for that. You can only register IMapper once regardless of the transient/scoped/singleton scope so it doesn't make sense to call it twice anyway.

That's a better solution than Mapper.Reset because you don't need to remember to call it in tear down code. You'll just get a fresh IMapper each time you start the service up in an integration test rather than being restricted to doing it once per process like you are at the moment.

@jbogard

This comment has been minimized.

Show comment
Hide comment
@jbogard

jbogard Nov 15, 2017

Member
Member

jbogard commented Nov 15, 2017

adamrodger pushed a commit to adamrodger/AutoMapper.Extensions.Microsoft.DependencyInjection that referenced this issue Nov 15, 2017

adamrodger pushed a commit to adamrodger/AutoMapper.Extensions.Microsoft.DependencyInjection that referenced this issue Nov 15, 2017

@adamrodger

This comment has been minimized.

Show comment
Hide comment
@adamrodger

adamrodger Nov 15, 2017

I've raised a MR to show the potential solution. All the tests still pass and that works within my integration tests which previously were failing.

The difference is that the static Mapper is now not initialised so you can't do Mapper.Map from anywhere you want, but the whole point of registering with DI in the first place is that you want to inject the IMapper instead of using statics isn't it?

adamrodger commented Nov 15, 2017

I've raised a MR to show the potential solution. All the tests still pass and that works within my integration tests which previously were failing.

The difference is that the static Mapper is now not initialised so you can't do Mapper.Map from anywhere you want, but the whole point of registering with DI in the first place is that you want to inject the IMapper instead of using statics isn't it?

@lbargaoanu

This comment has been minimized.

Show comment
Hide comment
@lbargaoanu

lbargaoanu Nov 15, 2017

Member

The mapper has to be scoped so it can receive per request dependencies.

Member

lbargaoanu commented Nov 15, 2017

The mapper has to be scoped so it can receive per request dependencies.

@adamrodger

This comment has been minimized.

Show comment
Hide comment
@adamrodger

adamrodger Nov 15, 2017

adamrodger commented Nov 15, 2017

@lbargaoanu

This comment has been minimized.

Show comment
Hide comment
@lbargaoanu

lbargaoanu Nov 15, 2017

Member

The mapper needs to create things per request, such as resolvers that receive a DbContext. The configuration needs to be a singleton. Other than that, I don't see anything wrong with your solution. But apparently @jbogard disagrees.

Member

lbargaoanu commented Nov 15, 2017

The mapper needs to create things per request, such as resolvers that receive a DbContext. The configuration needs to be a singleton. Other than that, I don't see anything wrong with your solution. But apparently @jbogard disagrees.

adamrodger added a commit to adamrodger/AutoMapper.Extensions.Microsoft.DependencyInjection that referenced this issue Nov 15, 2017

@adamrodger

This comment has been minimized.

Show comment
Hide comment
@adamrodger

adamrodger Nov 15, 2017

I've updated the MR and added tests to ensure the configuration is singleton but the mapper is scoped.

adamrodger commented Nov 15, 2017

I've updated the MR and added tests to ensure the configuration is singleton but the mapper is scoped.

@jbogard jbogard added this to the 3.2.0 milestone Nov 16, 2017

@jbogard jbogard closed this in 11f0eca Nov 16, 2017

@erikaspl

This comment has been minimized.

Show comment
Hide comment
@erikaspl

erikaspl Nov 22, 2017

with AutoMapper 6.2.1 I was able to resolve Mapper already initialized issue by registering Mapper.Reset() action for application shutdown in my startup's configure method:

public void Configure(IApplicationBuilder app, IHostingEnvironment env, 
            ILoggerFactory loggerFactory, IApplicationLifetime applicationLifetime)
    {
         .....
         applicationLifetime.ApplicationStopped.Register(() => AutoMapper.Mapper.Reset());
    }

When make sure TestServer dispose method is called in you test teardown:

        protected TestServer Server;
        [OneTimeSetUp]
        public void OneTimeBaseSetUp()
        {
             Server = new TestServer(....
        }


        [OneTimeTearDown]
        public void OneTimeBaseTearDown()
        {
            Server.Dispose();
        }

erikaspl commented Nov 22, 2017

with AutoMapper 6.2.1 I was able to resolve Mapper already initialized issue by registering Mapper.Reset() action for application shutdown in my startup's configure method:

public void Configure(IApplicationBuilder app, IHostingEnvironment env, 
            ILoggerFactory loggerFactory, IApplicationLifetime applicationLifetime)
    {
         .....
         applicationLifetime.ApplicationStopped.Register(() => AutoMapper.Mapper.Reset());
    }

When make sure TestServer dispose method is called in you test teardown:

        protected TestServer Server;
        [OneTimeSetUp]
        public void OneTimeBaseSetUp()
        {
             Server = new TestServer(....
        }


        [OneTimeTearDown]
        public void OneTimeBaseTearDown()
        {
            Server.Dispose();
        }
@sm-g

This comment has been minimized.

Show comment
Hide comment
@sm-g

sm-g Dec 17, 2017

@erikaspl So TestServer in your solution created only once for all tests? Not acceptable in some projects.
Adding (code from docs)

protected IntegrationTestFixture(string solutionRelativeTargetProjectParentDir)
{
   AutoMapper.ServiceCollectionExtensions.UseStaticRegistration = false;

   var startupAssembly = StartupType.Assembly;
   ...
}

was enough for me.

sm-g commented Dec 17, 2017

@erikaspl So TestServer in your solution created only once for all tests? Not acceptable in some projects.
Adding (code from docs)

protected IntegrationTestFixture(string solutionRelativeTargetProjectParentDir)
{
   AutoMapper.ServiceCollectionExtensions.UseStaticRegistration = false;

   var startupAssembly = StartupType.Assembly;
   ...
}

was enough for me.

@jbogard

This comment has been minimized.

Show comment
Hide comment
@jbogard

jbogard Dec 17, 2017

Member
Member

jbogard commented Dec 17, 2017

@erikaspl

This comment has been minimized.

Show comment
Hide comment
@erikaspl

erikaspl Feb 12, 2018

@sm-g [SetUp] and [TearDown] will work as well:

    public void Configure(IApplicationBuilder app, IHostingEnvironment env, 
            ILoggerFactory loggerFactory, IApplicationLifetime applicationLifetime)
    {
         .....
         applicationLifetime.ApplicationStopped.Register(() => AutoMapper.Mapper.Reset());
    }
        protected TestServer Server;
        [SetUp]
        public void OneTimeBaseSetUp()
        {
             Server = new TestServer(....
        }

        [TearDown]
        public void OneTimeBaseTearDown()
        {
            Server.Dispose();
        }

erikaspl commented Feb 12, 2018

@sm-g [SetUp] and [TearDown] will work as well:

    public void Configure(IApplicationBuilder app, IHostingEnvironment env, 
            ILoggerFactory loggerFactory, IApplicationLifetime applicationLifetime)
    {
         .....
         applicationLifetime.ApplicationStopped.Register(() => AutoMapper.Mapper.Reset());
    }
        protected TestServer Server;
        [SetUp]
        public void OneTimeBaseSetUp()
        {
             Server = new TestServer(....
        }

        [TearDown]
        public void OneTimeBaseTearDown()
        {
            Server.Dispose();
        }
@DrSensor

This comment has been minimized.

Show comment
Hide comment
@DrSensor

DrSensor Mar 31, 2018

I also run the same problem and my test are running in parallel. The only solution that is work for me is by locking the process and call Mapper.Reset() right before Mapper.Initialize is called:

private static Mutex mutex = new Mutex();

public void ConfigureServices(IServiceCollection services)
{
    mutex.WaitOne();
    Mapper.Reset();
    services.AddAutoMapper(x => x.AddProfile(new QLMapperProfile()), typeof(Startup));
    mutex.ReleaseMutex();
.
.

I also tried to lock the process and call Mapper.Reset() right before TestServer is instantiated but it fail.

DrSensor commented Mar 31, 2018

I also run the same problem and my test are running in parallel. The only solution that is work for me is by locking the process and call Mapper.Reset() right before Mapper.Initialize is called:

private static Mutex mutex = new Mutex();

public void ConfigureServices(IServiceCollection services)
{
    mutex.WaitOne();
    Mapper.Reset();
    services.AddAutoMapper(x => x.AddProfile(new QLMapperProfile()), typeof(Startup));
    mutex.ReleaseMutex();
.
.

I also tried to lock the process and call Mapper.Reset() right before TestServer is instantiated but it fail.

@jbogard

This comment has been minimized.

Show comment
Hide comment
@jbogard

jbogard Jul 18, 2018

Member

The static stuff with IServiceCollection just don't seem to mix, at all, so much so that I'm proposing that we remove the static initialization altogether and just to instance-based for services.AddAutoMapper.

Member

jbogard commented Jul 18, 2018

The static stuff with IServiceCollection just don't seem to mix, at all, so much so that I'm proposing that we remove the static initialization altogether and just to instance-based for services.AddAutoMapper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment