Skip to content
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

Removed generic type constraint in extension methods #62

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tiwahu
Copy link

@tiwahu tiwahu commented Aug 24, 2022

PR Details

Description

  • Allows TEntity to be a value type.

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

  • All existing tests passed.
  • Locally used the code called by the extension method and it worked with a long for TEntity.

Checklist

  • My code follows the code style of this project.
  • I have added tests to cover my changes.

    All tests passed. Hopefully covered by existing test suite...but I'll check that! Maybe we should make a value type test. ;)

  • All new and existing tests passed.

- Allows TEntity to be a value type.
@romantitov
Copy link
Owner

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
Copy link
Author

tiwahu commented May 15, 2024

@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
Copy link
Owner

@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
Copy link
Author

tiwahu commented Aug 21, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants