romantitov / MockQueryable

Mocking Entity Framework Core operations such ToListAsync, FirstOrDefaultAsync etc
MIT License
785 stars 76 forks source link

Removed generic type constraint in extension methods #62

Open tiwahu opened 2 years ago

tiwahu commented 2 years ago

PR Details

Description

The extension methods had a generic type constraint that prevented IQueryable<T> where T is a value type from being mocked.

I wasn't sure if the reference type constraint was intentional, an oversight, or leftover from older code. I successfully used the following locally to work around it and figured other could benefit:

var data = new TestAsyncEnumerableEfCore<long>(new[] { 42L });

Related Issue

How Has This Been Tested

Checklist

romantitov commented 9 months ago

hello @tiwahu sorry for the late answer. Thanks for your contribution. Could you provide a bit more explanation, what kind of problem you are trying to solve by using long as an entity? Could you please also add to the PR test cases for your specific cases with value types and I will be happy to include your changes to the next release.

tiwahu commented 5 months ago

@romantitov, sorry for such a late reply! This library is pretty awesome and simplified tests where we were starting to do it ourselves.

At some point we encountered a scenario where the TEntity, in an IQueryable<TEntity> that we wanted to mock and use async methods, was a value type. In our case, it was a long, but it could have been any value type (e.g., a database query that just returned a column of 64-bit integers).

I did something manual to work around going through the extension method, which has the complier restriction. The TestAsyncEnumerableEfCore<T> doesn't have the same class restriction for T, so it seemed like it would work, and it did. Is there a reason for the class restriction in the extension methods?

romantitov commented 2 months ago

@tiwahu sorry for the late answer.

Thank you for your feedback. I’m really glad to hear that you find MockQueryable to be a convenient tool.

I carefully reviewed your comments, but the specific use case you described is still a bit unclear to me.

The TEntity type parameter is constrained to class because, by definition, an entity cannot be a value type (you can refer to the concepts of Identifier Equality vs. Structural Equality for further clarification).

The scenario you mentioned, where a database query returns specific fields of entities, is already supported by the current implementation. For an example, you can take a look at the GetUserReports test. Although UserReport is implemented as a class in this example, you can change it to a struct, and the test will still work as expected. This demonstrates that there are no restrictions on the return type in such cases.

Since an entity represents records in a database, I’m having difficulty imagining a scenario where an entity could be of type long, as mentioned in your initial comment. If you believe there is a valid case where TEntity could be long (or any other value type), could you please provide a more detailed explanation of your use case, along with relevant test cases?

tiwahu commented 2 months ago

@romantitov a little sooner this time. 😉

Looking at my old notes (i.e., comments in code), I was able to work around the extension method constraint by instantiating a TestAsyncEnumerableEfCore<T> directly; however, using the extension method would be so much nicer.

The TEntity type parameter is constrained to class because, by definition, an entity cannot be a value type (you can refer to the concepts of Identifier Equality vs. Structural Equality for further clarification).

Of course, most of the time in apps using this library, the T in an IQueryable<T> is a TEntity, which represents a database record (or something like it), so a class constraint makes sense when the type must be a reference type. The extension method worked fine for those out of the box.

Here's the cool part...T can be a value type in IQueryable<T>. In this case, I just wanted to mock an IQueryable<long>, since T can be anything because it has no constraint.

Imagine database queries that only need values out of a single column of a table after applying some joins, where clauses, and sorting. For example, code doing a search for a paginated batch of ids, mapping to records, that some "other" code uses for some other purpose. The LINQ selects for those queries return an IQueryable<long> by simply selecting a property out of the TEntity with the 64-bit values it needs. No need for a complex object with a single property. No need for reference type elements when an IQueryable<T> of a value type will do just fine. It's faster! 🏎️

Test cases for that "other" code wanted to mock the results that it would get from async methods that returned IQueryable<long>. Fortunately, doing things like this worked:

            var ids = new TestAsyncEnumerableEfCore<long>(new[] { 4L, 8L, 15L, 16L, 23L, 42L });

That it isn't nearly as elegant or obvious as your excellent extension methods, though.

It seems like the extension methods constraints are arbitrary since the code works without it. I'm proposing the idea of removing the constraint. I can't think of a reason for it to be there, since any T can be mocked, instead of only a class-constrained TEntity.

It's been a while since I looked at this. Hope that makes sense.


Of course, I should figure out where to put the test cases in here for value types specifically! Though, the fact that the projects compile (or did back when I did it) without the class constraint is a pretty good test case.