Navigation test refactoring - improving structure for scenarios that don't support collections

[maumar]

Plus minor cleanup

Fixes #35707

[maumar]

@roji alternative approach, which splits the tests. It's still quite big change, but I guess the work is done so we can go ahead with it. Projection tests are the worst wrt class explosion because of tracking & no tracking/ntwir dimension (as we generate different shapers so my thinking is we should test both), maybe it wont be so bad going forward.

[roji]

Thanks @maumar, here are some comments. I didn't get around to properly reviewing the first PR, so the below also contains some general review notes which are unrelated to the collection/reference split in this PR. You don't have to deal with those in this PR specifically, though.

Understood about the explosion for projection tests - makes sense. I agree that we do want to keep projection tests separate from non-projection tests, because of the various shaper behavioral differences (tracking)... I think that's OK. And in any case, as this effort evolves we'll see where we end up and maybe improve things further.

• Shouldn't JsonRelationshipsInProjectionNoTrackingQueryCosmosTest extend JsonRelationshipsInProjectionNoTrackingQueryTestBase instead of JsonRelationshipsInProjectionQueryTestBase?

• On a related note, it seems that the no-tracking setup (via TrackingRewriter and RewriteServerQueryExpression) happens in the provider implementation, but it seems like it should happen once in the base class, rather than for each provider.

  • On a related note, why do we need to rewrite the query tree and mess around with AsNoTracking/AsTracking in this way? Why not just set the global default on the context via the fixture?

• Since the explosion is causing the names to get long, I propose simplifying the naming a bit:

  • We can just use Projection instead of InProjection everywhere to refer to the projection tests.
  • I don't think we need Relationships everywhere, since there should be a more concrete relationship type in its place (Navigation, OwnedJson, ComplexJson...). So I think the names stay pretty unambiguous if we drop Relationships in most places.
  • I personally never quite got why we insist on having Query in all the type names - it's not like we need to disambiguate with other non-query tests that have the same name. So I'd drop that too.

• First, the tests don't currently distinguish between owned with table splitting and owned without (separate table). I think that means that e.g. OwnedReferenceRelationshipsInProjectionQuerySqlServerTest exercises table splitting (because it's reference navigations and table splitting is the default), whereas OwnedRelationshipsInProjectionQuerySqlServerTest tests separate-table owned (because it's collection navigations and only separate-table is supported). If that's the case, then we're currently missing coverage for separate-table reference owned navigations.

• I'd definitely group together all the projection tests in the same directory - both Reference and non-Reference.

  • Importantly, the reference/collection split simply won't be relevant for many/most of the features exercised. For example, any feature that's relevant only for foreign-key navigations (like Include) doesn't need that split at all - it's only needed for stuff we need to exercise with table splitting, basically (since that's the only place where collection isn't supported). So I'd revert the split of the Include tests.
  • There aren't that many InProjection test suites (7 total), and the feature exercised (InProjection) is more important/fundamental than the type of relationship (reference/navigation).
  • So at the top-level I'd have Include and Projection, and inside Projection I'd just have all the test suites, Reference and non-Reference.

• EntityRelationshipsQueryFixtureBase (and related types) seems to refer to regular, non-owned foreign-key-based navigations; if that's so, should we call it NavigationRelationshipsQueryFixtureBase, since navigations is the proper name for this in EF?

• JsonRelationshipsQueryFixtureBase seems to refer to JSON owned entities, but we'll soon have complex types - so the naming should reflect that (e.g. OwnedJsonRelationshipsQueryFixtureBase, vs ComplexJsonRelationshipsQueryFixtureBase)

• Nit: I think we should rename tests starting with Project_ to Select_ - the naming should simply convey the LINQ operator being used. For example, SelectMany is another way to project, and tests doing that start with SelectMany_ which makes it super clear what they do (whereas Project_ is ambiguous).

[maumar]

@roji I updated the PR. Wrt using expression rewrite for NoTracking, rather than global option in change tracker - that would cause explosion of fixtures no? - would need a no tracking fixture at the core, relational and provider levels for each type of relationship. We can do it to make tests bit quicker (save the expression visit) but kept it as was for now.

[roji]

Thanks, that's starting to look really good/clean now, I think.

• We could make the naming even shorter by removing the Base suffix everywhere - since the name lacks a specific provider, it's already clear that it's abstract. Abstract base classes don't really have to have this suffix in .NET (there are many counter-examples). We can discuss with @AndriySvyryd though.
• Naming nit: I'd move NoTracking before Projection so OwnedJsonReferenceNoTrackingProjectionTestBase instead of OwnedJsonReferenceProjectionNoTrackingTestBase (these are projection tests (in the Projection directory), no-tracking only modifies them just like Reference or Json modifies them).
• Just in case you're interested, I did things this way in the M.E.VectorData test suite (link - just keeping things as simple as possible, keeping inner fixture types in the test base classes, minimal naming without any unneeded stuff (like Base), etc.

Wrt using expression rewrite for NoTracking, rather than global option in change tracker - that would cause explosion of fixtures no?

It would, yeah. Though if we just have an inner Fixture class in each of the test base classes which extends the current (shared) one:

public abstract class ComplexRelationshipsFixtureBase : RelationshipsQueryFixtureBase;

public abstract class ComplexProjectionNoTrackingTestBase<TFixture>(TFixture fixture)
    : ComplexProjectionTestBase<TFixture>(fixture)
        where TFixture : ComplexProjectionNoTrackingTestBase<TFixture>.Fixture, new()
{
    public new abstract class Fixture : ComplexRelationshipsFixtureBase
    {
        public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
            => base.AddOptions(builder).UseQueryTrackingBehavior(QueryTrackingBehavior.NoTracking);
    }
}

This way, each test base class by convention has an inner Fixture type which it accepts, and which does any customizations necessary, leading to a fixture-per-type scheme. There's still technically "explosion" if we count the number of types, but given that they're clearly organized and nested inside the relevant classes, it seems pretty OK to me.

What do you think?

[maumar]

@roji I made the change to NoTracking tests, the Base suffix removal I will do in another pr (we should probably do it for all the tests if we decide to go with this approach). Wrt Fixtures, lets discuss in person tomorrow. I like the idea of Fixtures inside test classes to reduce the file explosion. I'm still not sure how to factor things correctly when it comes to tracking. If we fork the fixtures on core level, we would then have, say, OwnedFixture OwnedNoTrackingFixture, then OwnedRelationalFixture, OwnedNoTrackingRelationalFixture, and same for sql server and sqlite. Problem is that relational fixtures can have significant logic in them (in on model creating) - that logic would have to be duplicated in the OwnedNoTrackingRelationalFixture, no?

[roji]

that logic would have to be duplicated in the OwnedNoTrackingRelationalFixture, no?

That's a good point - it's where .NET's single inheritance hits some limits. We could have the relational config logic sit in the main (tracking) relational fixture - in some method - and then call that method from the non-tracking relational fixture; that would allow reusing relational config while maintaining a tracking/no-tracking split at the fixture core level.