Skip to content

Conversation

roji
Copy link
Member

@roji roji commented Jun 25, 2025

In preparation for complex type query work (#36296).

This is a 99.9% test-only change, reorganizing the recently-added relationship tests.

  • Reorganize tests by mapping type first (navigation/owned/owned json...) rather than by query area (projection/include/...). So we now have a directory for all navigation tests, another for all complex table splitting tests, etc.
  • Merged lots of test suites together; instead of 18 base, we now have only 5.
    • We previously split test classes for reference vs. collection relationships; merged these together.
      • The idea was to support table splitting, which only supports reference relationships (so would only run the reference tests).
      • However, I don't want all our classes to be split just because of this (lots of extra files/management).
      • Also importantly, once we have complex JSON support, the collections inside table-split complex types can simply be mapped to JSON (that's a nice scenario to cover too).
    • We also split test classes for tracking vs. no-tracking; merged these together as well.
      • We now pass the tracking behavior as a parameter to AssertQuery (added that capability).
      • All owned navigation tests need special treatment as tracking queries projecting an owned entity aren't allowed. If we allow projecting owned entities (of course without tracking them, #36297), all this can be removed.
      • Note that we want to cover tracking/no-tracking only for projection tests. All other tests shouldn't care about tracking behavior.
  • Moved things around and restructured to better match what's in core vs. relational.
  • Some trivial type renames in the model.

@roji roji requested a review from a team as a code owner June 25, 2025 22:54
@roji roji requested review from AndriySvyryd and cincuranet June 25, 2025 22:54
throw new InvalidOperationException(
RelationalStrings.JsonEntityOrCollectionProjectedAtRootLevelInTrackingQuery(
nameof(EntityFrameworkQueryableExtensions.AsNoTracking)));
throw new InvalidOperationException(CoreStrings.OwnedEntitiesCannotBeTrackedWithoutTheirOwner);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only product change in this PR; it uses the core error message instead of having a separate relational one that says largely the same thing.

@roji roji marked this pull request as draft June 25, 2025 23:44
@roji roji marked this pull request as ready for review June 26, 2025 09:40
@roji roji enabled auto-merge (squash) June 26, 2025 09:40
@roji roji force-pushed the RelationshipTests branch from cb6f179 to 0e79c56 Compare June 26, 2025 09:43
.IsRequired(false);

modelBuilder.Entity<RelationshipsRootEntity>()
// TODO: Why is this a navigation and not a complex property?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you found out the answer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope :) But I've already rewritten this most of this anyway as part of implement the complex type tests, so it's no longer going to be there.

// The .NET Foundation licenses this file to you under the MIT license.

// All tests in OwnedNavigationsProjectionSqliteTest currently fail because of #26708
// (Stop generating composite keys for owned collections on SQLite)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed like a good idea just for documentation purposes, i.e. we have all files for all mapping modes, but irrelevant/problematic ones just have a comment explaining what's going on. Let me know if this bothers you, I can remove.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is no code in this file there's a high chance that it will become outdated with the next refactoring. I think the comment in SqliteComplianceTest.cs is more than enough

@AndriySvyryd AndriySvyryd disabled auto-merge June 27, 2025 19:22
In preparation for complex type query work (dotnet#36296)
@roji roji force-pushed the RelationshipTests branch from 0e79c56 to 6530ee9 Compare July 7, 2025 11:11
@roji roji enabled auto-merge (squash) July 7, 2025 11:11
@roji roji merged commit 49e8ab7 into dotnet:main Jul 7, 2025
7 checks passed
@roji roji deleted the RelationshipTests branch July 7, 2025 12:04
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