dotnet / EntityFramework.Docs

Documentation for Entity Framework Core and Entity Framework 6
https://docs.microsoft.com/ef/
Creative Commons Attribution 4.0 International
1.62k stars 1.96k forks source link

Tweak wording on repository pattern #4303

Open bb-avi opened 1 year ago

bb-avi commented 1 year ago
I have often used the Repository pattern and your comparison of the Repository pattern to other testing strategies is a bit misleading. Feature Repository Pattern Should Be Reason
Test double type Stub/mock Stub/mock This is fine - but this affects all other items in the list
Raw SQL? Yes No No DB query is being executed by the Stub/mock so how is raw SQL being tested?
Transactions? Yes No No DB is attached to the Stub/mock so how are transactions being tested?
Provider-specific translations? Yes No No Provider is used by the Stub/mock so how are these being tested?
Exact query behavior? Yes No No DB query is being executed by the Stub/mock so how is exact query behavior being tested?
Can use LINQ anywhere in the application? No No This is correct

As much as I like the repository pattern, it seems misplaced in this article. It doesn't test any of the listed features. As stated in the description of the Repository Pattern above:

...tests against the real database are still likely to be needed for the queries exposed by the repository.

But it's not just queries that will need tested - - all the features listed will still need tested using one of the other strategies.

I suggest removing the Repository Pattern from this article.


Document Details

Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

roji commented 1 year ago

The point here, is that code where the real (non-test) data layer uses raw SQL/transactions/whatever can be tested when using a repository; this is because the raw SQL/transactions is wrapped inside the repository and thus abstracted away, and when testing the repository implementation just returns whatever results the tester wants, in standard mock/stub fashion. This is in contrast with e.g. using the InMemory provider, where code using raw SQL/transactions simply cannot be tested at all.

In other words, assuming the user is looking for a unit testing strategy, this by definition doesn't include the database (at least not the system used in production); that generally means that no raw SQL/transactions are actually being used. But a repository still allows testing user code that uses those features when running in production.

bb-avi commented 1 year ago

Thank you for your quick response.

From the article:

The following table provides a quick, comparative view of the different testing techniques, and shows which functionality can be tested under which approach:

It seems it is saying what can be "tested" by each approach.

Raw SQL: The Repository pattern does not test Raw SQL it mocks it. If we compare this to mocking/stubbing the DbSet and DbContext (have it implement an interface, mock the interface, and provide DbSet stubs), we can do the same thing you mentioned with the Repository Pattern for Raw SQL: “return whatever results the tester wants, in standard mock/stub fashion.” Yet “Mock DbContext” has a “No” next to it. Both the SQLite-in-memory and a real/production database testing are the only ones that really "test" this.

Provider-specific translation: The Repository Pattern doesn’t test any provider-specific translation - it mocks it. Only a test against a real database can do this. A mock of the DbContext can again do the same thing the Repository pattern does, return whatever the tester wants.

Exact query behavior: The Repository Pattern doesn’t test any query behavior - it mocks it. All the other strategies test query behavior more thoroughly than using the repository pattern.

Since using the repository pattern doesn’t "test" raw SQL, transactions, translations, or queries - it mocks them, then the table should be changed to reflect this. When using the Repository pattern I would recommend still using one of the other testing strategies to more thoroughly test Raw SQL, transactions, provider-specific translations, and exact query behavior.

As much as I enjoy using the Repository pattern I think this over-states its usefulness.

roji commented 1 year ago

Nobody here is saying that the raw SQL capability itself can be tested - that would mean testing that your database product work - not your application - and makes no sense. What is said is that code which invokes raw SQL in production can be tested; this is what it means to mock/stub (we may want to do some minor edits to make sure that point is clear). This simply isn't true for techniques (see below).

If we compare this to mocking/stubbing the DbSet and DbContext (have it implement an interface, mock the interface, and provide DbSet stubs), we can do the same thing you mentioned with the Repository Pattern for Raw SQL [...] A mock of the DbContext can again do the same thing the Repository pattern does, return whatever the tester wants.

This is where you're probably missing the main point - it's not really possible (in any case, not feasible) to mock DbSet for query purposes, since LINQ differs from most APIs. With most APIs, there's a method on some type that your code calls (e.g. DownloadFile), and mocking/stubbing simply involves swapping out that specific method. With LINQ, you use static extension methods to compose complex query nodes on top of the DbSet, and finally use another standard static method to trigger the query's execution (e.g. ToList).

But you unfortunately cannot mock static extension methods (Where, ToList), so just directly mocking the query API doesn't work like, say, mocking a DownloadFile method. Therefore, what is usually meant by mocking/stubbing here is swapping out the DbSet itself - only the root of the query - for e.g. an in-memory/enumerable implementation. But the static methods (Where, ToList) composed on top are still the same un-mockable methods; and if your Where lambda invokes e.g. a provider-specific method, that method simply throws an exception since it can't be evaluated client-side - only translated to SQL.

Because of unique characteristic of LINQ - query operators are composed on top of a query root via un-mockable static extension methods - the only way to employ mocking/stubbing in a robust way is to hide the entire query behind a traditional method/API (like DownloadFile), and mock/stub that method. That's what the repository pattern does.

If I'm misunderstanding your point, please submit a minimal code sample which shows how you can mock a DbSet and call a provider-specific method in your query.

bb-avi commented 1 year ago

The Repository Pattern is a good pattern and I don't want to discourage anyone from using it. But I don't want anyone to use it with the wrong expectations.

Simple Example for Raw SQL with DbContext mocking:

public interface IDbContext {
    void ClearHistory();
}
internal class DbContext : IDbContext 
{
  public void ClearHistory() => Database.ExecuteSqlRaw("delete from History");
}

When testing other classes that use the IDbContext it will test Raw SQL as much as testing other classes that use a repository in front of the DbContext. (Which again in my view neither strategy tests Raw SQL - they mock/hide it).

Showing how mocking the IDbContext could affect other features would take quite a bit more code.

But that is not my point. My point is that none of these features are tested by the Repository pattern - they are mocked/hidden by it. Can we agree that the ability to mock something is not the same as testing something?

In short,

What I think is missing from this table is: "General LinQ Query Testing" - - which all the other approaches do (with caveats) - but the Repository Pattern does not.

roji commented 1 year ago
internal class DbContext : IDbContext 
{
  public void ClearHistory() => Database.ExecuteSqlRaw("delete from History");
}

This is essentially the repository pattern, but with the repository merged into DbContext instead of having an additional class wrapping your DbContext. The main point is that application code no longer executes queries directly on DbContext, but rather invokes methods which encapsulates those queries. This allows mocking/stubbing those methods (which allows proper unit testing of code which uses them), but is also the source of the architectural burden: you can no longer just do LINQ wherever you want, but always have to wrap it in (mockable) methods.

But that is not my point. My point is that none of these features are tested by the Repository pattern - they are mocked/hidden by it. Can we agree that the ability to mock something is not the same as testing something?

As I wrote above:

Nobody here is saying that the raw SQL capability itself can be tested - that would mean testing that your database product work - not your application - and makes no sense. What is said is that code which invokes raw SQL in production can be tested; this is what it means to mock/stub (we may want to do some minor edits to make sure that point is clear).

If your point here is that the wording should be tweaked to make it clearer that e.g. raw SQL isn't being tested, but rather code which uses raw SQL, then sure, we could see about making that clearer. Or are you trying to say something beyond that?

bb-avi commented 1 year ago

Maybe it is just the wording - - but there are strategies that actually test each feature. And when you put the aforementioned phrase over the table, then it sure sounds like you're saying which strategies can test each feature. Maybe a better section would be the following:

The following table provides a quick, comparative view of the different testing strategies, and shows which functionality can be mocked or tested for each strategy:

Feature In-memory SQLite in-memory Mock DbContext Repository Pattern Testing against the database
Test double type Fake Fake Fake Mock/Stub Real - no double
Raw SQL? None Depends None Mocked Tested
Transactions? None (ignored) Tested None Mocked Tested
Provider-specific translations? None None None Mocked Tested
General query behavior? Tested Tested Tested Mocked Tested
Exact query behavior? Depends Depends Depends Mocked Tested
Can use LINQ anywhere in the application? Yes Yes Yes No* Yes