dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.79k stars 3.19k forks source link

Exception when trying to use InMemoryDatabase for unit testing #6080

Closed Jargon64 closed 2 years ago

Jargon64 commented 8 years ago

I'm not sure whether this goes here or in the Testing repo but it's relating to EF. I'm trying to get xUnit working with EF's InMemoryDatabase but I'm having an issue I can't get around and none of the documentation is helping. I've Google'd around for my error but no one else seems to have experiencing it. It's either the documentation is out of date or I'm doing something wrong.

I have the following controller test constructor (it's failing at the constructor so I guess no more is needed):

public ApiAuthControllerTest()
{
    serviceProvider = new ServiceCollection()
        .AddEntityFrameworkInMemoryDatabase()
        .BuildServiceProvider();

    var optionsBuilder = new DbContextOptionsBuilder<ApplicationDbContext>()
        .UseInMemoryDatabase()
        .UseInternalServiceProvider(serviceProvider);

    loggerFactory = new TestLoggerFactory();

    context = new ApplicationDbContext(optionsBuilder.Options);

    controller = new ApiAuthController(context, null, null, null, null, null, loggerFactory);
}

And when I run the test from the command line I'm getting the following exception:

System.InvalidOperationException : Unable to resolve service for type 'Microsoft.EntityFrameworkCore.Storage.IRelationalConnection'. This is often because no database provider has been configured for this DbContext. A provider can be configured by overriding the DbContext.OnConfiguring method or by using AddDbContext on the application service provider. If AddDbContext is used, then also ensure that your DbContext type accepts a DbContextOptions<TContext> object in its constructor and passes it to the base constructor for DbContext.
  Stack Trace:
       at Microsoft.EntityFrameworkCore.Infrastructure.AccessorExtensions.GetService[TService](IInfrastructure`1 accessor)
       at Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.SetCommandTimeout(DatabaseFacade databaseFacade, Nullable`1 timeout)
       at Project.Models.ApplicationDbContext..ctor(DbContextOptions`1 options)
       at Project.Tests.Controllers.ApiAuthControllerTest..ctor()

I'm probably doing something wrong but I've tried every variation from the documentation and posts I've found around the web to no avail. A little guidance here would be extremely appreciated.

ajcvickers commented 8 years ago

@Jargon64 When you are using the in-memory database you can't make calls to relational-specific code. SetCommandTimeout is one such call. See #5150 for a discussion of testing relational code.

Jargon64 commented 8 years ago

@ajcvickers Thanks for the swift response and pointing me in the right direction. Just skimming over that other issue highlights that I'm going to continue to experience issues if I keep heading down this path. I think I will attempt to use SQLite in memory mode as has been suggested and hope it works for everything I need to do.

radenkozec commented 8 years ago

@ajcvickers I need to use raw SQL with Entity Framework Core and I am stuck now with same issue with InMemoryDatabase.

Is there a way to execute raw SQL with Entity Framework and not get this error?

ajcvickers commented 8 years ago

@radenkozec If you are asking whether you can execute raw SQL against the in-memory database, then the answer is no. Seems like a good case for using SQLite in memory mode.

radenkozec commented 8 years ago

@ajcvickers Strange that you suggest SQLite in memory mode as a solution on every post and I have for example 30+ integration tests using in-memory database that works great. Now I have only 1-2 particular cases where I need to use raw SQL because Range Remove in Entity Framework is doing foreach and not work as expected and you suggest that I drop everything and migrate to SQLite.

Is there a option to supply raw SELECT for Range Remove and then use that select to form Raw delete that will be used instead of RangeRemove? Similar like we have with Adapter / Dataset in Visual Studio 2003 where we supplied Select and based on that Adapter generated automatically Insert/Update/Delete

jjxtra commented 5 years ago

I just added a try/catch around my Migrate call so it gets eaten during tests

danielleiszen commented 5 years ago

I would suggest having a switch on EntityFramework InMemory configuration options whether we would like it to throw these kind of exceptions or not. Then we wont. Most of the code that is not supported - if not all of it - has nothing to do with our business logic, hence can be ignored during unit testing.

jholovacs commented 5 years ago

This should be reopened. The problem is not the lack of support, the problem is it throws an exception. Let the InMemoryDb ignore the relational-db specific calls, they shouldn't really be part of testable business logic anyway... but the way things are right now, you basically cannot use the in-memory db context for its intended purpose if you have any code that references an unsupported feature. That's just bad design.

roji commented 5 years ago

@jholovacs in some cases it's possible to ignore certain relational features without any impact (e.g. timeouts), but in many (most?) others that simply doesn't work, e.g. transactions, raw SQL... At the end of the day, if you require relational features in your tests, then you should be looking at a relational database (e.g. in-memory Sqlite).

[...] they shouldn't really be part of testable business logic anyway

Agreed, but to me that points more in the direction of the user not setting the timeout in tests in the first place, removing the need for InMemory to ignore it.

jholovacs commented 5 years ago

@roji While I agree you lose some testability for the relational aspects, that is usually not the focus of the unit tests (at least the ones I write)... let's say there is a transaction or timeout set in the code for a class. If we accept the fact that we cannot really test EF's implementation with the in-memory context, we can still test our business logic of what we do with the data once we fetch it... if it doesn't blow up. As long as it does blow up, though, I can't use the in-memory context for testing anything at all in my service, which functionally makes the in-memory context completely pointless.

smitpatel commented 5 years ago

they shouldn't really be part of testable business logic anyway

If something is not part of testable business logic then probably shouldn't test it. And if the code you are trying to unit test is using relational features then either you need to use relational provider to test or your code is not refactored/abstracted enough to isolate relational features.

jholovacs commented 5 years ago

@smitpatel

If something is not part of testable business logic then probably shouldn't test it.

...that's what I'm saying. If you inject a dbcontext as a service into your class, you shouldn't be testing any of its functionality, you should just simulate what it would do under "normal" circumstances so you can test what your class does with the service. The db context is already extremely difficult/ impossible to mock as you would normally do with an injected service; the in-memory dbcontext is the generally encouraged way to "mock" the context. But... if it throws exceptions because some particular feature is meaningless (like a CommandTimeout... which is actually not a "relational db" feature at all, but rather an I/O issue, likewise transactions are a transactional feature and not a relational feature) then you cannot use this method to test what actually should be tested.

We shouldn't be testing relational features, or literally any db context features in our classes, but that doesn't mean we shouldn't be using relational features. Those should be tested by the writers of the db context code. If I put a CommandTimeout of 120 for a particular method, I'm just circumventing a problem that has nothing to do with the code I'm writing, but rather DB system behind the abstraction of the db context. I'm not testing that. Likewise, for transactions, I shouldn't be testing that some data operation is properly rolled back, that code is someone else's, and should already be tested; I should be testing that I'm taking some action upon a rollback event, because that's the business logic that I wrote.

For example, let's say I have a service class like so:

public class FooService : IFooService
{
    private readonly MyDbContext _context;
    public FooService(MyDbContext context)
    {
        _context = context;
    }

    public async Task<Foo> GetTheFoo(ing fooId)
    {
        var foo = await _context.Foos.FirstOrDefaultAsync(x => x.Id == fooId);
        if (foo == null) 
        {
            foo = new Foo {Id = fooId, Status = Status.New};
            await _context.AddAsync(foo);
            await _context.SaveChangesAsync();
        }
        return foo;
    }
}

The only business logic here that's really testable is, "if the foo with the supplied ID doesn't exist, create it and return the result".

This would be a perfect use case for using an in-memory context for testing... but if we have a SetCommandTimeout in the db context definition, we can't do it. That really does not make a lick of sense to me.

smitpatel commented 5 years ago

If you are inject DbContext in your method then InMemory provider also works because all the relational configuration should happen before DbContext is injected. But if you are setting CommandTimeOut inside your method then you cannot test that method. That method is not abstracted out with work with any DbContext provider. You are using relational code.

The only business logic here that's really testable is, "if the foo with the supplied ID doesn't exist, create it and return the result".

Though you say about testing only business logic, you are still testing DbContext. If you want to just test busiless logic then you should have a method which fetches you foo, if null then create a new foo and call a method to add it to database. That way you can mock the methods which does actual DbContext tasks and test only your business logic.

Mocking DbContext is not recommended but if you are saying that you want to test "business logic" which does not require connecting to database, then refactor your code to unit test business logic. If connecting to DbContext is required then use InMemory DbContext but make sure your code is not using something which is provider specific like command timeout/transaction. If your code is using relational features than you have to use relational database.

If you don't want to test relational features then don't call into code which uses them. You may need to refactor to separate out that from your business logic.

roji commented 5 years ago

Just to state the obvious, if your method uses transactions and these are silently being ignored in tests (because InMemory is being used), your tests are falsely telling you everything is fine although you have not actually tested anything.

jholovacs commented 5 years ago

I'm sorry, I think you are suggesting a very impractical and pointless solution to get around a problem that seems entirely created for no good reason, and only exists under testing conditions. What does a command timeout have to do with a relational database? All databases, relational or otherwise, are queryable, and could theoretically time out. If the underlying database is not something that will time out, the proper thing to do is to ignore the command, not throw an exception.

The db context is a service that is injected for managing stored data in some database, right? What you seem to be suggesting is to inject the db context into some shim service whose sole job is to pretend it doesn't care whether or not the database is relational to get around a testing problem and then inject that service into other classes that need to access the data. That sounds flatly absurd... it's making an abstraction layer to address a design flaw in the db context that isn't even being used under runtime circumstances.

@roji I completely disagree with you; my tests tell me that I am doing what I mean to be doing. I'm not testing that transactions work as expected; that's not my code.

roji commented 5 years ago

@jholovacs I think the problem is that you're only thinking about "positive" scenarios, i.e. when the timeout doesn't fire and when the transaction commits and completes successfully.

Transactions have a well-defined, visible behavior, and if your code is using them they're requesting that behavior. An InMemory database is incapable of rolling back, so if your test code rolls back (as part of the scenario it's exercising!), we're lying to it by ignoring that. To make sure I'm understood, imagine someone running a test explicitly exercising how their logic behaves when a transaction is rolled back - they are legitimately expecting inserted data not to be in the database after the rollback (again, not a simple happy scenario where all transaction succeed and can therefore be ignored entirely). This is before discussing tests which depend on transaction isolation in the face of concurrency, which InMemory again cannot provide. All these may not be the scenarios you need right this minute, but we must take them into account - so we can't just ignore calls.

The same is theoretically true with timeouts (how does my logic behave in the face of actual timeouts?), although I agree actual scenarios are more far-fetched.

my tests tell me that I am doing what I mean to be doing. I'm not testing that transactions work as expected; that's not my code.

If you are not testing transactions work, your test should not be calling functions that tell us to start, commit or rollback transactions. You are saying one thing in your code, but here you're telling that you don't really mean what you said.

What you seem to be suggesting is to inject the db context into some shim service whose sole job is to pretend it doesn't care whether or not the database is relational [...]

There are other simpler solutions for this. Your function inside the context could simply check if it's being executed against a relational database, and set the timeout only if so.

jholovacs commented 5 years ago

An InMemory database is incapable of rolling back, so if your test code rolls back (as part of the scenario it's exercising!), we're lying to it by ignoring that.

This is precisely what I was trying to get at; if I was unit testing a method where this happens, I definitely should not be testing that the rollback in the data store worked, because none of that is code that I wrote. I should instead be testing that the code I wrote is doing as I intended. Were I to want to do an integration test where I want to look at the data store state at the end of an operation, then clearly an in-memory db would be insufficient. I can't see this as "lying" as much as ignoring something that obviously does not apply to the situation.

If you are not testing transactions work, your test should not be calling functions that tell us to start, commit or rollback transactions.

That's simply not true. If I was writing the database or EF code, then I would be testing whether or not the rollback functionality worked, but if I'm writing my tests for code that invokes the rollback, I should be quite confident that the rollback behaved as expected, since that code isn't mine and has been tested by its author already. Why in the world would I test that?

Your function inside the context could simply check if it's being executed against a relational database, and set the timeout only if so.

Again... this is putting logic into our context just so we can test against it... IMO, it's an awful idea.

It seems I'm not going to be able to convince you, but I do believe this is a serious design flaw, and makes the in-memory database much less useful in testing.

I'd also like to just point out that while I have no statistical data to back this up, I would imagine the vast majority of databases used by EF are relational in nature, and will likely continue to be. Why make an in-memory database for testing that doesn't support the "basics" that we see on a DbConnection/ DbCommand? How much more effort would it take to make a "relational" in-memory database where we don't have to deal with these headaches?

roji commented 5 years ago

That's simply not true. If I was writing the database or EF code, then I would be testing whether or not the rollback functionality worked, but if I'm writing my tests for code that invokes the rollback, I should be quite confident that the rollback behaved as expected, since that code isn't mine and has been tested by its author already. Why in the world would I test that?

You misunderstand me. I'm not saying you should be testing whether the EF actually executed the rollback, or that the database performed its duty. You should be testing how your application logic behaves around cases where you transaction was rolled back for some reason. For example, your application may start a transaction, but issue a rollback at some point when some exceptional condition occurs (related or unrelated to the database). It should then be capable of continuing successfully, possibly by restarting the same transaction and retrying. Fully testing this requires that your rollback actually be performed (uncommitted data should not be visible).

FWIW we express the limitations of the InMemory provider loud and clear in its doc page - it is not a relational database, and if you want to support relational-related logic (such as transactions) you should not be using it. In-memory Sqlite is a good alternative.

jholovacs commented 5 years ago

You should be testing how your application logic behaves around cases where you transaction was rolled back for some reason

Why would I not be able to test that? If I capture some exception or state where the logic is to roll back and then do something, that's still completely testable, regardless of whether or not the database actually rolled back. There's nothing preventing me from testing my code, except for the explosion that comes from invoking a rollback on an in-memory db...

roji commented 5 years ago

Why would I not be able to test that? If I capture some exception or state where the logic is to roll back and then do something, that's still completely testable, regardless of whether or not the database actually rolled back. There's nothing preventing me from testing my code, except for the explosion that comes from invoking a rollback on an in-memory db...

The point is that after you call the rollback function, the database should be rolled back - this is why an actual test of your application involving a rollback is not possible with InMemory.

jholovacs commented 5 years ago

I understand what you're saying, I'm just disagreeing that it's something that should be tested or even considered in any unit test. Yes, the database should be rolled back, and under runtime situations it would be; you issued the rollback command, of course it should be rolled back... but I don't care about that in the method under test. The important thing is that I issued the command, and performed some testable logic. If I want to do some data integrity testing, then it's time to dust off a transactional db system, but that's way out of scope for basic unit testing.

jjxtra commented 5 years ago

I changed my migrate try/catch to a check of the provider type. If it is in memory, I don't do the migration. All my transactions are now also wrapped in my own transaction class, which again checks for the in memory provider and if found, makes the transaction a no-op.

You could also consider using a sqlite file instead of inmemory db. This will slow down tests slightly but should give real-world results and behavior, but I have found it to not be necessary with the above tweaks. If you do use sqlite and other providers, you'll need to hand edit the migration code for any identity columns, for some reason the code first migration is not smart enough to see an identity key on a field and use it for all providers, it only adds the annotation for the specified provider.

Not perfect, but for me have found it to be good enough.

roji commented 5 years ago

I don't care about that in the method under test

You may not agree or care about it in whatever particular method you have in mind. But as I wrote above, rollback has very specific semantics which in many cases are part of what's being tested. Just like a test expects data to be there after a commit, it should expect for the data not to be there after rollback.

InMemory is simply not a relational database, nor can it masquerade as one while just ignoring relational semantics.

jholovacs commented 5 years ago

If you care about testing a rollback, then clearly using a database that does not support transactions to test is pointless... but how many people are using this to test for their SQL Server instance, or MySQL, or SQLite? It works fine for them for a long time, then there's one query that's running a little long, they tweak the timeout... and now their entire test suite crashes. IMO, this is not a good design.

roji commented 5 years ago

I've tried to explain our logic here, there doesn't seem to be anything more to say. If you're using relational features in your tests, we can either honor them where we can (relational database), or not run the test where we can't (InMemory). We can't start arbitrarily ignoring calls on the theory that most users care or don't care. If you want relational features, use a relational database.

ajcvickers commented 5 years ago

I don't want to re-hash what has already been said here, but I think it's worth emphasizing a few points.

First, we cannot force the in-memory provider to be used only for testing, and I have seen places where it is used for other purposes. Fundamentally, the in-memory provider follows the same patterns as any other provider.

This means that, as for any provider, it should provide clear feedback when it can't do what has been asked. Typically this means throwing an exception by default, we an opt-out if appropriate--just like for transactions. By default, there will be an exception saying that transactions are supported, but with a way to opt-in to no-op on transaction calls.

Second, it is not always easy to decide whether a feature is common to all providers, only used by relational providers, or entirely provider-specific. I think we could have chosen to make timeouts common rather than relational-only--that's valid feedback.

Also, it is challenging to make non-relational providers implement relational methods because this would require a dependency on the relational assembly, or at least a level of coupling that wouldn't be ideal. This is something I have been thinking about on and off for a while. One thing we could do is add a common feature for, for example, timeouts, with the relational implementation overriding anything set by the common feature. This is something I will discuss with the team.

Third, while the in-memory provider is marketed as "for testing" I think that is somewhat misleading. Its actual main value is as a simple back-end for all our internal non-provider-specific tests. When we started EF Core, it seemed like it would also be useful for testing applications. This can work with appropriately factored test code and a good understanding of where the provider behavior is different. However, I haven't recommended this for many years, and I don't think anyone else on the team would recommend it either. The main reason is that it's too easy to get caught out by differences in provider behavior, and therefore not realize that the behavior of your test is different from the behavior in production. This can be mitigated by using a provider that is closer to what is being used in production--for example, SQLite. However, the only way to know if something really works is to test with the same code that is running in production.