efcore icon indicating copy to clipboard operation
efcore copied to clipboard

Enable testing of compiled models in CI and on SQLite

Open ranma42 opened this issue 1 year ago • 6 comments

This changeset enables the testing of compiled models on SQLite regardless of the availability of spatial extensions.

ranma42 avatar Jun 28 '24 13:06 ranma42

This is an attempt at making the CI hit the same issue as https://github.com/dotnet/efcore/issues/22901#issuecomment-2194172006

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 avatar Jun 28 '24 13:06 ranma42

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

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

This holds true since https://github.com/dotnet/efcore/pull/25966

ranma42 avatar Jun 29 '24 00:06 ranma42

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.

ranma42 avatar Jun 29 '24 01:06 ranma42

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

AndriySvyryd avatar Jun 29 '24 02:06 AndriySvyryd

@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 40d7aa7f40844f5762d31b6ed117f1d2e86c1ca5 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 avatar Jun 29 '24 07:06 ranma42

The current state of the PR is that:

  • 40d7aa7f40844f5762d31b6ed117f1d2e86c1ca5 fails (see https://dev.azure.com/dnceng-public/public/_build/results?buildId=724842&view=results) as expected (aka the tests are run)
  • 4a928374540448e58dd5a5008a50ef63d0a542b1 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?)

ranma42 avatar Jun 29 '24 09:06 ranma42

@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.

AndriySvyryd avatar Jul 01 '24 21:07 AndriySvyryd

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

ranma42 avatar Jul 01 '24 22:07 ranma42

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

AndriySvyryd avatar Jul 01 '24 22:07 AndriySvyryd

Thanks for your contribution!

AndriySvyryd avatar Jul 02 '24 20:07 AndriySvyryd