Enable testing of compiled models in CI and on SQLite
This changeset enables the testing of compiled models on SQLite regardless of the availability of spatial extensions.
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)
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
- prefers the
T.Equals(T)method overop_Equality; - translates
T.Equals(T)with null protection (opposed toop_Equalitywhich is translated without null protection)
This holds true since https://github.com/dotnet/efcore/pull/25966
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.
We don't validate baselines in CI because it uses a different folder structure
@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 😇
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?)
@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.
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 😇 ).
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
Thanks for your contribution!