Skip to content

Default to using SQL Server testcontainer in tests#37809

Open
roji wants to merge 1 commit intodotnet:mainfrom
roji:SqlServerTestContainer
Open

Default to using SQL Server testcontainer in tests#37809
roji wants to merge 1 commit intodotnet:mainfrom
roji:SqlServerTestContainer

Conversation

@roji
Copy link
Member

@roji roji commented Feb 26, 2026

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

@roji roji requested a review from a team as a code owner February 26, 2026 12:15
Copilot AI review requested due to automatic review settings February 26, 2026 12:15
Copy link

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.

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.MsSql and updated TestEnvironment to 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

  • DefaultConnection is wrapped in Lazy, but _dataSource is initialized eagerly via new SqlConnectionStringBuilder(DefaultConnection) during type initialization. This defeats the laziness and will start the container as soon as any TestEnvironment member is accessed (e.g. IsCI in SqlServerConditionAttribute), 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);

Comment on lines +41 to +45
_container = new MsSqlBuilder("mcr.microsoft.com/mssql/server:2025-latest")
.Build();

_container.StartAsync().GetAwaiter().GetResult();

Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
@ErikEJ
Copy link
Contributor

ErikEJ commented Feb 26, 2026

@roji Only issue could be resource constraints on your Windows agent? and unsure about in-memory tables tests?

@roji
Copy link
Member Author

roji commented Feb 26, 2026

@roji Only issue could be resource constraints on your Windows agent?

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 unsure about in-memory tables tests?

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.

@AndriySvyryd
Copy link
Member

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.

@roji
Copy link
Member Author

roji commented Feb 27, 2026

We shouldn't do this by default on Windows, because it will be significantly slower as it would recreate all databases from scratch.

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.

Also, you should check whether Docker is available first.

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

How does that sound?

@roji
Copy link
Member Author

roji commented Feb 27, 2026

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndriySvyryd
Copy link
Member

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

An alternative solution is to add a flag that appends a GUID to the database name and deletes the database once finished

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.

It comes with VS, not Windows

What do you see being the advantage of that? If Docker isn't available, we'd have to fail anyway, so this would just be a different failure message no, which doesn't seem very valuable? If you care that much about the error message, we can always catch and rethrow any exceptions coming out of the testcontainer start.

The advantage would be that they can be skipped in CI in that case and don't fail the build.

@roji
Copy link
Member Author

roji commented Feb 27, 2026

An alternative solution is to add a flag that appends a GUID to the database name and deletes the database once finished

True. Some thoughts on this:

  • This also has the advantage of reusing the same SQL Server instance instead spinning up multiple instances - important since each SQL Server instance takes ~2GB of RAM.
  • I'm not sure if typical agent sandboxing allows setting up access from inside the sandbox to an external databases (would need to research). On the other hand it's also not 100% clear whether they allow docker-in-docker (to spin up the testcontainer within the sandbox).
  • If we do this, GUID-based database names are something I 100% want to avoid for regular interactive use. It's a pain to not be able to look at the database after the test (or during), plus they also get left behind when tests crash in the middle and need to be cleaned up - it really is an inferior way to work IMHO.
  • So we'd likely need some sort of environment variable to tell our tests to append the GUIDs, and instruct the agent to pass this environment variable (which we wouldn't use ourselves in interactive use). A similar thing would need to be done with the testcontainer anyway, since I still want to run against my regular installed SQL Server, whereas agents do testcontainers (so the connection string env var would need to differ).
  • Since GUID-appending doesn't provide full isolation, there might be cases where we fiddle with global SQL Server state and cause trouble. But I admit this is mostly theoretical, I think.

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.

It comes with VS, not Windows

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.

What do you see being the advantage of that? If Docker isn't available, we'd have to fail anyway, so this would just be a different failure message no, which doesn't seem very valuable? If you care that much about the error message, we can always catch and rethrow any exceptions coming out of the testcontainer start.

The advantage would be that they can be skipped in CI in that case and don't fail the build.

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 <SkipTests>), not by detecting whether SQL Server is present or not - I don't see any reason not to continue just doing that. We can simply modify the <SkipTests> to only skip on CI to achieve the same effect, this way tests are skipped in CI but fail locally (in a very clear way) if docker isn't installed. How does that sound?

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 <SkipTests>) just to account for problematic images.

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Feb 27, 2026

An alternative solution is to add a flag that appends a GUID to the database name and deletes the database once finished

True.

Are you going to submit a PR for this?

Though we do need to remember that not everyone uses VS, even on Windows.

I'd argue that it's more likely than Docker, at least for contributors to this repo

We currently skip SQL Server tests for non-Windows at the csproj level (see ), not by detecting whether SQL Server is present or not - I don't see any reason not to continue just doing that. We can simply modify the to only skip on CI to achieve the same effect, this way tests are skipped in CI but fail locally (in a very clear way) if docker isn't installed. How does that sound?

Ok

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.

Use testcontainer for SQL Server tests by default

4 participants