Allow set operations over other mapping types #16813
Conversation
|
@roji Remember to re-target to release/3.0 before merging. Marking as blocked for now as a reminder... |
|
@ajcvickers done |
roji
force-pushed
the
MoreFlexibleSetOperations
branch
from
1956bd4
to
2c243df
Compare
test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs
Show resolved
Hide resolved
roji
force-pushed
the
MoreFlexibleSetOperations
branch
2 times, most recently
from
cbe4ac9
to
b6930c4
Compare
test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.SetOperations.cs
Show resolved
Hide resolved
test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs
Outdated
Show resolved
Hide resolved
Including different mapping types on the two sides, as long as the result is compatible. Fixes #16725
roji
force-pushed
the
MoreFlexibleSetOperations
branch
from
b6930c4
to
e85773b
Compare
| } | ||
|
|
||
| public override async Task Union_over_different_projection_types(bool isAsync, string leftType, string rightType) | ||
| { |
There was a problem hiding this comment.
Asserting Sqls dynamically like this is nightmare to maintain test.
So we are down to 2 choice. Either split out each into different tests (sync/async can be together like other tests)
or run loops to generate queries to execute one by one. Essentially removing leftType/rightType from parameter of the test.
Like Convert_ToDecimal test
There was a problem hiding this comment.
Would have been better to receive this previously as I was asking for feedback.
To be sure I understand, you're saying it's more maintainable for us to have 72 manually-written SQL assertions? For comparison, Convert_ToDecimal has 8.
If you're not happy with the way the test looks, let me propose something. All of these test cases/assertions don't actually correspond to different code paths inside - the actual set operation logic doesn't care one bit if one side is a constant, a column, or a function call - as long as they return the same thing. In other words, coverage isn't actually increased. Maybe a single test per expression type (on both sides), plus one extra for different expression types could be sufficient.
There was a problem hiding this comment.
I think we should go with the tests as @roji has written them for now. We can file an issue to discuss as a team whether we in general like this pattern or not, and then follow up appropriately.
ghost
deleted the
Including different mapping types on the two sides, as long as the result is compatible.
Fixes #16725