Skip to content

Commit fc12213

Browse files
ajcvickersajcvickers
committed
Review feedback
1 parent 2e47df6 commit fc12213

7 files changed

Lines changed: 120 additions & 76 deletions

File tree

‎AGENTS.md‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ The C# driver for MongoDB.
3434
| Feature area | Required environment variables |
3535
|---|---|
3636
| Atlas Search | `ATLAS_SEARCH_TESTS_ENABLED`, `ATLAS_SEARCH_URI` |
37-
| Atlas Search index helpers | `ATLAS_SEARCH_INDEX_HELPERS_TESTS_ENABLED`, `ATLAS_SEARCH_URI` |
3837
| CSFLE / auto-encryption | `CRYPT_SHARED_LIB_PATH` |
3938
| CSFLE with KMS mock servers | `KMS_MOCK_SERVERS_ENABLED` |
4039
| CSFLE with AWS KMS | `CSFLE_AWS_TEMPORARY_CREDS_ENABLED` |

‎evergreen/run-atlas-search-index-helpers-test.sh‎

Lines changed: 0 additions & 17 deletions
This file was deleted.

‎tests/MongoDB.Driver.Tests/Search/AtlasSearchFixture.cs‎

Lines changed: 84 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,11 @@ public sealed class AtlasSearchFixture : IDisposable
6565
private bool _returnScopeInitialized;
6666
private bool? _isRerankSupported;
6767

68+
// Routes events from the shared cluster to whichever subscribers test classes
69+
// have currently attached. Replaces the previously-shared single EventCapturer
70+
// so per-test-class capturers can coexist without stepping on each other.
71+
private readonly CompositeEventSubscriber _eventRouter = new();
72+
6873
public AtlasSearchFixture()
6974
{
7075
// The fixture must be tolerant of missing env vars so xUnit can skip individual
@@ -78,19 +83,27 @@ public AtlasSearchFixture()
7883
var settings = MongoClientSettings.FromConnectionString(atlasSearchUri);
7984
settings.ClusterSource = DisposingClusterSource.Instance;
8085

81-
EventCapturer = new EventCapturer().Capture<CommandStartedEvent>(e => e.CommandName == "aggregate");
82-
settings.ClusterConfigurator = b => b.Subscribe(EventCapturer);
86+
settings.ClusterConfigurator = b => b.Subscribe(_eventRouter);
8387

8488
Client = new MongoClient(settings);
8589
DatabaseName = "atlas_search_" + Guid.NewGuid().ToString("N");
8690
}
8791

8892
public IMongoClient Client { get; }
8993

90-
public EventCapturer EventCapturer { get; }
91-
9294
public string DatabaseName { get; }
9395

96+
/// <summary>
97+
/// Attaches a subscriber to the fixture-owned cluster. Each test class should
98+
/// register its own <see cref="EventCapturer"/> here (rather than sharing one)
99+
/// and detach it during teardown.
100+
/// </summary>
101+
public void AddEventSubscriber(IEventSubscriber subscriber) =>
102+
_eventRouter.Add(subscriber);
103+
104+
public void RemoveEventSubscriber(IEventSubscriber subscriber) =>
105+
_eventRouter.Remove(subscriber);
106+
94107
/// <summary>
95108
/// Lazily probes for $rerank support on first access. Tests guard themselves on this so the
96109
/// suite continues to work on Atlas deployments that don't yet ship the rerank model.
@@ -100,20 +113,22 @@ public bool IsRerankSupported
100113
get
101114
{
102115
EnsureMoviesInitialized();
103-
if (_isRerankSupported is bool cached)
116+
if (_isRerankSupported is { } cached)
104117
{
105118
return cached;
106119
}
107120
lock (_initLock)
108121
{
109-
if (_isRerankSupported is bool again)
122+
if (_isRerankSupported is { } again)
110123
{
111124
return again;
112125
}
113126
_isRerankSupported = ProbeRerankSupport();
114127
// ProbeRerankSupport fires a real $search+$rerank aggregate; clear so
115128
// tests asserting on captured aggregate events don't see the probe.
116-
EventCapturer?.Clear();
129+
// Run unconditionally — tests may have already attached capturers via
130+
// AddEventSubscriber before the first IsRerankSupported probe fires.
131+
ClearCapturedEvents();
117132
return _isRerankSupported.Value;
118133
}
119134
}
@@ -190,8 +205,6 @@ public void Dispose()
190205
Console.Error.WriteLine(
191206
$"AtlasSearchFixture: failed to drop database '{DatabaseName}': {ex}");
192207
}
193-
194-
Client.Dispose();
195208
}
196209

197210
// ---- Seeders ----
@@ -249,7 +262,7 @@ private void EnsureHistoricalInitialized()
249262
});
250263

251264
_historicalInitialized = true;
252-
EventCapturer?.Clear();
265+
ClearCapturedEvents();
253266
}
254267
}
255268

@@ -315,7 +328,7 @@ private void EnsureMoviesInitialized()
315328
});
316329

317330
_moviesInitialized = true;
318-
EventCapturer?.Clear();
331+
ClearCapturedEvents();
319332
}
320333
}
321334

@@ -371,7 +384,7 @@ private void EnsureEmbeddedMoviesInitialized()
371384
});
372385

373386
_embeddedMoviesInitialized = true;
374-
EventCapturer?.Clear();
387+
ClearCapturedEvents();
375388
}
376389
}
377390

@@ -415,7 +428,7 @@ private void EnsureAirbnbInitialized()
415428
});
416429

417430
_airbnbInitialized = true;
418-
EventCapturer?.Clear();
431+
ClearCapturedEvents();
419432
}
420433
}
421434

@@ -445,7 +458,7 @@ private void EnsureTestClassesInitialized()
445458
});
446459

447460
_testClassesInitialized = true;
448-
EventCapturer?.Clear();
461+
ClearCapturedEvents();
449462
}
450463
}
451464

@@ -476,7 +489,7 @@ private void EnsureBinaryVectorInitialized()
476489
});
477490

478491
_binaryVectorInitialized = true;
479-
EventCapturer?.Clear();
492+
ClearCapturedEvents();
480493
}
481494
}
482495

@@ -526,7 +539,7 @@ private void EnsureAutoEmbedInitialized()
526539
WaitForAutoEmbedIndexReady(collection, AutoEmbedIndexName);
527540

528541
_autoEmbedInitialized = true;
529-
EventCapturer?.Clear();
542+
ClearCapturedEvents();
530543
}
531544
}
532545

@@ -715,7 +728,7 @@ private void EnsureReturnScopeInitialized()
715728
});
716729

717730
_returnScopeInitialized = true;
718-
EventCapturer?.Clear();
731+
ClearCapturedEvents();
719732
}
720733
}
721734

@@ -861,5 +874,59 @@ private bool ProbeRerankSupport()
861874
return false;
862875
}
863876
}
877+
878+
// Drops any events buffered by currently-attached EventCapturers. Called by seed
879+
// paths whose driver-side commands would otherwise pollute a test's capture.
880+
private void ClearCapturedEvents() => _eventRouter.ClearCapturers();
881+
882+
/// <summary>
883+
/// Forwards events from the cluster to whichever <see cref="IEventSubscriber"/>s
884+
/// test classes have currently attached. The cluster's <see cref="EventPublisher"/>
885+
/// caches handlers on first publish, so this subscriber always reports a handler
886+
/// for every event type — that cached delegate then consults the live subscriber
887+
/// list at each publish, letting tests add/remove their own capturers at will.
888+
/// </summary>
889+
private sealed class CompositeEventSubscriber : IEventSubscriber
890+
{
891+
private readonly object _gate = new();
892+
private readonly List<IEventSubscriber> _subscribers = new();
893+
894+
public void Add(IEventSubscriber subscriber)
895+
{
896+
lock (_gate) _subscribers.Add(subscriber);
897+
}
898+
899+
public void Remove(IEventSubscriber subscriber)
900+
{
901+
lock (_gate) _subscribers.Remove(subscriber);
902+
}
903+
904+
public void ClearCapturers()
905+
{
906+
IEventSubscriber[] snapshot;
907+
lock (_gate) snapshot = _subscribers.ToArray();
908+
foreach (var s in snapshot)
909+
{
910+
(s as EventCapturer)?.Clear();
911+
}
912+
}
913+
914+
public bool TryGetEventHandler<TEvent>(out Action<TEvent> handler)
915+
{
916+
handler = e =>
917+
{
918+
IEventSubscriber[] snapshot;
919+
lock (_gate) snapshot = _subscribers.ToArray();
920+
foreach (var s in snapshot)
921+
{
922+
if (s.TryGetEventHandler<TEvent>(out var inner))
923+
{
924+
inner(e);
925+
}
926+
}
927+
};
928+
return true;
929+
}
930+
}
864931
}
865932
}

‎tests/MongoDB.Driver.Tests/Search/AtlasSearchIndexManagmentTests.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828

2929
namespace MongoDB.Driver.Tests.Search
3030
{
31-
[Category("AtlasSearchIndexHelpers")]
31+
[Trait("Category", "AtlasSearch")]
3232
[Collection(AtlasSearchCollection.Name)]
3333
public class AtlasSearchIndexManagementTests : LoggableTestClass
3434
{
@@ -50,7 +50,7 @@ private readonly BsonDocument _vectorIndexDefinition
5050

5151
public AtlasSearchIndexManagementTests(AtlasSearchFixture fixture, ITestOutputHelper testOutputHelper) : base(testOutputHelper)
5252
{
53-
RequireEnvironment.Check().EnvironmentVariable("ATLAS_SEARCH_INDEX_HELPERS_TESTS_ENABLED");
53+
RequireEnvironment.Check().EnvironmentVariable("ATLAS_SEARCH_TESTS_ENABLED");
5454
RequireEnvironment.Check().EnvironmentVariable("ATLAS_SEARCH_URI");
5555

5656
_mongoClient = fixture.Client;

‎tests/MongoDB.Driver.Tests/Search/AtlasSearchTests.cs‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -292,11 +292,17 @@ public AtlasSearchTests(AtlasSearchFixture fixture, ITestOutputHelper testOutput
292292

293293
_fixture = fixture;
294294
_mongoClient = fixture.Client;
295-
_eventCapturer = fixture.EventCapturer;
296295

297-
// Tests assert against the most recent aggregate (e.g., ValidateSearchStage uses .Single()),
298-
// so reset the shared capturer per-test rather than relying on a per-test client.
299-
_eventCapturer.Clear();
296+
// Each test instance owns its capturer so concurrent test classes sharing the
297+
// fixture's MongoClient can't observe (or clear) each other's events. xUnit
298+
// serializes within the AtlasSearch collection, so the capturer is single-writer.
299+
_eventCapturer = new EventCapturer().Capture<CommandStartedEvent>(e => e.CommandName == "aggregate");
300+
_fixture.AddEventSubscriber(_eventCapturer);
301+
}
302+
303+
protected override void DisposeInternal()
304+
{
305+
_fixture.RemoveEventSubscriber(_eventCapturer);
300306
}
301307

302308
[Fact]
@@ -899,8 +905,6 @@ public void Rerank()
899905
[Fact]
900906
public void SearchSequenceToken()
901907
{
902-
// Lean seed has 4 "flower"-titled movies; limit smaller than the matching set so the
903-
// SearchAfter / SearchBefore pagination exercise is still meaningful.
904908
const int limitVal = 4;
905909
var titles = new[]
906910
{

‎tests/MongoDB.Driver.Tests/Search/AutoEmbedVectorSearchTests.cs‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,6 @@ public AutoEmbedVectorSearchTests(AtlasSearchFixture fixture, ITestOutputHelper
4747

4848
_collection = fixture.GetAutoEmbedMoviesCollection<Movie>();
4949
_autoEmbedIndexName = AtlasSearchFixture.AutoEmbedIndexName;
50-
51-
// The fixture's EventCapturer is shared across every test class in the
52-
// collection. We don't assert on it here, but clear it so captured aggregate
53-
// events from AutoEmbedVectorSearchTests don't accumulate without bound.
54-
fixture.EventCapturer?.Clear();
5550
}
5651

5752
[Fact(Timeout = Timeout)]

0 commit comments

Comments
 (0)