Skip to content

Remove file-based seeding for Cosmos DB functional tests#38005

Merged
AndriySvyryd merged 3 commits intodotnet:mainfrom
JoasE:feature/cosmos-northwind-speedup
Mar 27, 2026
Merged

Remove file-based seeding for Cosmos DB functional tests#38005
AndriySvyryd merged 3 commits intodotnet:mainfrom
JoasE:feature/cosmos-northwind-speedup

Conversation

@JoasE
Copy link
Copy Markdown
Contributor

@JoasE JoasE commented Mar 26, 2026

File based seeding was very slow and used manual serialization. As far as I could see, it was only necessary for 1 test that could be rewritten. This approach, leveraging EF's batching SaveChangesAsync, is much faster. This is a remnant from: #37766 and is related to modernizing our serialization setup, removing the dependency on netwonsoft json. If file based seeding is still desired for some other reason, I will close this and publish a separate PR refactoring this to use STJ. Let me know which direction is wanted

File based seeding was very slow and used manual serialization. As far as I could see, it was only necessary for 1 test that could be rewritten. The approach levaraging EF's batching SaveChangesAsync is much faster. This is a remnant from: dotnet#37766 and is related to modernizing our serialization setup, removing the dependency on netwonsoft json. If file based seeding is still desired for some other reason, I will close this and publish a separate PR refactoring this to use STJ. Let me know which direction is optimal
@JoasE JoasE marked this pull request as ready for review March 26, 2026 10:12
@JoasE JoasE requested a review from a team as a code owner March 26, 2026 10:12
Copy link
Copy Markdown
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

LGTM - I've never quite understand why we seed northwind from a file (including in relational).

But leaving to @AndriySvyryd to approve as he may have more context/insight.

@AndriySvyryd
Copy link
Copy Markdown
Member

I've never quite understand why we seed northwind from a file (including in relational).

To test the query pipeline independently of the update pipeline

@roji
Copy link
Copy Markdown
Member

roji commented Mar 26, 2026

So you think that's something we need to keep doing?

@AndriySvyryd
Copy link
Copy Markdown
Member

So you think that's something we need to keep doing?

Not necessarily


private static string CreateName(string name)
=> TestEnvironment.IsEmulator || name == "Northwind" || name == "Northwind2" || name == "Northwind3"
=> TestEnvironment.IsEmulator || _reservedNames.Contains(name)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the creation is fast now, we don't need to share and preserve the database. Let it uniquify it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

to be clear: it's a lot faster, but still takes ~4 seconds. I've removed the _reservedNames and delete the database after each test now

Copy link
Copy Markdown

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 removes the legacy file-based Northwind seeding path used by EFCore.Cosmos functional tests, shifting seeding to the normal EF-based pipeline (batched SaveChangesAsync) to improve performance and reduce reliance on manual JSON serialization.

Changes:

  • Removed file-based seeding support (and the associated Newtonsoft-based implementation) from CosmosTestStore.
  • Deleted the specialized CosmosNorthwindTestStoreFactory and switched the Northwind Cosmos fixture to the standard CosmosTestStoreFactory.
  • Updated the affected FromSql test and removed Northwind.json output-copy configuration from the test project.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/EFCore.Cosmos.FunctionalTests/TestUtilities/CosmosTestStore.cs Removes file-based seeding path; introduces reserved-name handling adjustments.
test/EFCore.Cosmos.FunctionalTests/TestUtilities/CosmosNorthwindTestStoreFactory.cs Deletes the Northwind-specific factory previously used for file seeding.
test/EFCore.Cosmos.FunctionalTests/Query/NorthwindQueryCosmosFixture.cs Uses the default Cosmos test store factory rather than the deleted Northwind-specific one.
test/EFCore.Cosmos.FunctionalTests/Query/FromSqlQueryCosmosTest.cs Adjusts the raw Cosmos SQL predicate/expected results to align with EF-seeded data shape.
test/EFCore.Cosmos.FunctionalTests/EFCore.Cosmos.FunctionalTests.csproj Stops copying Northwind.json to output.
Comments suppressed due to low confidence (1)

test/EFCore.Cosmos.FunctionalTests/TestUtilities/CosmosTestStore.cs:485

  • DisposeAsync now skips both RemoveShared and EnsureDeletedAsync for any store whose Name is in _reservedNames (Northwind/Northwind2/Northwind3). Previously, only the file-seeded store avoided deletion; with this change, the standard Northwind stores will no longer be cleaned up, which can leak Cosmos databases/containers (especially when running against a real Cosmos account) and can leave stale data between runs. Consider restoring deletion/removal for Northwind/Northwind2 (or restricting the non-deletion behavior to the one store that truly needs to persist) so that reserved naming doesn’t implicitly mean “never delete.”
    public override async ValueTask DisposeAsync()
    {
        if (_initialized
            && !_reservedNames.Contains(Name))
        {
            if (_connectionAvailable == false)
            {
                return;
            }

            if (Shared)
            {
                GetTestStoreIndex(ServiceProvider).RemoveShared(GetType().Name + Name);
            }

            await EnsureDeletedAsync(_storeContext).ConfigureAwait(false);

@JoasE
Copy link
Copy Markdown
Contributor Author

JoasE commented Mar 27, 2026

To test the query pipeline independently of the update pipeline

@AndriySvyryd That is a good point! But from the context I gather that you don't think it's needed anymore. I could spend some timer refactor/improve/speed up the file based seeding via a CosmosClient aswell if you think it's worth it let me know. Otherwise you can go ahead and merge this

@JoasE JoasE requested a review from AndriySvyryd March 27, 2026 11:27
@AndriySvyryd AndriySvyryd enabled auto-merge (squash) March 27, 2026 18:36
@AndriySvyryd
Copy link
Copy Markdown
Member

Thanks for your contribution!

@AndriySvyryd AndriySvyryd merged commit 52872e5 into dotnet:main Mar 27, 2026
10 checks passed
@JoasE JoasE deleted the feature/cosmos-northwind-speedup branch March 27, 2026 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants