Enable testing of compiled models in CI #34111

ranma42 · 2024-06-28T13:45:13Z

This changeset enables the testing of compiled models in CI.

ranma42 · 2024-06-28T13:46:40Z

This is an attempt at making the CI hit the same issue as #22901 (comment)

Locally I bisected the regression as follows:

$ git bisect log
# bad: [1847e1e9e0bde22a70a2c9388d9e9e36c6d9842d] Simplify and improve null semantics rewrites (#34072)
# good: [979e4496ccc7e3597725ad88bb0ad64eee4e8848] Fix build regression (#34078)
git bisect start 'origin/main' 'HEAD'
# bad: [5ac6eca3e708981c6ef2793b940e88b81f869d5b] FromSql work (#34065)
git bisect bad 5ac6eca3e708981c6ef2793b940e88b81f869d5b
# bad: [2295738e636ee6a44d50afd137500995cfcd675b] Update dependencies from https://github.com/dotnet/runtime build 20240623.2 (#34077)
git bisect bad 2295738e636ee6a44d50afd137500995cfcd675b
# good: [93c77f5583d7d471a2fd19e165da29b0dd798796] Cosmos: Fixes around array projection (#34061)
git bisect good 93c77f5583d7d471a2fd19e165da29b0dd798796
# first bad commit: [2295738e636ee6a44d50afd137500995cfcd675b] Update dependencies from https://github.com/dotnet/runtime build 20240623.2 (#34077)

ranma42 · 2024-06-28T16:52:56Z

test/EFCore.Specification.Tests/Scaffolding/CompiledModelTestBase.cs

@@ -1494,38 +1494,28 @@ protected virtual void AddDesignTimeServices(IServiceCollection services)
        if (string.IsNullOrEmpty(testDirectory)
            || !Directory.Exists(testDirectory))
        {
-            return;
+            throw new Exception("Test directory not found");


the tests in CI fail on this line, aka without this change they are not checking the baselines

ranma42 · 2024-06-29T00:03:06Z

The regression noticed by @ErikEJ and @cincuranet is apparently caused by a bad interaction between the introduction of the Uri.Equals(Uri) method in dotnet/runtime@94bb125 and
https://github.com/dotnet/efcore/blob/main/src/EFCore/ChangeTracking/ValueComparer%60.cs#L140-L176
Currently EFCore

prefers the T.Equals(T) method over op_Equality;
translates T.Equals(T) with null protection (opposed to op_Equality which is translated without null protection)

This holds true since #25966

ranma42 · 2024-06-29T01:37:30Z

This PR makes it possible to run the compiled model tests in CI and updates the baselines.
I will push a PR that simplifies the generated model in the next few days.

AndriySvyryd · 2024-06-29T02:06:38Z

We don't validate baselines in CI because it uses a different folder structure

ranma42 · 2024-06-29T07:55:32Z

@AndriySvyryd I see that in some cases the testDirectory is empty, but apparently in other cases ("efcore-ci (Build {Linux,Windows,macOS}") it can really run, at lease according to the results from https://dev.azure.com/dnceng-public/public/_build/results?buildId=724755&view=results

I pushed 40d7aa7 which does not contain the updated baselines (aka it should fail the CI checks) and later I will push the baseline updates on top to fix them.

In the meantime, I am marking this as draft, as I believe that some additional requirements/design discussion might be needed before considering this changeset ready 😇

ranma42 · 2024-06-29T09:31:16Z

The current state of the PR is that:

40d7aa7 fails (see https://dev.azure.com/dnceng-public/public/_build/results?buildId=724842&view=results) as expected (aka the tests are run)
4a92837 passes them (see https://dev.azure.com/dnceng-public/public/_build/results?buildId=724854&view=results and/or the checks on the PR) as desired (aka the new baselines match the output)

🚀

I am not 100% happy with the code that handles the mangling by DeterministicSourcePaths, but I believe it is very valuable to have at least some environment that automatically runs these tests, as it would avoid regressions such as the current one.

@AndriySvyryd what do you think about this approach? Does it make sense to move in this direction? (and/or do you have any suggestions on how to improve the PR?)

AndriySvyryd · 2024-07-01T20:56:47Z

test/EFCore.Specification.Tests/Scaffolding/CompiledModelTestBase.cs

+
+        foreach (var (path, ex) in exceptions)
+        {
+            throw new Exception($"Difference found in {path}", ex);


This should be an AggregateException and the top-level message should contain the number of files that are different.

AndriySvyryd · 2024-07-01T20:58:02Z

test/EFCore.Sqlite.FunctionalTests/Scaffolding/CompiledModelSqliteTest.cs

@@ -11,7 +11,6 @@

 namespace Microsoft.EntityFrameworkCore.Scaffolding;

-[SpatialiteRequired]


Why was this removed? The tests still use spatial types

AFAICT no test in this class uses Spatialite: the generated model would, but the test is only compiling it (in fact, if they required it, they would fail on the linux CI).

That is just a point in time, the corresponding asserts were commented out due to a bug in the runtime.

I was unable to find the commented-out asserts :(
Note that the tests successfully compile a model containing NTS Points

efcore/test/EFCore.Sqlite.FunctionalTests/Scaffolding/CompiledModelSqliteTest.cs

Lines 21 to 39 in 0302bb7

modelBuilder.Entity<Data>(

eb =>

{

eb.Property<int>("Id");

eb.HasKey("Id");

eb.Property<Point>("Point")

.HasSrid(1101);

});

modelBuilder.Entity<PrincipalBase>(

eb =>

{

eb.Property<Point>("Point")

.HasColumnType("geometry")

.HasDefaultValue(

NtsGeometryServices.Instance.CreateGeometryFactory(srid: 0).CreatePoint(new CoordinateZM(0, 0, 0, 0)))

.HasConversion<CastingConverter<Point, Point>, CustomValueComparer<Point>, CustomValueComparer<Point>>();

});

efcore/test/EFCore.Specification.Tests/Scaffolding/CompiledModelTestBase.cs

Line 35 in 0302bb7

// Blocked by dotnet/runtime/issues/89439

👍 I dropped 64a3fef and I will look into re-enabling these (in a separate PR).

I'll handle them. The fix is somewhat involved.

AndriySvyryd · 2024-07-01T21:04:30Z

@AndriySvyryd what do you think about this approach? Does it make sense to move in this direction? (and/or do you have any suggestions on how to improve the PR?)

It's brittle, but I am not strongly opposed to it.

An alternative would be to copy the baselines to the output folder and use different paths for reading and writing.

ranma42 · 2024-07-01T22:43:19Z

I updated the code to throw an AggregateException that combines all of the exceptions.

Regarding changing the reading/writing of the baselines, I am afraid it would be a more invasive change.
IIUC it would affect the DX (EF_TEST_REWRITE_BASELINES=1 would not be sufficient to refresh the baselines and/or compare them to the current committed version) and it would require some changes to the build system (I did not check how complex they would be... my main concern when touching that would be to understand what happens in the various build environments).
If that is the best way to do that, I can try (but I might need some guidance 😇 ).

AndriySvyryd · 2024-07-01T22:56:57Z

Regarding changing the reading/writing of the baselines, I am afraid it would be a more invasive change.

I agree, that's why I think that the current approach is good enough

Instead of considering the test passed if no check was done, mark it as failed. Additionally, handle paths mangled by DeterministicSourcePaths.

Instead of throwing the first exception, throw an aggregation of all of them.

AndriySvyryd · 2024-07-02T20:44:17Z

Thanks for your contribution!

ranma42 commented Jun 28, 2024

View reviewed changes

ranma42 force-pushed the test-sqlite-compiled-model branch from e75a48f to c233064 Compare June 28, 2024 20:59

ranma42 changed the title ~~Enable testing of compiled models on SQLite~~ Enable testing of compiled models on SQLite & in CI Jun 28, 2024

ranma42 changed the title ~~Enable testing of compiled models on SQLite & in CI~~ Enable testing of compiled models in CI and on SQLite Jun 28, 2024

ranma42 force-pushed the test-sqlite-compiled-model branch from c233064 to 3170315 Compare June 29, 2024 01:13

ranma42 marked this pull request as ready for review June 29, 2024 01:36

ranma42 force-pushed the test-sqlite-compiled-model branch from 3170315 to 594b426 Compare June 29, 2024 05:55

Mark tests as skipped instead of replacing with no-op

95f5917

ranma42 force-pushed the test-sqlite-compiled-model branch from 594b426 to 40d7aa7 Compare June 29, 2024 07:53

ranma42 marked this pull request as draft June 29, 2024 07:55

ranma42 marked this pull request as ready for review June 30, 2024 18:12

AndriySvyryd reviewed Jul 1, 2024

View reviewed changes

ranma42 added 3 commits July 2, 2024 10:01

Avoid most silent failures in CompiledModelTestBase.AssertBaseline

b1c3bd8

Instead of considering the test passed if no check was done, mark it as failed. Additionally, handle paths mangled by DeterministicSourcePaths.

Update baselines

6b67e70

Aggregate all of the exceptions

57ecfca

Instead of throwing the first exception, throw an aggregation of all of them.

ranma42 force-pushed the test-sqlite-compiled-model branch from bdce444 to 57ecfca Compare July 2, 2024 08:01

ranma42 changed the title ~~Enable testing of compiled models in CI and on SQLite~~ Enable testing of compiled models in CI Jul 2, 2024

This was referenced Jul 2, 2024

Compiled model tests failing on Windows #34137

Closed

All PRs were merged on green, and yet the build was still broken #34139

Closed

AndriySvyryd approved these changes Jul 2, 2024

View reviewed changes

AndriySvyryd merged commit 3a49a80 into dotnet:main Jul 2, 2024
7 checks passed

AndriySvyryd added the community-contribution label Jul 2, 2024

AndriySvyryd mentioned this pull request Jul 2, 2024

Fix build break #34138

Closed

ranma42 deleted the test-sqlite-compiled-model branch July 2, 2024 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable testing of compiled models in CI #34111

Enable testing of compiled models in CI #34111

ranma42 commented Jun 28, 2024 •

edited

Loading

ranma42 commented Jun 28, 2024

ranma42 Jun 28, 2024

ranma42 commented Jun 29, 2024 •

edited

Loading

ranma42 commented Jun 29, 2024

AndriySvyryd commented Jun 29, 2024

ranma42 commented Jun 29, 2024

ranma42 commented Jun 29, 2024

AndriySvyryd Jul 1, 2024

AndriySvyryd Jul 1, 2024

ranma42 Jul 1, 2024 •

edited

Loading

AndriySvyryd Jul 1, 2024

ranma42 Jul 2, 2024

AndriySvyryd Jul 2, 2024

ranma42 Jul 2, 2024

AndriySvyryd Jul 2, 2024 •

edited

Loading

AndriySvyryd commented Jul 1, 2024

ranma42 commented Jul 1, 2024

AndriySvyryd commented Jul 1, 2024

AndriySvyryd commented Jul 2, 2024

		@@ -11,7 +11,6 @@

		namespace Microsoft.EntityFrameworkCore.Scaffolding;

		[SpatialiteRequired]

	modelBuilder.Entity<Data>(
	eb =>
	{
	eb.Property<int>("Id");
	eb.HasKey("Id");

	eb.Property<Point>("Point")
	.HasSrid(1101);
	});

	modelBuilder.Entity<PrincipalBase>(
	eb =>
	{
	eb.Property<Point>("Point")
	.HasColumnType("geometry")
	.HasDefaultValue(
	NtsGeometryServices.Instance.CreateGeometryFactory(srid: 0).CreatePoint(new CoordinateZM(0, 0, 0, 0)))
	.HasConversion<CastingConverter<Point, Point>, CustomValueComparer<Point>, CustomValueComparer<Point>>();
	});

Enable testing of compiled models in CI #34111

Enable testing of compiled models in CI #34111

Conversation

ranma42 commented Jun 28, 2024 • edited Loading

ranma42 commented Jun 28, 2024

ranma42 Jun 28, 2024

Choose a reason for hiding this comment

ranma42 commented Jun 29, 2024 • edited Loading

ranma42 commented Jun 29, 2024

AndriySvyryd commented Jun 29, 2024

ranma42 commented Jun 29, 2024

ranma42 commented Jun 29, 2024

AndriySvyryd Jul 1, 2024

Choose a reason for hiding this comment

AndriySvyryd Jul 1, 2024

Choose a reason for hiding this comment

ranma42 Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

AndriySvyryd Jul 1, 2024

Choose a reason for hiding this comment

ranma42 Jul 2, 2024

Choose a reason for hiding this comment

AndriySvyryd Jul 2, 2024

Choose a reason for hiding this comment

ranma42 Jul 2, 2024

Choose a reason for hiding this comment

AndriySvyryd Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

AndriySvyryd commented Jul 1, 2024

ranma42 commented Jul 1, 2024

AndriySvyryd commented Jul 1, 2024

AndriySvyryd commented Jul 2, 2024

ranma42 commented Jun 28, 2024 •

edited

Loading

ranma42 commented Jun 29, 2024 •

edited

Loading

ranma42 Jul 1, 2024 •

edited

Loading

AndriySvyryd Jul 2, 2024 •

edited

Loading