search

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.73k stars 3.17k forks source link

Consider allowing non-shared (ad-hoc) tests within regular QueryTestBase classes #33526

Open roji opened 6 months ago

roji commented 6 months ago

We currently have two types of query test classes: classes which inherit from QueryTestBase, where a single model is shared across all tests, and non-shared (ad-hoc) tests, where each test creates its own model. This leads to a situation where the same "area" frequently has two test classes, e.g. JsonQuerySqlServerTest and AdHocJsonQuerySqlServerTest.

We could consider simply allowing non-shared tests inside regular QueryTestBase test classes; this would reduce the overall number of test classes, it would concentrate all the tests relating to a given area in a single class, we'd be able to put related tests next to each other, etc. Tests using non-shared models would automatically use a separate database (whose name could be derived from the shared-model one, e.g. by tacking on "NonShared").

One theoretical downside is that we would no longer parallelize pairs of shared and non-shared tests as we do now, but given the overall number of tests and classes we have, that seems to be really negligible.

/cc @maumar

maumar commented 6 months ago

I like having them separate. Test files are already large enough (e.g. gears of war is 8.5k lines of code), that we had to split some of the test suites. Also AssertQuery tests have a particular structure, and AdHoc tests have their own separate structure that is very different. We could normalize on the class names though to at least logically group tests classes thematically that way.

roji commented 6 months ago

Test files are already large enough (e.g. gears of war is 8.5k lines of code), that we had to split some of the test suites.

OK, but that seems like an orthogonal problem - it may indeed make sense to split big test classes (like gears) to multiple classes - ideally based on the functionality/area being tested; but splitting based on whether a one-time model is being used or not is weird...

Also AssertQuery tests have a particular structure, and AdHoc tests have their own separate structure that is very different.

Sure, but that's all contained within the test method, so it feels like an internal implementation detail... The high-order bit for me is for tests which test the same area to be grouped together, regardless of how they're implemented.

It just feels wrong (and messy) to do a query test base for some area, and then suddenly I need a one-time model for some specific thing, and I need to create a new class just for adhoc, and very related tests find themselves in separate files instead of in one place.

But of course no strong feelings here (we can also discuss next week).

maumar commented 6 months ago

Sounds good, I can probably be persuaded also. Let's talk in person and look at some code ;)

AndriySvyryd commented 5 months ago

While I agree that we should do a better job grouping tests by scenarios, I don't think that the current separation is detrimental. Both approaches follow different patterns and lifetimes, having a clear separation is beneficial when dealing with test failures.

roji commented 5 months ago

OK, will bring to a design discussion at some point - but am OK to close if you guys aren't convinced.