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.

[maumar]

should we put all the classes in the common directory with other relationship tests instead? (rather than separate one for References)

[roji]

Yeah, I definitely think so - see general review comments on why.

[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).

[roji]

Yeah, I definitely think so - see general review comments on why.