Default to using SQL Server testcontainer in tests#37809
Default to using SQL Server testcontainer in tests#37809roji wants to merge 1 commit intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates EF Core’s SQL Server test infrastructure to no longer require a preconfigured SQL Server connection on non-Windows machines by defaulting to a SQL Server Testcontainers-based setup (while keeping Windows defaulting to LocalDB).
Changes:
- Removed non-Windows SQL Server test skipping (
<SkipTests ...>) across multiple SQL Server-dependent test projects. - Changed SQL Server test configuration to have no default connection string in
config.json, delegating defaults to code. - Added
Testcontainers.MsSqland updatedTestEnvironmentto start a SQL Server container on non-Windows when no connection string is configured.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.VisualBasic.FunctionalTests/EFCore.VisualBasic.FunctionalTests.vbproj | Removes non-Windows skip condition so tests can run using the new default container behavior. |
| test/EFCore.SqlServer.HierarchyId.Tests/EFCore.SqlServer.HierarchyId.Tests.csproj | Removes non-Windows skip condition to allow running via container-based defaults. |
| test/EFCore.SqlServer.FunctionalTests/config.json | Sets DefaultConnection to null so defaults come from TestEnvironment. |
| test/EFCore.SqlServer.FunctionalTests/TestUtilities/TestEnvironment.cs | Adds logic to use LocalDB on Windows, otherwise start a SQL Server testcontainer when no configured connection exists. |
| test/EFCore.SqlServer.FunctionalTests/EFCore.SqlServer.FunctionalTests.csproj | Removes skip condition and adds Testcontainers.MsSql package reference. |
| test/EFCore.OData.FunctionalTests/EFCore.OData.FunctionalTests.csproj | Removes non-Windows skip condition to allow running via container-based defaults. |
| test/EFCore.AspNet.SqlServer.FunctionalTests/EFCore.AspNet.SqlServer.FunctionalTests.csproj | Removes non-Windows skip condition to allow running via container-based defaults. |
| test/Directory.Packages.props | Adds centralized package version for Testcontainers.MsSql. |
Comments suppressed due to low confidence (1)
test/EFCore.SqlServer.FunctionalTests/TestUtilities/TestEnvironment.cs:63
DefaultConnectionis wrapped inLazy, but_dataSourceis initialized eagerly vianew SqlConnectionStringBuilder(DefaultConnection)during type initialization. This defeats the laziness and will start the container as soon as anyTestEnvironmentmember is accessed (e.g.IsCIinSqlServerConditionAttribute), even when no database is actually needed. Consider making_dataSource(and any other derived values) lazy as well, so container startup only happens when a DB-dependent member is accessed.
private static readonly string _dataSource = new SqlConnectionStringBuilder(DefaultConnection).DataSource;
public static bool IsConfigured { get; } = !string.IsNullOrEmpty(_dataSource);
| _container = new MsSqlBuilder("mcr.microsoft.com/mssql/server:2025-latest") | ||
| .Build(); | ||
|
|
||
| _container.StartAsync().GetAwaiter().GetResult(); | ||
|
|
There was a problem hiding this comment.
If _container.StartAsync() throws (e.g. Docker not available, image pull failure), the container instance is left undisposed because the ProcessExit handler is registered only after the start completes. Wrap startup in a try/catch/finally (disposing the container on failure) and/or register cleanup before starting to avoid leaving orphaned containers behind.
|
@roji Only issue could be resource constraints on your Windows agent? and unsure about in-memory tables tests? |
Yep, SQL Server is unfortunately quite big, so if we start spinning up lots of these, that won't end well. However, this is just a fall-back in case there's no environment variable/config.json, so you can always override it. In other words, this PR as it is doesn't change current behavior, it only defaults to testcontainers where we previously would have failed because there's no SQL Server. I do think we should also consider extending this to Windows, rather than default to LocalDB (again for isolation with multiple agents etc.) - see comment above.
And full-text search which also isn't there. But again, this PR doesn't make things any worse... If anything, we can further evolve this to maybe have the testcontainer have these features. |
|
We shouldn't do this by default on Windows, because it will be significantly slower as it would recreate all databases from scratch. Also, you should check whether Docker is available first. |
True, but the flip side is that with LocalDB-by-default you can't have multiple agents doing concurrent work on EF. My expectation is that people doing serious work on EF install SQL Server anyway and just set up the environment variable (or if they want to, set up the environment variable to point to LocalDB). Also, the default LocalDB that comes with Windows is probably far behind at this point compared to SQL Server 2025 features (JSON, vector...). I'm not sure it's that great a default any more.
What do you see being the advantage of that? If Docker isn't available, we'd have to fail anyway (DockerUnavailableException is thrown, which seems very explanatory), so this would just be a different failure message no? I see that the mac tests are now failing in CI, because we're no longer skipping them and there's no docker installed there (maybe this is what you were referring to?). For this, I'd rather very specifically skip on Mac in CI, so that normal users simply get DockerUnavailableException if they attempt to run the SQL Server tests and haven't installed docker (and also haven't defined How does that sound? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
An alternative solution is to add a flag that appends a GUID to the database name and deletes the database once finished
It comes with VS, not Windows
The advantage would be that they can be skipped in CI in that case and don't fail the build. |
True. Some thoughts on this:
Sorry, was not aware. Though we do need to remember that not everyone uses VS, even on Windows. BTW the performance impact of starting the container and creating/seeding the database isn't that big. In MEVD I do exactly this (default to spinning up testcontainers) and it's reasonable (otherwise testcontainers would be useless). As a heavy developer I still prefer to run against a permanent instance to optimize this further, but it's not that important for the more casual user (or for agents). To summarize, I'd rather our default were to just run our tests against real SQL Server - and the latest version of it at that (via container); if someone (i.e. us) wants to, they always have the option of defining an environment variable and doing whatever they want.
But when not running in CI, we do want them to fail. We currently skip SQL Server tests for non-Windows at the csproj level (see In any case, our CI situation is exceptional and a bit odd - the images should just have docker and we should be able to run SQL Server tests on them. I'm reluctant to go into complexity inside our test infra (beyond |
Are you going to submit a PR for this?
I'd argue that it's more likely than Docker, at least for contributors to this repo
Ok |
See #33267 (comment) for an AI agent motivation for this; not yet 100% sure how it all hangs together but this seems like a good idea regardless.
@AndriySvyryd note that the PR only defaults to testcontains on non-Windows; for now I've left Windows to default to LocalDB. I think it actually might make sense to apply the change to Windows as well for the same reasons (multiple parallel agents with isolated databases - see commented linked to above).
Let me know if this makes sense to you or you prefer to keep LocalDB as a default.
Closes #33267