Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor method/member translation tests into their own suite #35319

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

roji
Copy link
Member

@roji roji commented Dec 11, 2024

This introduces a new BasicTypes model, and reorganizes many of our method/member translation tests into new test suites under Query/Translations.

  • The tests have a simple model containing all basic, universally supported data. This is better than Northwind (which has only a subset of the types we need, fixed data, and cannot be updated) and GearsOfWar (which is generally a mess, is relational-only because of the use of navigations, and requires updating many times because of the different duplications).
  • Tests are organized naturally based on the translation they exercise (e.g. TemporalTranslations, StringTranslations...) rather than the LINQ operator they're in (NorthwindWhereQueryTestBase, NorthwindSelectQueryTestBase). This makes it very easy to know where to find or put a test for a given translation.
  • This is non-exhaustive: there are still many translation tests in Northwind/GearsOfWar/elsewhere that should move into the new tests. Doing this fully is a pretty huge task, but this is big step in that direction.
  • Where we were testing both Where and Select, combined those into the same test (see Consider having AssertTranslation that tests both predicate and projection #35318).
  • Generally did lots of cleanup on all affected tests.
  • All tests have SQL assertions, for all providers.
  • All code has NRTs turned on.

Closes #34872

@roji roji requested a review from maumar December 11, 2024 15:35
@roji roji force-pushed the TranslationTests branch 2 times, most recently from 0015683 to 4d48e59 Compare December 11, 2024 19:57
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 49 out of 64 changed files in this pull request and generated 1 suggestion.

Files not reviewed (15)
  • test/EFCore.Specification.Tests/Query/NorthwindNavigationsQueryTestBase.cs: Evaluated as low risk
  • test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs: Evaluated as low risk
  • test/EFCore.Cosmos.FunctionalTests/Query/Translations/MiscellaneousTranslationsCosmosTest.cs: Evaluated as low risk
  • test/EFCore.Specification.Tests/Query/NorthwindSelectQueryTestBase.cs: Evaluated as low risk
  • test/EFCore.InMemory.FunctionalTests/Query/NorthwindWhereQueryInMemoryTest.cs: Evaluated as low risk
  • test/EFCore.InMemory.FunctionalTests/Query/Translations/BasicTypesQueryInMemoryFixture.cs: Evaluated as low risk
  • test/EFCore.Specification.Tests/Query/Translations/BasicTypesQueryFixtureBase.cs: Evaluated as low risk
  • test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs: Evaluated as low risk
  • test/EFCore.Relational.Specification.Tests/Query/NorthwindWhereQueryRelationalTestBase.cs: Evaluated as low risk
  • test/EFCore.Relational.Specification.Tests/Query/NorthwindFunctionsQueryRelationalTestBase.cs: Evaluated as low risk
  • test/EFCore.Specification.Tests/Query/NorthwindAggregateOperatorsQueryTestBase.cs: Evaluated as low risk
  • test/EFCore.InMemory.FunctionalTests/Query/NorthwindFunctionsQueryInMemoryTest.cs: Evaluated as low risk
  • test/EFCore.InMemory.FunctionalTests/Query/Translations/TemporalTranslationsInMemoryTest.cs: Evaluated as low risk
  • test/EFCore.InMemory.FunctionalTests/Query/Translations/MathTranslationsInMemoryTest.cs: Evaluated as low risk
  • test/EFCore.InMemory.FunctionalTests/Query/Translations/EnumTranslationsInMemoryTest.cs: Evaluated as low risk
Comments skipped due to low confidence (1)

test/EFCore.InMemory.FunctionalTests/Query/Translations/StringTranslationsInMemoryTest.cs:6

  • Syntax error in class declaration. It should be 'public class StringTranslationsInMemoryTest : StringTranslationsTestBase' instead of 'public class StringTranslationsInMemoryTest(BasicTypesQueryInMemoryFixture fixture) : StringTranslationsTestBase(fixture);'.
public class StringTranslationsInMemoryTest(BasicTypesQueryInMemoryFixture fixture) : StringTranslationsTestBase<BasicTypesQueryInMemoryFixture>(fixture);


public bool Bool { get; set; }
public Guid Guid { get; set; }
public required byte[] ByteArray { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Missing unsigned types, char, Half 😁, Uri, IPAddress, PhysicalAddress

Copy link
Member Author

@roji roji Dec 12, 2024

Choose a reason for hiding this comment

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

We can add these as needed no? Especially given we don't actually support any translations e.g. on IPAddress/PhysicalAddress? For PG which supports these, I was planning to add properties to the model (e.g. in a separate entity) and have extra translations test suites to exercise that (something like this already in EFCore.PG).

In other words, for now the types I've added were mostly types for which there are already existing tests that would exercise them - but I think it should be fine to add more types later.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AndriySvyryd I'm going to go ahead and merge this without the additional types, for now. I agree with you that at least some of these make sense, but let's add them later as needed (I've already spent enough time here...).

@roji roji enabled auto-merge (squash) December 13, 2024 08:09
@roji roji disabled auto-merge December 13, 2024 11:14
@roji roji merged commit c099cef into dotnet:main Dec 13, 2024
5 of 7 checks passed
@roji roji deleted the TranslationTests branch December 13, 2024 11:15
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.

Reorganize translation tests by declaring type, with a dedicated model
2 participants