Skip to content

[MEVD] Specification/conformance integration test suite for providers #10194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
roji opened this issue Jan 15, 2025 · 11 comments · Fixed by #10944
Open

[MEVD] Specification/conformance integration test suite for providers #10194

roji opened this issue Jan 15, 2025 · 11 comments · Fixed by #10944
Assignees
Labels
memory connector memory msft.ext.vectordata Related to Microsoft.Extensions.VectorData .NET Issue or Pull requests regarding .NET code sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community)

Comments

@roji
Copy link
Member

roji commented Jan 15, 2025

The current integration tests for NEVM providers are implemented separately for each provider, with no support from the abstraction. To promote better universal coverage and reduce the per-provider work needed, we could expoes a "specification test suite" with the abstraction, which providers implement, and which execute a set of standardized tests to ensure the provider works correctly. A good model here would be the Entity Framework Core specification test suite, which exercises an EF provider in a myriad of ways.

/cc @westey-m

@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code triage labels Jan 15, 2025
@github-actions github-actions bot changed the title .NET NEVM: Specification test suite for providers .Net NEVM: Specification test suite for providers Jan 15, 2025
@markwallace-microsoft markwallace-microsoft added msft.ext.vectordata Related to Microsoft.Extensions.VectorData memory connector memory and removed triage labels Jan 16, 2025
@dmytrostruk
Copy link
Member

dmytrostruk commented Jan 21, 2025

Small note: I already refactored the integration tests for IVectorStore interface to have a single set of tests for each provider:
https://github.com/microsoft/semantic-kernel/blob/main/dotnet/src/IntegrationTests/Connectors/Memory/BaseVectorStoreTests.cs

I tried to do the same with IVectorStoreRecordCollection interface and it appeared that it's more complicated than IVectorStore, since each provider has some limitations (e.g. the requirements for collection/table/field names, supported index kinds, dimensions etc), which makes it harder to unify into a single test suite to cover all methods in that abstraction and all available connectors. At the end, I noticed that I still need to override some test cases for specific connectors to ensure high coverage and this put the idea of a single test suite under the question.

As an alternative solution, we can keep current tests for specific providers to ensure high coverage but add separate set of test cases (maybe basic ones) using abstraction only and enable it for as many connectors as we can.

@roji
Copy link
Member Author

roji commented Jan 22, 2025

@dmytrostruk thanks for the context, yeah, that all makes sense. I plan to take a look at this, will share what i come up with. I agree that it's OK if an implementation needs to have its own tests in addition to the "standard" ones it can get for free - I do hope we'll be able to provide a meaningful set of standard ones, without being prevented from testing too much by differences between the database etc.

@roji roji changed the title .Net NEVM: Specification test suite for providers .Net NEVD: Specification test suite for providers Jan 31, 2025
@evchaki evchaki added the sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community) label Jan 31, 2025
@roji
Copy link
Member Author

roji commented Feb 6, 2025

Note: the basic infrastructure for this and first test implementation are being done as part of #10273 (LINQ-based filtering).

  • Base test classes and fixtures to be extended by each provider (and published via a separate nuget). This is the main point of the exercise.
  • Use of testcontainers wherever possible; this simplifies setting up the Docker container, manages waiting for the service to actually be available (reduces flakiness), etc.
  • Where container-based testing isn't possible (since there's no Docker image), have the test suite check whether the required configuration for accessing the service is defined - and skip if not. This allows a dev to simply define e.g. an endpoint URL environment variable to activate testing a connector, while not failing tests where that config is missing (just skip).
  • Separate integration test project for each provider. This makes sense since we eventually would like connectors to be owned by their respective vector database vendors, and so ideally move out of the SK repo. This also separates the tests for different containers, allowing easier work on one without affecting others, etc.

Assuming we're all OK with this direction (we should discuss), am keeping this issue to track porting over the other non-filter integration tests over into this new integration test structure.

@dmytrostruk
Copy link
Member

Base test classes and fixtures to be extended by each provider (and published via a separate nuget)

Do we need to publish test-related code via NuGet? If we need to share something internally between test projects - we have a concept of InternalUtilities in a repo, where we define some shared classes and then include them into specific projects where needed in compile time.

@roji
Copy link
Member Author

roji commented Feb 8, 2025

@dmytrostruk the reason to publish test-related code via NuGet is for supporting connectors which are maintained outside the repo, e.g. by the database vendor (I think there's one today already?). Such vendors should be able to easily use our specification/conformance tests, and also update the nuget as tests are improved/added for new APIs. FWIW we have this approach in EF, where a "specification" test suite is published as a nuget, and it greatly helps external implementation maintainers.

Of course, for connectors inside the SK repo, a simple <PackageReference> can be used to reference the test infra stuff, which is very similar to having shared internal classes. Do you see any specific problem with doing things this way (i.e. with a nuget)?

@roji roji closed this as completed Feb 8, 2025
@roji roji reopened this Feb 8, 2025
@roji roji changed the title .Net NEVD: Specification test suite for providers .Net MEVD: Specification test suite for providers Feb 8, 2025
roji added a commit to roji/semantic-kernel that referenced this issue Feb 8, 2025
roji added a commit to roji/semantic-kernel that referenced this issue Feb 8, 2025
roji added a commit to roji/semantic-kernel that referenced this issue Feb 8, 2025
roji added a commit to roji/semantic-kernel that referenced this issue Feb 9, 2025
roji added a commit to roji/semantic-kernel that referenced this issue Feb 9, 2025
roji added a commit to roji/semantic-kernel that referenced this issue Feb 9, 2025
roji added a commit to roji/semantic-kernel that referenced this issue Feb 9, 2025
roji added a commit to roji/semantic-kernel that referenced this issue Feb 9, 2025
@dmytrostruk
Copy link
Member

@roji Thanks for your response!

Do you see any specific problem with doing things this way (i.e. with a nuget)?

Not at all, just wanted to learn more about this approach, since we didn't publish a package to NuGet that is related to tests in Semantic Kernel yet. I think publishing specification test suite should really help connector developers to speed up the development process. I also think that it's not only about test suites, but also our internal classes such as VectorStoreRecordPropertyReader that we use for multiple connectors in SK repository, but they are not publicly available at the moment. If we publish such classes via NuGet, it could become a great "toolkit" for connector development.

Although, I'm not sure if we should handle it all together in scope of the same task. For example, we can start with internal test suite, use it for some period of time in SK repo, see how the tests perform (i.e. sometimes they can be flacky etc.). Later, when we are comfortable with sharing these tests with other developers, we can publish it via NuGet. I guess it's a matter of prioritization.

@roji
Copy link
Member Author

roji commented Feb 9, 2025

Not at all, just wanted to learn more about this approach, since we didn't publish a package to NuGet that is related to tests in Semantic Kernel yet.

Sure thing. FYI EF Core publishes very extensive tests in this way: an EF provider gets a ton of test coverage for free, and this is vital to making sure providers implement the functionality they need to, behave in the right way, etc.

but also our internal classes such as VectorStoreRecordPropertyReader that we use for multiple connectors in SK repository, but they are not publicly available at the moment.

Absolutely - that indeed also needs to be made public. External connectors generally shouldn't ever need to copy files from the SK repo (e.g. VectorStoreRecordPropertyReader), since that means they don't automatically get fixes and improvements to that code.

For example, we can start with internal test suite, use it for some period of time in SK repo, see how the tests perform (i.e. sometimes they can be flacky etc.)

We can. Though if something's flaky, that would be a specific database, or its implementation of the tests - the (abstract) tests themselves should never be flaky (if they are that's a bug). I understand the worry about flakiness, but at least from what I've seen working on the LINQ filtering tests, things seem rock solid across all databases. On that:

  • Testcontainers is responsible for waiting until the service within the container is actually ready (e.g. by probing a port, doing an HTTP request...). Your current integration tests manually manage bringing up the Docker container etc., and so they have to do their own retry loops until e.g. PG is up and accepting connections (I'm guessing that may have been a source of flakiness).
  • Some databases do not guarantee that inserted data is visible immediately when searching (eventual consistency); to address this, after seeding the base fixture logic does a search for all seed data and if they're not available, loops until it sees them.

So I don't think there's any particular reason to not publish a test nuget. Of course, it's not the most urgent thing we have to do, and given that there are almost no external connector developers, that can probably wait until after release. But I'm trying to at least design things in a way that will make this possible etc.

@markwallace-microsoft markwallace-microsoft moved this to Sprint: Planned in Semantic Kernel Feb 10, 2025
roji added a commit that referenced this issue Feb 11, 2025
Implement LINQ-based vector search filtering
    
Closes #10156
Does most of #10194
@roji roji changed the title .Net MEVD: Specification test suite for providers [MEVD] Specification/conformance integration test suite for providers Mar 9, 2025
@roji roji linked a pull request Mar 13, 2025 that will close this issue
@roji roji linked a pull request Mar 13, 2025 that will close this issue
@roji roji moved this from Sprint: Planned to Sprint: In Progress in Semantic Kernel Mar 13, 2025
github-merge-queue bot pushed a commit that referenced this issue Mar 13, 2025
- introduce CollectionConformanceTests
- introduce RecordConformanceTests
- make GenerateNextKey thread-safe
- make sure the tests don't create collection if there is no need to
- implement the tests for Postgres, SQL Server and Redis (partially)

contributes to #10194 and #10882 and #10883
@adamsitnik
Copy link
Member

adamsitnik commented Mar 13, 2025

#10944 has helped, but not solved the problem (yet)

@adamsitnik
Copy link
Member

The tests should also cover custom mappers.

@markwallace-microsoft markwallace-microsoft added the Build Features planned for next Build conference label Mar 19, 2025
adamsitnik added a commit that referenced this issue Mar 31, 2025
contributes to #10194

---------

Co-authored-by: Shay Rojansky <[email protected]>
@markwallace-microsoft markwallace-microsoft removed the Build Features planned for next Build conference label Apr 15, 2025
@markwallace-microsoft
Copy link
Member

We have sufficient coverage for Build. Keeping this open as there is more we want to do.

@roji
Copy link
Member Author

roji commented Apr 15, 2025

General overview of tasks that need to be done here (nothing for Build):

  • Make sure we have full coverage. This includes porting all the older integration tests into the new project.
  • Consider also moving the unit tests, this way we have exactly on test project per connector.
    • For connectors where we have a testcontainer, there generally shouldn't be a need to distinguish between integration and unit tests.
    • For connectors where we don't have a testcontainer, integration tests (which require the database) are already automatically skipped/disabled unless the cloud endpoint is provided via environment variables.
    • This means that users should be able to just run a connector test project and expect everything to just always work.
    • If we really feel like we need to distinguish between unit and integration tests, we can still use test categories/traits to do that (rather than splitting to two projects).
  • Do a general cleanup/refactoring pass. Different people added tests at different times with slightly different patterns, just make everything more consistent etc.
  • Decide on the naming - we sometimes call these specification tests (like EF), sometimes conformance tests (probably better?).
  • Change the project/namespace/package name to something like Microsoft.Extensions.VectorData.ConformanceTests
  • Actually publish Microsoft.Extensions.VectorData.ConformanceTests as a nuget package for external connectors to consume

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
memory connector memory msft.ext.vectordata Related to Microsoft.Extensions.VectorData .NET Issue or Pull requests regarding .NET code sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community)
Projects
Status: Sprint: In Progress
Development

Successfully merging a pull request may close this issue.

5 participants