CSHARP-5338: Improve integration test performance by test fixtures #1332

sanych-sun · 2024-05-27T23:37:48Z

Most of the integration tests we have currently re-inserting the same data for each run. For the tests that is reading data (like a most LINQ tests) it's inefficient. Proposed changes let us utilize xUnit's Fixture to share the test data for the test collection. It means that Fixture will hold the test data, which will be inserted only one before tests will be run for a test class. Also fixture will be disposed after all tests were run, so we can cleanup the test collection.

I've choose 20 last tests in Jira folder because all of them are integration and refactored to use the approach. On mine local machine I saw like a 4 times improvement for this tests.

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

sanych-sun · 2024-05-27T23:41:41Z

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

+
+        protected TFixture Fixture { get; }
+
+        protected void AssertStages(IEnumerable<BsonDocument> stages, params string[] expectedStages)


All Assert- and Translate- methods were simply copied from existing Linq3IntegrationTest base class (which probably has to be removed)

A lot of these Translate methods only apply to LINQ integration tests, does it matter that they are in a more general purpose class now called IntegrationTest when not all integration tests use them?

We could derive Linq3IntegrationTest from IntegrationTest to keep the LINQ specific methods contained.

Or we could just decide it's OK for them to be bundled here. In which case the Linq3IntegrationTest class could be removed once nothing is using it any more.

I like the idea to derive Linq3IntegrationTest from IntegrationTest. Will check if we can do it now, or should do it as the very last step of the refactoring if there are any conflicts.

I've decided to introduce new LinqIntegrationTest base class.
For 2 reasons:

we will be using the class to test both V3 and V2 LinqProvider (as we mostly doing)

In such way we can gradually update tests and once done - we will remove 'old' base classes.

sanych-sun · 2024-05-27T23:47:03Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4702Tests.cs

 using MongoDB.TestHelpers.XunitExtensions;
 using Xunit;

 namespace MongoDB.Driver.Tests.Linq.Linq3Implementation.Jira
 {
-    public class CSharp702Tests : Linq3IntegrationTest
+    public class CSharp702Tests : IntegrationTest<CSharp702Tests.TestDataFixture>


The main difference with comparing to the current test class structure:

Test class should be inherited from IntegrationTest base class

For typical test class that uses only 1 set of data we should create TestDataFixture as nested class and provide it as a type parameter for base IntegrationTest

Test class should have ctor to pass the Fixture created by xUnit infrastructure

Test collection is available through the Fixture property

Test data should be moved from GetCollection method to TestDataFixture

Maybe fix the class name while we're at it? Doesn't match the filename.

Class was renamed.

sanych-sun · 2024-05-27T23:50:30Z

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

+        protected TFixture Fixture { get; }
+
+        protected void AssertStages(IEnumerable<BsonDocument> stages, params string[] expectedStages)
+        {


BTW, we already have 2 sets of such methods: in Linq3IntegrationTest class and also in Linq3TestHelpers static class. We probably ought to standardize the approach and have either static helper class or use base IntegrationTest class.

Linq3TestHelpers is a very old class that is not used in any new tests, but hasn't been refactored out yet of older classes. Once we refactor to no longer use Linq3TestHelpers it can definitely be deleted.

I'll try to consolidate all usages into the just introduced LinqIntegrationTest base class and remove "old" methods.

sanych-sun · 2024-05-28T00:02:29Z

tests/MongoDB.Driver.TestHelpers/TemporaryCollectionFixture.cs

+
+        protected abstract IEnumerable<TModel> GetInitialData();
+
+        protected virtual string CollectionName => _collectionName.Value;


Default collection name would be taken from the parent class name, as I suggest to have fixture as a nested class of the test class.

BorisDog

Looks like a great improvement to our tests. Waiting to see how faster our tests get when the rest of the tests are migrated.

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

BorisDog · 2024-05-29T22:54:22Z

tests/MongoDB.Driver.TestHelpers/TemporaryDatabaseFixture.cs

+            }
+        }
+
+        public IMongoClient MongoDbClient => DriverTestConfiguration.Client;


Maybe create own disposable MongoClient?
But then it's better to be shared across the tests.

Done. Client(s) will be created per fixture and will be disposed together with fixture. See TemporaryDatabaseFixture class for implementation.

BorisDog · 2024-05-29T23:26:04Z

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

+        }
+    }
+
+    public abstract class IntegrationTest : IntegrationTest<TemporaryDatabaseFixture>


This one is not needed?

It will be used for tests that uses TemporaryDatabaseFixture instead of TemporaryCollectionFixture. I'll show an example in next commit.

Please find example of usage in CSharp4066Tests test class.

Maybe a better naming to reflect that. And then why not having matching class for TemporaryCollectionFixture ?

I've removed the class for now to avoid confusions. We might consider to re-introduce it later.

BorisDog · 2024-05-29T23:31:47Z

tests/MongoDB.Driver.TestHelpers/TemporaryCollectionFixture.cs

+
+namespace MongoDB.Driver.TestHelpers
+{
+    public abstract class TemporaryCollectionFixture<TModel> : TemporaryDatabaseFixture


We might need to readjust the naming, as more tests are migrated to use this approach. Something like ReadOnlyCollectionFixture...

rstam

Overall I think this approach is great! Nice!

rstam · 2024-05-28T20:54:48Z

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

+
+        protected TFixture Fixture { get; }
+
+        protected void AssertStages(IEnumerable<BsonDocument> stages, params string[] expectedStages)


A lot of these Translate methods only apply to LINQ integration tests, does it matter that they are in a more general purpose class now called IntegrationTest when not all integration tests use them?

We could derive Linq3IntegrationTest from IntegrationTest to keep the LINQ specific methods contained.

Or we could just decide it's OK for them to be bundled here. In which case the Linq3IntegrationTest class could be removed once nothing is using it any more.

rstam · 2024-05-28T20:55:43Z

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

+        protected TFixture Fixture { get; }
+
+        protected void AssertStages(IEnumerable<BsonDocument> stages, params string[] expectedStages)
+        {


Linq3TestHelpers is a very old class that is not used in any new tests, but hasn't been refactored out yet of older classes. Once we refactor to no longer use Linq3TestHelpers it can definitely be deleted.

rstam · 2024-05-28T20:58:58Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4702Tests.cs

@@ -48,7 +54,7 @@ public class CSharp702Tests : Linq3IntegrationTest
        public void Query2_using_list_should_work(
            [Values(LinqProvider.V2, LinqProvider.V3)] LinqProvider linqProvider)
        {
-            var collection = GetCollection(linqProvider);
+            var collection = Fixture.GetCollection<Model>(linqProvider);


Why not just:

var collection = Fixture.GetCollection(linqProvider);

I guess it's because there is a contradiction between TModel and BsonDocument in GetInitialData.

There is probably a better way to solve that contradiction (see GetRawInitialData proposal).

Yes, in this particular test we are inserting the data using one model, but want to read the data as another.

I've decided to make InitializeCollection virtual, so derived fixtures are free to use any initialization logic.

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

rstam · 2024-05-29T23:09:12Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4772Tests.cs

        }

-        private class Profile
+        public class ProfileModel


Do you remember why you renamed the class?

The name conflict with some namespace. I did not investigate this deep, just renamed the model to get rid of conflict.

rstam · 2024-05-29T23:29:26Z

tests/MongoDB.Driver.TestHelpers/TemporaryDatabaseFixture.cs

+        private static readonly string __timeStamp = DateTime.Now.ToString("yyyyMMdd-HHmmss");
+
+        protected readonly string _databaseName;
+        private readonly ConcurrentBag<string> _createdCollections = new();


Rename field _createdCollectionNames because we are only storing the names, not the collections.

rstam · 2024-05-29T23:30:07Z

tests/MongoDB.Driver.TestHelpers/TemporaryDatabaseFixture.cs

+            }
+        }
+
+        public IMongoClient MongoDbClient => DriverTestConfiguration.Client;


I would remove this property.

We want to encourage using GetClient(linqProvider) and not using this property by mistake.

rstam · 2024-05-29T23:30:37Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4702Tests.cs

 using MongoDB.TestHelpers.XunitExtensions;
 using Xunit;

 namespace MongoDB.Driver.Tests.Linq.Linq3Implementation.Jira
 {
-    public class CSharp702Tests : Linq3IntegrationTest
+    public class CSharp702Tests : IntegrationTest<CSharp702Tests.TestDataFixture>


Maybe fix the class name while we're at it? Doesn't match the filename.

rstam · 2024-05-29T23:32:25Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4702Tests.cs

        private class Model
        {
            public int Id { get; set; }
            public List<string> List { get; set; }
            public HashSet<string> HashSet { get; set; }
        }
+
+        public class TestDataFixture : TemporaryCollectionFixture<BsonDocument>


public class CollectionFixture : TemporaryCollectionFixture<MyModel>

I would name this class CollectionFixture rather than TestDataFixture. To me the most important thing is that it is a "collection fixture". Initializing the test data is a secondary function.

Or Model if you don't get the same naming conflict with Amazon,SecurityServices that I was getting.

rstam · 2024-05-29T23:33:03Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4702Tests.cs

+
+        public class TestDataFixture : TemporaryCollectionFixture<BsonDocument>
+        {
+            protected override IEnumerable<BsonDocument> GetInitialData()


GetRawInitialData

InitializeCollection is virtual now as per your another suggestion.

BorisDog · 2024-05-31T23:02:15Z

tests/MongoDB.TestHelpers/XunitExtensions/IntegrationTestAttribute.cs

+        public IntegrationTestAttribute()
+            : base("Integration")
+        {
+


minor: remove empty line

BorisDog · 2024-05-31T23:15:26Z

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

+        }
+    }
+
+    public abstract class IntegrationTest : IntegrationTest<TemporaryDatabaseFixture>


Maybe a better naming to reflect that. And then why not having matching class for TemporaryCollectionFixture ?

BorisDog · 2024-05-31T23:16:42Z

tests/MongoDB.Driver.TestHelpers/TemporaryDatabaseFixture.cs

+        }
+
+        public IMongoClient GetClient(LinqProvider provider)
+            => DriverTestConfiguration.GetLinqClient(provider);


The question regarding the client still remains. Do we want to use this opportunity to start working with disposable clients?

Done. See TemporaryDatabaseFixture for implementation.

BorisDog · 2024-05-31T23:18:22Z

tests/MongoDB.Driver.TestHelpers/TemporaryDatabaseFixture.cs

+            {
+                database.DropCollection(collection);
+            }
+        }


Why not to drop the database instead?

Currently it would be almost the same, but if we approach running in parallel task - then it will be safer to remove the collection as database might be in use by other tests/fixtures.

BorisDog · 2024-05-31T23:19:55Z

tests/MongoDB.Driver.TestHelpers/TemporaryDatabaseFixture.cs

+            if (string.IsNullOrEmpty(collectionName))
+            {
+                var stack = new System.Diagnostics.StackTrace();
+                var frame = stack.GetFrame(1); // skip 1 frame to get the calling method info


Nice,
I wanted to suggest [CallerMemberName] , but we having type name is better.

BorisDog · 2024-05-31T23:31:12Z

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

+namespace MongoDB.Driver.Tests
+{
+    [IntegrationTest]
+    public abstract class IntegrationTest<TFixture> : IClassFixture<TFixture>


Probably worth inheriting for LoggableTestClass, to have logging and timeouts for all.
Ideally all test would derive from LoggableTestClass.

IntegrationTest is now inherited from LoggableTestClass. Let's discuss offline if we need some extra functionality around.

BorisDog · 2024-05-31T23:34:27Z

tests/MongoDB.Driver.TestHelpers/TemporaryDatabaseFixture.cs

+                var frame = stack.GetFrame(1); // skip 1 frame to get the calling method info
+                var method = frame.GetMethod();
+                collectionName = $"{method.DeclaringType.Name}.{method.Name}";
+            }


It's worth having some tests that consume same collection, overwrite same data. To see how we avoid collisions.
So we are ready for parallelization.

For test Theories - yep. It will be more complicated, as we will need to add some extra suffixes to the names probably (based on test parameters? or test sequential number). Will try to find a way to work with this.

sanych-sun · 2024-06-08T00:49:09Z

CSharpDriver.sln.DotSettings

@@ -1,4 +1,14 @@
 <wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">


This file is really hard to review, but we need the changes to let Rider/ReSharper add Integration Tests from template. I can demo how it works on some stand up.

Maybe we should split templates change from the fixtures changes?
It's better to make sure that template work correctly for both VS and Rider, so we don't tie these two efforts together.

BorisDog · 2024-10-17T23:25:01Z

CSharpDriver.sln.DotSettings

@@ -1,4 +1,14 @@
 <wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">


Maybe we should split templates change from the fixtures changes?
It's better to make sure that template work correctly for both VS and Rider, so we don't tie these two efforts together.

BorisDog · 2024-10-17T23:28:07Z

tests/MongoDB.Driver.TestHelpers/DatabaseFixture.cs

+
+namespace MongoDB.Driver.Tests
+{
+    public class DatabaseFixture: IDisposable


nit: space before ':'

BorisDog · 2024-10-17T23:34:20Z

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

+            });
+        }
+
+        public IMongoCollection<T> GetCollection<T>(Action<MongoClientSettings> configure = null, string collectionName = null)


Should this be protected? Just to signal that these methods are intended for derived classes.

BorisDog · 2024-10-25T23:11:10Z

tests/MongoDB.Driver.TestHelpers/CollectionFixture.cs

+
+        public virtual bool ResetOnEachGet => false;
+
+        public override IMongoCollection<T> GetCollection<T>(Action<MongoClientSettings> configure = null, string collectionName = null)


Do we have cases where T != TDocument?

BorisDog · 2024-10-25T23:15:40Z

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

+
+        public IMongoDatabase GetDatabase(Action<MongoClientSettings> configure = null)
+        {
+            return _fixture.GetDatabase(settings =>


minor: expression body ?

BorisDog · 2024-10-25T23:21:02Z

tests/MongoDB.Driver.TestHelpers/DatabaseFixture.cs

+            if (string.IsNullOrEmpty(collectionName))
+            {
+                var stack = new System.Diagnostics.StackTrace();
+                var frame = stack.GetFrame(2); // skip 2 frame to get the calling method info (this method and IntegrationTest method)


nit: frames

BorisDog · 2024-10-25T23:21:36Z

tests/MongoDB.Driver.TestHelpers/DatabaseFixture.cs

+            {
+                database.DropCollection(collection);
+            }
+        }


Don't we want just to drop the DB instead?

BorisDog · 2024-10-25T23:22:59Z

tests/MongoDB.Driver.TestHelpers/DatabaseFixture.cs

+    {
+        private static readonly string __timeStamp = DateTime.Now.ToString("yyyyMMdd-HHmmss");
+
+        private readonly string _databaseName = $"CsharpDriver-{__timeStamp}";


nit: CSharp

BorisDog · 2024-10-25T23:24:13Z

tests/MongoDB.Driver.TestHelpers/DatabaseFixture.cs

+        private readonly HashSet<string> _usedCollections = new();
+
+        public virtual void Dispose()
+        {


Dispose the client.

BorisDog · 2024-10-25T23:29:30Z

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs

+{
+    [IntegrationTest]
+    public abstract class IntegrationTest<TFixture> : LoggableTestClass, IClassFixture<TFixture>
+        where TFixture : DatabaseFixture


Do we want to consider reusing the client between tests as well (ICollectionFixture)?
So we don't dispose it after each tests class. For example single client for all LinqIntegrationTest?

sanych-sun requested a review from a team as a code owner May 27, 2024 23:37

sanych-sun requested review from JamesKovacs, rstam, BorisDog and adelinowona and removed request for a team May 27, 2024 23:38

sanych-sun commented May 27, 2024

View reviewed changes

tests/MongoDB.Driver.TestHelpers/IntegrationTest.cs Outdated Show resolved Hide resolved

sanych-sun commented May 27, 2024

View reviewed changes

sanych-sun commented May 28, 2024

View reviewed changes

BorisDog reviewed May 29, 2024

View reviewed changes

rstam reviewed May 29, 2024

View reviewed changes

sanych-sun requested review from rstam and BorisDog May 31, 2024 19:42

BorisDog reviewed May 31, 2024

View reviewed changes

sanych-sun commented Jun 8, 2024

View reviewed changes

sanych-sun requested a review from BorisDog June 8, 2024 00:56

JamesKovacs removed their request for review June 14, 2024 23:08

rstam removed their request for review September 30, 2024 17:22

sanych-sun force-pushed the experiments/unit-tests-run-in-parallel branch 2 times, most recently from f1f0475 to c2c4ad2 Compare October 17, 2024 00:21

sanych-sun requested a review from rstam October 17, 2024 16:32

sanych-sun changed the title ~~[WIP] Refactor tests to use fixture for test data population~~ CSHARP-5338: Improve integration test performance by test fixtures Oct 17, 2024

BorisDog requested changes Oct 25, 2024

View reviewed changes

Oleksandr Poliakov added 5 commits October 29, 2024 11:54

Add data population fixture

c0f75e8

PR

031dd1a

PR

b4a58d4

Introduce LinqIntegrationTest class

476b180

Rework after 3.0 release

7da9712

Fix collection name resolving

1e68de6

sanych-sun force-pushed the experiments/unit-tests-run-in-parallel branch from f4578b4 to 1e68de6 Compare October 29, 2024 18:54


		protected TFixture Fixture { get; }

		protected void AssertStages(IEnumerable<BsonDocument> stages, params string[] expectedStages)


		protected abstract IEnumerable<TModel> GetInitialData();

		protected virtual string CollectionName => _collectionName.Value;

		@@ -1,4 +1,14 @@
		<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">


		public virtual bool ResetOnEachGet => false;

		public override IMongoCollection<T> GetCollection<T>(Action<MongoClientSettings> configure = null, string collectionName = null)

CSHARP-5338: Improve integration test performance by test fixtures #1332

Are you sure you want to change the base?

CSHARP-5338: Improve integration test performance by test fixtures #1332

Conversation

sanych-sun commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun Jun 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun May 28, 2024 • edited Loading

Choose a reason for hiding this comment

BorisDog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun Jun 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun Jun 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun Jun 8, 2024 •

edited

Loading

sanych-sun May 27, 2024 •

edited

Loading

sanych-sun May 28, 2024 •

edited

Loading

sanych-sun Jun 8, 2024 •

edited

Loading

sanych-sun Jun 8, 2024 •

edited

Loading